FS#9312 - Charging Code for Gigabeat S

Attached to Project: Rockbox
Opened by Michael Sevakis (MikeS) - Saturday, 23 August 2008, 13:39 GMT
Last edited by Michael Sevakis (MikeS) - Sunday, 21 December 2008, 18:13 GMT
Task Type Patches
Category Battery/Charging
Status Closed
Assigned To Michael Sevakis (MikeS)
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


Enables charging for Gigabeat S.

Can charge through USB. When connected to USB only, the unit seems to have trouble actually doing that since the disk always spinning when connected. At least it seems that way at the moment.

The AC adapter will supply enough current to charge regardless.
This task depends upon

Closed by  Michael Sevakis (MikeS)
Sunday, 21 December 2008, 18:13 GMT
Reason for closing:  Accepted
Additional comments about closing:  Ladies and gentlemen, we have charging! The readings look good. Thanks to all who tested.
Comment by Michael Sevakis (MikeS) - Saturday, 23 August 2008, 15:01 GMT
Extends the CV timeout from 120 to 180 minutes since the hw kicks it in a little early.
Comment by Bryan Childs (GodEater) - Wednesday, 27 August 2008, 14:01 GMT
Hopefully applying #8943 should take care of the disk spinning - certainly seems to have done in my case.
Comment by Andrew Mahone (Unhelpful) - Sunday, 14 September 2008, 01:46 GMT
The problem I'm seeing is pretty simple. When the player transitions from plugged->unplugged, the battery reading is stuck at 100%, and the debug->battery second page shows "State: Error". Both problems clear when it is plugged in again, but reappear on unplug.

After adding some additional debug data, I traced the point at which the error state is set to stat_battery_reading, which returns failure unconditionally if the CHGDETS is not set. Modifying the test to fixes or works around the error state, but I am not sure if this is correct. I've attached a patch for this work-around, to be applied over the existing charging patch. I'd especially like to be sure this is not a hardware problem, so if somebody else with an S could verify that the charging patch exhibits the same behavior, that would be a big help.
Comment by Michael Sevakis (MikeS) - Sunday, 14 September 2008, 04:34 GMT
I guess it's a workaround for now but doesn't a replug reset the error state? It is supposed to and it does in fact do so when I replug on my device.

I can get an error from the stat but it's only if I do things rapidly, not every time since the stat is only done during transitions. It's easier to make happen by unplugging USB with no charger connected (so it is trying to charge from USB)
Comment by Andrew Mahone (Unhelpful) - Sunday, 14 September 2008, 20:16 GMT
Plugin resets the error state, but it's set again on unplug, and as a result rockbox only reports battery % correctly while plugged in, which is not too useful. I just ran tried plugging and unplugging, five trials each of USB and AC, and it never fails to enter the error state on unplug. There may be some difference between units, or something wrong with mine - it *is* a motherboard from one "broken" player and the rest from another, but everything else seems to work correctly, and the OF doesn't complain in any way about unplug.

If this is a real error, maybe instead of some work-around or other to hide it, it would be more appropriate to have rockbox still report the battery % normally while CHARGE_STATE_ERROR is set. This seems sensible to me, if this state really only indicates an error with the charger or charging code, and not with the battery itself. Or I can keep hunting for whatever race condition leads to charging_algorithm_small_step being called with charger_input_state not agreeing with CHGDETS.
Comment by Michael Sevakis (MikeS) - Sunday, 14 September 2008, 20:52 GMT
OF is designed differently so who knows what is does internally.

I think I will just have it distinguish an ADC error from a "no charger" error which really isn't an error but it should quit trying to charge if that happens. The "race" condition will always be there and must be ok since the thread's info is always slightly behind physical events happening to the player.
Comment by Andrew Mahone (Unhelpful) - Sunday, 14 September 2008, 21:57 GMT
The patch as it is isn't hiding any ADC errors, to my knowledge. stat_battery_reading with my patch will still return error on an ADC read error, but will return the read value if CHGDETS is off. Would it be more appropriate to return zero instead of the actual value read?
Comment by Andrew Mahone (Unhelpful) - Monday, 15 September 2008, 00:52 GMT
... nevermind, I see what you were saying. What about testing at the top of charging_algorithm_small_step? Will it mess up other things in the power thread to set charger_input_state there?
Comment by Michael Sevakis (MikeS) - Thursday, 18 September 2008, 09:01 GMT
What has to change is voltage_to_battery_level because any error state is basically equivalent to DISCHARGING. Additionally, with this algorithm for LiIon, TRICKLE/TOPOFF _do not_ imply the battery is 100%. TRICKLE means the battery is very, very low and TOPOFF means the constant current mode is operating.
Comment by Andrew Mahone (Unhelpful) - Thursday, 18 September 2008, 10:36 GMT
I already had a patch doing pretty much that, except for the TRICKLE/TOPOFF handling. Here's that, with the TRICKLE/TOPOFF stuff added. I'm not sure if I ought to remove the cap at 99% for TOPOFF. Does the charger ever actually exit TOPOFF while plugged?
Comment by Michael Sevakis (MikeS) - Thursday, 18 September 2008, 23:51 GMT
It exits TOPOFF when the charging cycle finishes or gets an error. You never keep a LiIon battery on a constant maintenence charge; you retstart the cycle when, in this case, it drops back to 4.05V which should just recycle in TOPOFF.

I was also reworking that function last night in a different manner so that the target can massage the state to something more general.
Comment by Andrew Mahone (Unhelpful) - Friday, 19 September 2008, 09:54 GMT
If it's going to cycle back and forth between discharge and topoff states, I would think clamping battery % to 99 during topoff doesn't make sense, then. You shouldn't even have to special-case it, if trickle is only ever used on low battery - and then I guess you could just remove special cases entirely and always report the percentage calculated from the battery voltage.
Comment by Michael Sevakis (MikeS) - Saturday, 20 September 2008, 09:12 GMT
How do you figure that? A LiIon cell is only, what, 70% charged when it hits the constant voltage portion of the cycle (that's with a nice "brick wall" of a regulator). Should it say it's at 100% long before finishing?

Conversion of voltage to % when charging will definitely be a different curve if it is to be accurate because of the rise from internal cell resistance and other effects.
Comment by Andrew Mahone (Unhelpful) - Sunday, 21 September 2008, 04:06 GMT
The first try I took at this simply treated topoff and trickle as charging, instead of assuming them to mean 100%. I read the MIN in that branch as being there to prevent a "charging" battery from reading full... but if the percent->voltage table is correct, shouldn't that keep it from setting the level to 100% before charging is reasonably close to finished?
Comment by Michael Sevakis (MikeS) - Sunday, 21 September 2008, 13:44 GMT
You have to account for the fact that there is an initial steep voltage drop during discharge, so the peak voltage really should be considered as over 100%. This also allows for variations in voltage after settling after charging.

Existing code is was written for software-controlled NiCd and NiMh charging, 100% hardware-controlled charging with software monitoring or no charging at all.
Comment by Michael Sevakis (MikeS) - Monday, 22 September 2008, 13:31 GMT
We'll just dump the error for transient unplug. It should stabilize itself and it gets around the problems for now. After a charging rework it should be possible to refine.
Comment by Michael Sevakis (MikeS) - Sunday, 19 October 2008, 23:34 GMT
Ok, I'm just throwing in an update word here (no patch yet). I have simplified things a bit but have discovered that to make things tenable with a USB connection sans charger, at least some juice is needed just to maintain and power the device when NOT charging. The OF does do that since the battery does not drain while plugged into USB only.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 05 November 2008, 12:49 GMT
resync and a subtle request for a committable update :p
Comment by Robert Menes (RMenes379) - Monday, 10 November 2008, 16:12 GMT
Jonathan's update works quite nicely on my beast. :)
Comment by Michael Sevakis (MikeS) - Monday, 10 November 2008, 17:33 GMT
I am working towards a committable update. If you don't mind USB off temporarily I'd feel safe about committing now. Getting things right for USB without charger is holding me up (how to maintain the battery with the cable current at proper topoff).
Comment by Michael Sevakis (MikeS) - Wednesday, 19 November 2008, 05:41 GMT
Update to keep things synced. A little more simplification and development of it was done too. I'm still thinking through the USB battery maintainance bit however.
Comment by Michael Sevakis (MikeS) - Wednesday, 19 November 2008, 06:53 GMT
Update again. Incidental parts of the patch were committed already to simplify it.
Comment by William Peters (w1ll14m) - Wednesday, 10 December 2008, 14:34 GMT
Wonderful patch! :)
As far as can tell this works really good, i've been using these patches for over a month now.

This patch needs a resync to work with current revision (19383).
Comment by Michael Sevakis (MikeS) - Friday, 19 December 2008, 11:06 GMT
This is a release candidate that handles USB charging and powering. Charging from USB-only will result in less run time than from the adaptor but that is how the hardware works and is true of the original firmware. It does it's best to maintain the battery at all times when on USB only. Perhaps some minor tweeks will be needed but this seem to work well.

This has a few fancy readouts in the battery debug menu as well. I'll probably keep those even though a couple aren't used for charging. :)
Comment by Michael Sevakis (MikeS) - Friday, 19 December 2008, 11:22 GMT
Remove fmradio_i2c.h change not relevant to patch.
Comment by Michael Sevakis (MikeS) - Friday, 19 December 2008, 13:57 GMT
Heh. I committed some code that was in the previous patch. Resync. Has a couple tweeks in handling the disabled state.
Comment by Alex Parker (BigBambi) - Friday, 19 December 2008, 17:19 GMT
This works brilliantly on my beast with a larger aftermarket battery as well as the standard battery. Thanks very much for all your work on this Mike, it is much appreciated! :)
Comment by Michael Sevakis (MikeS) - Saturday, 20 December 2008, 10:09 GMT
It would be good to open-circuit battery readings after each type of charging is complete for comparison. The AC adapter has priority over USB power.

For USB-only charging, after chaging is reported to have stopped, plug the A/C adapter, flip the battery switch off, shut the player down and reboot. The fast you do this, the better. Observe voltage reported by the bootloader (hold a key while booting to pause the screen). Battery must be less than or equal to 4.0V to trigger USB charging.

For AC charging (you should be able to do this immediately after USB charging), simply wait a few minutes until the battery voltage settles. It will flatten-out in the voltage history graph. Battery must be less than or equal to 4.1V to trigger ac charging.

Give readings for each and what capacity battery was charged. It should always settle to under 4.2V. Mine settles around 4.176V for AC and 4.09V for USB-only with the 700mAh OEM battery.
Comment by Andrew Mahone (Unhelpful) - Saturday, 20 December 2008, 11:30 GMT
4.183V on AC / 4.095V on USB for 700mAh OEM battery,
Comment by Nils Wallménius (nls) - Saturday, 20 December 2008, 12:23 GMT
4.205V on AC for me with a standard battery, didn't test usb yet.
Comment by Michael Sevakis (MikeS) - Saturday, 20 December 2008, 12:41 GMT
4.205V _after_ the regulator turned off and the battery settled?
Comment by Alex Parker (BigBambi) - Saturday, 20 December 2008, 19:20 GMT
4.113V after USB charging for me, 1000 mAh replacement battery. Will do A/C now.

Edit: 4.174V after A/C charging, with same 1000 mAh battery
Comment by Nils Wallménius (nls) - Sunday, 21 December 2008, 10:44 GMT
Mike, ah, no, it stopped charging at 4.205, don't remember exactly where it settled after that but around 4.15 i think.