Re: sbs-battery: Reading CHARGE in uAh

[Michael Heinemann <posted@heine.so>]

On 2017-07-10 03:35, Phil Reid wrote:
> On 7/07/2017 21:06, Michael Heinemann wrote:
>> 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)?
> 
> Where you getting an i2c error or it just failing to set.
> I've got a number of different Inspired Energy batteries on hand that
> I can test against.
> I'll try and run some tests with you patch today and see what I get 
> with them.
> 
The i2c calls return successfully. The value reported is in the wrong 
unit and
the status register is also not changed.

I saw that you introduced a delay, this should help at least in
~50% of the times. Do you always see correct values with your
batteries?

I added something like this at the end of sbs_set_battery_mode

     ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
     if ((ret & BATTERY_MODE_MASK) != mode)
                return -ENODATA;

In this way one knows if the value is actually in the right unit
which as it is currently implemented is not the case. But this of
course adds more delay.

I will now do some tests with your submitted patches.


> I have had problems with some older models not reporting data
> correctly under some conditions.
> Inspired energy acknowledged a bug in the battery at the time.
> We've been using them for a while, but not with the linux driver and
> I've not used the mode bit before.
> 
>> 
>> 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?
>>