Re: sbs-battery: Reading CHARGE in uAh

[Michael Heinemann <posted@heine.so>]

On 2017-07-07 10:27, Phil Reid wrote:
> On 7/07/2017 16:01, Michael Heinemann wrote:
>> Hi Phil,
>> On 2017-07-07 03:49, Phil Reid wrote:
>>> G'day Michael,
>>> On 6/07/2017 23:23, Michael Heinemann wrote:
>>>> 
>>>> 
>>>> I got an INSPIRED ENERGY NH2057 Battery which is compliant to SBS 
>>>> Specification Version 1.1.
>>>> 
>>>> The Battery is generally working but I noticed that the values I get 
>>>> from sysfs for the CHARGE values
>>>> are identical to the values I get as ENERGY and they are in uWh. 
>>>> According to the power_supply_class
>>>> documentation CHARGE should be in uAh and ENERGY in uWh.
>>>> 
>>>> So I took a look into the drivers/power/supply/sbs-battery.c and 
>>>> noticed a possible bug in the code
>>>> when switching from BATTERY_MODE_WATTS to BATTERY_MODE_AMPS:
>>>> 
>>>> The configuration bit, according to the SBS Spec, is bit 15 of the 
>>>> BatteryMode Register.
>>>> 
>>>> These two modes are reflected in the code as:
>>>> 
>>>> enum sbs_battery_mode {
>>>>      BATTERY_MODE_AMPS,
>>>>      BATTERY_MODE_WATTS
>>>> };
>>>> 
>>>> When I look into the 'sbs_set_battery_mode' function I don't see any 
>>>> point where these mode bits are
>>>> shifted to bit 15. When I debug I only see the register being 
>>>> changed from 0x8000 to 0x8001 and back.
>>>> 
>>>> So I'm a bit confused, is this actually working for somebody? I see 
>>>> in the 'for-next' branch [1] that
>>>> there is currently work done to add a mutex to the mode switch. So 
>>>> at least it seems to be working for
>>>> some people. But I don't see why. Am I missing something?
>>>> 
>>>> 
>>> 
>>> Your analyse looks correct. I've not look that closely at the data
>>> from the battery as yet.
>>> There's looks to be a few issues in there related to switching mode.
>>> 
>>> The return value from sbs_set_battery_mode also looks suspect.
>>> Perhaps it should just return 0 on success.
>>> 
>>> And then sbs_get_battery_capacity needs cleaning up as it's using the
>>> return value to set the mode at the end.
>>> 
>>> It setts the mode, and then I guess the intention was to set it back 
>>> again.
>>> But do we event need to do that? The next get is going to set the 
>>> correct mode.
>>> And the mode setting will soon be wrapped in a mutex.
>> 
>> I spend some more time to look at the code and my conclusion is that 
>> the cause
>> of the problem is a mix up of two different approaches for the mode.
>> 
>> The initial mode is passed in as 0 or 1 coming from enum 
>> sbs_battery_mode.
>> But when the setting back is done the value comes from the return 
>> value of
>> the previous call to sbs_set_battery_mode which is 0x8000 or 0x0000.
>> 
>> At least for my battery this leads to a situation where the status 
>> register is
>> 0x8000 so it's in mWh Mode. Then the mode=0 is passed in to set uAh.
>> 
>> But this statement
>> 
>> if ((original_val & BATTERY_MODE_MASK) == mode)
>>          return mode;
>> 
>> is true by mistake ((0x8000 & 0x8000) == 0) so the function exits and 
>> never
>> sets anything.
> Yeap that what I thought.
> 
>> 
>> What would be the "kernel style" way to fix this?
> That I'm not sure of. My personal style would be against using the enum 
> value.
> Unless is was set to a constant. ie:
> BATTERY_MODE_AMPS  = 0,
> BATTERY_MODE_WATTS = 0x8000
> But I'd probably make the enum a #define instead with above value
> 
> 
>> Define the sbs_battery_mode like this:
>> 
>> enum sbs_battery_mode {
>>      BATTERY_MODE_AMPS,
>>      BATTERY_MODE_WATTS
>> };
>> 
>> Or add a #define CAPACITY_MODE_OFFSET 0xf and shift the mode bit in 
>> the function?
>> 
>> I tested a bit with the shifted bit and the added mutex and get I 
>> correct values now.
>> Without the mutex it's more or less randomly changing between uAh and 
>> mWh.
>> I would writeup a patch and submit it as proposal.
>> 
>>  From my understanding of the SBS Spec (Page 19) the intention of the 
>> Mode setting is
>> to select one Mode for your power source and leave it at that. For 
>> Example the AtRate()
>> calculations become wrong when you change the mode. But as the driver 
>> is not using
>> these features that might not be an issue.
> Had a quick look at that and it'd have to do the same thing as the
> sbs_get_battery_capacity
> call and use the mutex if it was implemented. Don't think we need to
> wrory about it at present thou.
> 
>> 
>> I would agree that the driver does not need to change back the Mode 
>> bit as it is
>> set on every read anyway. So this is unnecessary overhead.
>> If I find time I'll submit a patch to remove this and clean up a bit.
>> 
> I like any speed improvement. It takes a while to poll all the values
> from the battery.

I did some more testing and came to a bit disappointing outcome. I
don't know if this is specific to my battery or a general thing with
SBS devices.
When I set the CAPACITY_MODE bit the change is not always effective
immediately. Sometimes it works, sometimes it takes a few ms and
sometimes the bit never changes and I have to write it again.

So there seems to be no guarantee to get the reading in the expected
format.

How could this be handled? Return an error when the CAPACITY_MODE bit
was not set succesfully? Retry setting it (which takes time)?

I guess if some service is using the values from the battery it
normaly relies on ENERGY or CHARGE and not both. So not setting
back the mode after each read would be a first step in the
right direction. What do you think?

Regards,
Michael