diff mbox

lib: fwts_battery: fix incorrect strings for a battery with unit of mWh

Message ID 1337780657-28856-1-git-send-email-alex.hung@canonical.com
State Accepted
Headers show

Commit Message

Alex Hung May 23, 2012, 1:44 p.m. UTC 
Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/lib/src/fwts_battery.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Colin Ian King May 23, 2012, 5:19 p.m. UTC | #1 
Although we did discuss this on irc today, I'm still struggling to 
understand why this is required, can explain it a little more for me. 
Thanks Alex.

Colin


On 23/05/12 14:44, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>   src/lib/src/fwts_battery.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 5f21dfa..468c616 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -45,11 +45,11 @@ static int fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>   	switch (type) {
>   	case FWTS_BATTERY_DESIGN_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>   		break;
>   	case FWTS_BATTERY_REMAINING_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>   		break;
>   	default:
>   		return FWTS_ERROR;
>
Alex Hung May 24, 2012, 2:34 a.m. UTC | #2 
Hi Colin,

It was actually one bug that led to another topics

== Topic 1 ==

The one that is related to fwts is simple: when I was testing the the 
trip point patch, I found fwts_battery_get_capacity_sys_fs always 
returns 0 for remaining capacity. After some digging in, it is because 
the strings mismatch as the patch.

The below is the output from my thinkpad x200
========================================================
@x200:~$ cat /sys/class/power_supply/BAT0/uevent
POWER_SUPPLY_NAME=BAT0
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CYCLE_COUNT=0
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=14400000
POWER_SUPPLY_VOLTAGE_NOW=16684000
POWER_SUPPLY_POWER_NOW=0
POWER_SUPPLY_ENERGY_FULL_DESIGN=28800000
POWER_SUPPLY_ENERGY_FULL=24430000
POWER_SUPPLY_ENERGY_NOW=24430000
POWER_SUPPLY_MODEL_NAME=42T4646
POWER_SUPPLY_MANUFACTURER=SANYO
POWER_SUPPLY_SERIAL_NUMBER= 1720
========================================================

== Topic 2 ==

The led to the discussion we had - I was testing on x220 which has such 
problem, and then it turns on thinkpad sometimes returns power unit as 
mAh and sometimes as mWh, and this seems to be a BIOS bug - I will send 
Lenovo for their feedbacks.

Cheers,
Alex Hung

On 05/24/2012 01:19 AM, Colin Ian King wrote:
> Although we did discuss this on irc today, I'm still struggling to
> understand why this is required, can explain it a little more for me.
> Thanks Alex.
>
> Colin
>
>
> On 23/05/12 14:44, Alex Hung wrote:
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>> src/lib/src/fwts_battery.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
>> index 5f21dfa..468c616 100644
>> --- a/src/lib/src/fwts_battery.c
>> +++ b/src/lib/src/fwts_battery.c
>> @@ -45,11 +45,11 @@ static int
>> fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>> switch (type) {
>> case FWTS_BATTERY_DESIGN_CAPACITY:
>> field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
>> - field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
>> + field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>> break;
>> case FWTS_BATTERY_REMAINING_CAPACITY:
>> field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
>> - field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
>> + field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>> break;
>> default:
>> return FWTS_ERROR;
>>
>
>
Ivan Hu May 25, 2012, 6:42 a.m. UTC | #3 
On 05/23/2012 09:44 PM, Alex Hung wrote:
> Signed-off-by: Alex Hung<alex.hung@canonical.com>
> ---
>   src/lib/src/fwts_battery.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 5f21dfa..468c616 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -45,11 +45,11 @@ static int fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>   	switch (type) {
>   	case FWTS_BATTERY_DESIGN_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>   		break;
>   	case FWTS_BATTERY_REMAINING_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>   		break;
>   	default:
>   		return FWTS_ERROR;
Acked-by: Ivan Hu<ivan.hu@canonical.com>
Ivan Hu May 25, 2012, 6:54 a.m. UTC | #4 
On 05/24/2012 10:34 AM, Alex Hung wrote:
> Hi Colin,
>
> It was actually one bug that led to another topics
>
> == Topic 1 ==
>
> The one that is related to fwts is simple: when I was testing the the 
> trip point patch, I found fwts_battery_get_capacity_sys_fs always 
> returns 0 for remaining capacity. After some digging in, it is because 
> the strings mismatch as the patch.
>
> The below is the output from my thinkpad x200
> ========================================================
> @x200:~$ cat /sys/class/power_supply/BAT0/uevent
> POWER_SUPPLY_NAME=BAT0
> POWER_SUPPLY_STATUS=Full
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=0
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=14400000
> POWER_SUPPLY_VOLTAGE_NOW=16684000
> POWER_SUPPLY_POWER_NOW=0
> POWER_SUPPLY_ENERGY_FULL_DESIGN=28800000
> POWER_SUPPLY_ENERGY_FULL=24430000
> POWER_SUPPLY_ENERGY_NOW=24430000
> POWER_SUPPLY_MODEL_NAME=42T4646
> POWER_SUPPLY_MANUFACTURER=SANYO
> POWER_SUPPLY_SERIAL_NUMBER= 1720
> ========================================================
>
> == Topic 2 ==
>
> The led to the discussion we had - I was testing on x220 which has 
> such problem, and then it turns on thinkpad sometimes returns power 
> unit as mAh and sometimes as mWh, and this seems to be a BIOS bug - I 
> will send Lenovo for their feedbacks.
>
> Cheers,
> Alex Hung
>
> On 05/24/2012 01:19 AM, Colin Ian King wrote:
>> Although we did discuss this on irc today, I'm still struggling to
>> understand why this is required, can explain it a little more for me.
>> Thanks Alex.
>>
>> Colin
>>
>>
>> On 23/05/12 14:44, Alex Hung wrote:
>>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>>> ---
>>> src/lib/src/fwts_battery.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
>>> index 5f21dfa..468c616 100644
>>> --- a/src/lib/src/fwts_battery.c
>>> +++ b/src/lib/src/fwts_battery.c
>>> @@ -45,11 +45,11 @@ static int
>>> fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>>> switch (type) {
>>> case FWTS_BATTERY_DESIGN_CAPACITY:
>>> field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
>>> - field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
>>> + field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>>> break;
>>> case FWTS_BATTERY_REMAINING_CAPACITY:
>>> field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
>>> - field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
>>> + field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>>> break;
>>> default:
>>> return FWTS_ERROR;
>>>
>>
>>
>
>

Hi Alex,

I've checked the uevent on my lenovo x220,

======================================
POWER_SUPPLY_NAME=BAT0
POWER_SUPPLY_STATUS=Unknown
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CYCLE_COUNT=0
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=14800000
POWER_SUPPLY_VOLTAGE_NOW=16422000
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_CHARGE_FULL_DESIGN=2886000
POWER_SUPPLY_CHARGE_FULL=2883000
POWER_SUPPLY_CHARGE_NOW=2787000
POWER_SUPPLY_MODEL_NAME=42T4901
POWER_SUPPLY_MANUFACTURER=Panasonic
POWER_SUPPLY_SERIAL_NUMBER= 1794
======================================

it seems using "POWER_SUPPLY_CHARGE_FULL".

best regards,
Ivan
Alex Hung May 25, 2012, 6:59 a.m. UTC | #5 
On 05/25/2012 02:54 PM, IvanHu wrote:
> On 05/24/2012 10:34 AM, Alex Hung wrote:
>> Hi Colin,
>>
>> It was actually one bug that led to another topics
>>
>> == Topic 1 ==
>>
>> The one that is related to fwts is simple: when I was testing the the
>> trip point patch, I found fwts_battery_get_capacity_sys_fs always
>> returns 0 for remaining capacity. After some digging in, it is because
>> the strings mismatch as the patch.
>>
>> The below is the output from my thinkpad x200
>> ========================================================
>> @x200:~$ cat /sys/class/power_supply/BAT0/uevent
>> POWER_SUPPLY_NAME=BAT0
>> POWER_SUPPLY_STATUS=Full
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>> POWER_SUPPLY_CYCLE_COUNT=0
>> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=14400000
>> POWER_SUPPLY_VOLTAGE_NOW=16684000
>> POWER_SUPPLY_POWER_NOW=0
>> POWER_SUPPLY_ENERGY_FULL_DESIGN=28800000
>> POWER_SUPPLY_ENERGY_FULL=24430000
>> POWER_SUPPLY_ENERGY_NOW=24430000
>> POWER_SUPPLY_MODEL_NAME=42T4646
>> POWER_SUPPLY_MANUFACTURER=SANYO
>> POWER_SUPPLY_SERIAL_NUMBER= 1720
>> ========================================================
>>
>> == Topic 2 ==
>>
>> The led to the discussion we had - I was testing on x220 which has
>> such problem, and then it turns on thinkpad sometimes returns power
>> unit as mAh and sometimes as mWh, and this seems to be a BIOS bug - I
>> will send Lenovo for their feedbacks.
>>
>> Cheers,
>> Alex Hung
>>
>> On 05/24/2012 01:19 AM, Colin Ian King wrote:
>>> Although we did discuss this on irc today, I'm still struggling to
>>> understand why this is required, can explain it a little more for me.
>>> Thanks Alex.
>>>
>>> Colin
>>>
>>>
>>> On 23/05/12 14:44, Alex Hung wrote:
>>>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>>>> ---
>>>> src/lib/src/fwts_battery.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
>>>> index 5f21dfa..468c616 100644
>>>> --- a/src/lib/src/fwts_battery.c
>>>> +++ b/src/lib/src/fwts_battery.c
>>>> @@ -45,11 +45,11 @@ static int
>>>> fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>>>> switch (type) {
>>>> case FWTS_BATTERY_DESIGN_CAPACITY:
>>>> field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
>>>> - field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
>>>> + field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>>>> break;
>>>> case FWTS_BATTERY_REMAINING_CAPACITY:
>>>> field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
>>>> - field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
>>>> + field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>>>> break;
>>>> default:
>>>> return FWTS_ERROR;
>>>>
>>>
>>>
>>
>>
>
> Hi Alex,
>
> I've checked the uevent on my lenovo x220,
>
> ======================================
> POWER_SUPPLY_NAME=BAT0
> POWER_SUPPLY_STATUS=Unknown
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=0
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=14800000
> POWER_SUPPLY_VOLTAGE_NOW=16422000
> POWER_SUPPLY_CURRENT_NOW=0
> POWER_SUPPLY_CHARGE_FULL_DESIGN=2886000
> POWER_SUPPLY_CHARGE_FULL=2883000
> POWER_SUPPLY_CHARGE_NOW=2787000
> POWER_SUPPLY_MODEL_NAME=42T4901
> POWER_SUPPLY_MANUFACTURER=Panasonic
> POWER_SUPPLY_SERIAL_NUMBER= 1794
> ======================================
>
> it seems using "POWER_SUPPLY_CHARGE_FULL".
>
> best regards,
> Ivan
>

Yes, it is a BIOS bug(?) that it returns mAh in _BIF (therefore 
POWER_SUPPLY_CHARGE_FULL) but it sometimes will change to mWh (and 
therefore POWER_SUPPLY_ENERGY_FULL. I was not able to duplicate it but 
it should be related to s3.
Colin Ian King May 25, 2012, 8:30 a.m. UTC | #6 
On 23/05/12 14:44, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>   src/lib/src/fwts_battery.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 5f21dfa..468c616 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -45,11 +45,11 @@ static int fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
>   	switch (type) {
>   	case FWTS_BATTERY_DESIGN_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
>   		break;
>   	case FWTS_BATTERY_REMAINING_CAPACITY:
>   		field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
> -		field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
> +		field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
>   		break;
>   	default:
>   		return FWTS_ERROR;
>
I can't fully recall why I had this set this way, I think I wrote this 
on a plane and I may have easily made a mistake, so I'm very much 
inclined to go with Alex's changes and if we have evidence later on that 
this needs more attention then we will need to re-visit this. But for 
now, lets go with this.  Thanks Alex for spotting this issue.

Acked-by: Colin Ian King <colin.king@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
index 5f21dfa..468c616 100644
--- a/src/lib/src/fwts_battery.c
+++ b/src/lib/src/fwts_battery.c
@@ -45,11 +45,11 @@  static int fwts_battery_get_capacity_sys_fs(fwts_framework *fw,
 	switch (type) {
 	case FWTS_BATTERY_DESIGN_CAPACITY:
 		field_mAh = "POWER_SUPPLY_CHARGE_FULL_DESIGN=";
-		field_mWh = "ENERGY_SUPPLY_CHARGE_FULL_DESIGN=";
+		field_mWh = "POWER_SUPPLY_ENERGY_FULL_DESIGN=";
 		break;
 	case FWTS_BATTERY_REMAINING_CAPACITY:
 		field_mAh = "POWER_SUPPLY_CHARGE_NOW=";
-		field_mWh = "ENERGY_SUPPLY_CHARGE_NOW=";
+		field_mWh = "POWER_SUPPLY_ENERGY_NOW=";
 		break;
 	default:
 		return FWTS_ERROR;