[PATCH] acpi: battery: added trip point tests for acpi batteries.

[Alex Hung]

On 05/25/2012 11:37 AM, IvanHu wrote:
> On 05/22/2012 02:26 PM, Alex Hung wrote:
>> Signed-off-by: Alex Hung<alex.hung at canonical.com>
>> ---
>> src/acpi/battery/battery.c | 41 ++++++++
>> src/lib/include/fwts_battery.h | 3 +
>> src/lib/src/fwts_battery.c | 215 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 259 insertions(+)
>>
>> diff --git a/src/acpi/battery/battery.c b/src/acpi/battery/battery.c
>> index d9e4d5e..206a443 100644
>> --- a/src/acpi/battery/battery.c
>> +++ b/src/acpi/battery/battery.c
>> @@ -188,6 +188,46 @@ static void
>> check_battery_cycle_count(fwts_framework *fw, int index, char *name)
>>
>> }
>>
>> +static void check_battery_trip_point(fwts_framework *fw, int index,
>> char *name)
>> +{
>> + int trip_point;
>> + int trip_point_org;
>> + fwts_printf(fw, "==== Checking trip point of battery '%s' ====\n",
>> name);
>> +
>> + if (!fwts_battery_check_trip_point_support(fw, index))
>> + {
>> + fwts_printf(fw, "===== not supported - skip test ====\n");
>> + return;
>> + }
>> +
>> + if (fwts_battery_get_trip_point(fw, index,&trip_point) == FWTS_OK)
>> + trip_point_org = trip_point;
>> + else
>> + trip_point_org = 0;
>
> Hi Alex,
>
> It seems that fwts_battery_check_trip_point_support depends on calling
> the fwts_battery_get_trip_point function to see if it is successful and
> checks trip_point value.
> So, above code will get trip point value twice. Is it possible to get
> trip point once and add the checking here ?

My original intended fwts_battery_check_trip_point_support() is a little 
more sophisticated than this, but it was blocked by a bug in kernel 
(/acpi/battery.c). I submitted a patch to Linux kernel.

The algorithm is supposed to be
1. call fwts_battery_get_trip_point to get trip point (or not supported)
2. call fwts_battery_set_trip_point
3. call fwts_battery_get_trip_point to confirm the set fails (i.e. still 
not supported).

PS. the patch is to fix a bug in (2)

Calling fwts_battery_get_trip_point to get 0 is assuming nobody changes 
"alarm" previously and relying on the fact Linux kernel check _BIX and 
use warning_level as default for trip point. This is only partially 
right in limited scenario. Until the patch is merged to Linux kernel, 
the above algorithm cannot be included fwts; however, I still want to 
use a function to remind that a check is necessary even though it is too 
simple for the moment.

>
> Part from this minor thoughts, it looks OK to me. Thanks!
>
> Best regards,
> Ivan
>
>> +
>> + fwts_log_info(fw, "Test battery '%s' downward trip point.", name);
>> + fwts_printf(fw, "==== Please now UNPLUG the AC power of the machine
>> ====\n");
>> + fwts_press_enter(fw);
>> + sleep(2);
>> + trip_point = get_full(fw, index) - 5;
>> + fwts_battery_set_trip_point(fw, index, trip_point);
>> + fwts_cpu_consume_start();
>> + wait_for_acpi_event(fw, name);
>> + fwts_cpu_consume_complete();
>> +
>> + fwts_log_info(fw, "Test battery '%s' upwards trip point.", name);
>> + fwts_printf(fw, "==== Please now PLUG the AC power of the machine
>> ====\n");
>> + fwts_press_enter(fw);
>> + sleep(2);
>> + trip_point = get_full(fw, index) + 3;
>> + fwts_battery_set_trip_point(fw, index, trip_point);
>> + wait_for_acpi_event(fw, name);
>> + trip_point = get_full(fw, index);
>> +
>> + if (trip_point_org != 0)
>> + fwts_battery_set_trip_point(fw, index, trip_point_org);
>> +}
>> +
>> static void do_battery_test(fwts_framework *fw, int index)
>> {
>> char name[PATH_MAX];
>> @@ -211,6 +251,7 @@ static void do_battery_test(fwts_framework *fw,
>> int index)
>> wait_for_acpi_event(fw, name);
>> check_charging(fw, index, name);
>> check_battery_cycle_count(fw, index, name);
>> + check_battery_trip_point(fw, index, name);
>> }
>>
>> static int battery_test1(fwts_framework *fw)
>> diff --git a/src/lib/include/fwts_battery.h
>> b/src/lib/include/fwts_battery.h
>> index 83d381b..8fbd838 100644
>> --- a/src/lib/include/fwts_battery.h
>> +++ b/src/lib/include/fwts_battery.h
>> @@ -27,6 +27,9 @@
>>
>> int fwts_battery_get_count(fwts_framework *fw, int *count);
>> int fwts_battery_get_cycle_count(fwts_framework *fw, int index, int
>> *cycle_count);
>> +bool fwts_battery_check_trip_point_support(fwts_framework *fw, int
>> index);
>> +int fwts_battery_set_trip_point(fwts_framework *fw, int index, int
>> trip_point);
>> +int fwts_battery_get_trip_point(fwts_framework *fw, int index, int
>> *trip_point);
>> int fwts_battery_get_capacity(fwts_framework *fw, int type, int index,
>> uint32_t *capacity_mAh, uint32_t *capacity_mWh);
>> int fwts_battery_get_name(fwts_framework *fw, int index, char *name);
>>
>> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
>> index d8cc318..2fcc657 100644
>> --- a/src/lib/src/fwts_battery.c
>> +++ b/src/lib/src/fwts_battery.c
>> @@ -356,6 +356,221 @@ static int
>> fwts_battery_get_cycle_count_proc_fs(fwts_framework *fw, DIR *dir, in
>> return FWTS_OK;
>> }
>>
>> +static int fwts_battery_set_trip_point_sys_fs(fwts_framework *fw, DIR
>> *dir, int index, int trip_point)
>> +{
>> + struct dirent *entry;
>> + int i = 0;
>> +
>> + do {
>> + entry = readdir(dir);
>> + if (entry&& strlen(entry->d_name)> 2) {
>> + char path[PATH_MAX];
>> + char *data;
>> + FILE *fp;
>> + bool match;
>> +
>> + /* Check that type field matches the expected type */
>> + snprintf(path, sizeof(path), "%s/%s/type",
>> FWTS_SYS_CLASS_POWER_SUPPLY, entry->d_name);
>> + if ((data = fwts_get(path)) != NULL) {
>> + bool mismatch = (strstr(data, "Battery") == NULL);
>> + free(data);
>> + if (mismatch)
>> + continue; /* type don't match, skip this entry */
>> + } else
>> + continue; /* can't check type, skip this entry */
>> + match = ((index == FWTS_BATTERY_ALL) || (index == i));
>> + i++;
>> + if (!match)
>> + continue;
>> +
>> + snprintf(path, sizeof(path), "%s/%s/alarm",
>> FWTS_SYS_CLASS_POWER_SUPPLY, entry->d_name);
>> + if ((fp = fopen(path, "rw+")) == NULL) {
>> + fwts_log_info(fw, "Battery %s present but undersupported - no state
>> present.", entry->d_name);
>> + } else {
>> + char buffer[512];
>> + sprintf(buffer, "%d", trip_point * 1000);
>> + fputs(buffer, fp);
>> + fclose(fp);
>> + }
>> + }
>> + } while (entry);
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +static int fwts_battery_get_trip_point_sys_fs(fwts_framework *fw, DIR
>> *dir, int index, int *trip_point)
>> +{
>> + struct dirent *entry;
>> + int i = 0;
>> +
>> + *trip_point = 0;
>> + do {
>> + entry = readdir(dir);
>> + if (entry&& strlen(entry->d_name)> 2) {
>> + char path[PATH_MAX];
>> + char *data;
>> + int val;
>> + FILE *fp;
>> + bool match;
>> +
>> + /* Check that type field matches the expected type */
>> + snprintf(path, sizeof(path), "%s/%s/type",
>> FWTS_SYS_CLASS_POWER_SUPPLY, entry->d_name);
>> + if ((data = fwts_get(path)) != NULL) {
>> + bool mismatch = (strstr(data, "Battery") == NULL);
>> + free(data);
>> + if (mismatch)
>> + continue; /* type don't match, skip this entry */
>> + } else
>> + continue; /* can't check type, skip this entry */
>> + match = ((index == FWTS_BATTERY_ALL) || (index == i));
>> + i++;
>> + if (!match)
>> + continue;
>> +
>> + snprintf(path, sizeof(path), "%s/%s/alarm",
>> FWTS_SYS_CLASS_POWER_SUPPLY, entry->d_name);
>> + if ((fp = fopen(path, "r")) == NULL) {
>> + fwts_log_info(fw, "Battery %s present but undersupported - no state
>> present.", entry->d_name);
>> + } else {
>> + char buffer[4096];
>> + while (fgets(buffer, sizeof(buffer)-1, fp) != NULL) {
>> + sscanf(buffer, "%d",&val);
>> + *trip_point = val / 1000;
>> + }
>> + fclose(fp);
>> + }
>> + }
>> + } while (entry);
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +static int fwts_battery_set_trip_point_proc_fs(fwts_framework *fw,
>> DIR *dir, int index, int trip_point)
>> +{
>> + struct dirent *entry;
>> + char *file;
>> + int i = 0;
>> +
>> + file = "alarm";
>> +
>> + do {
>> + entry = readdir(dir);
>> + if (entry&& strlen(entry->d_name)> 2) {
>> + char path[PATH_MAX];
>> + FILE *fp;
>> + bool match = ((index == FWTS_BATTERY_ALL) || (index == i));
>> +
>> + i++;
>> + if (!match)
>> + continue;
>> +
>> + snprintf(path, sizeof(path), "%s/%s/%s", FWTS_PROC_ACPI_BATTERY,
>> entry->d_name, file);
>> + if ((fp = fopen(path, "rw+")) == NULL) {
>> + fwts_log_info(fw, "Battery %s present but undersupported - no state
>> present.", entry->d_name);
>> + } else {
>> + char buffer[512];
>> + sprintf(buffer, "%d", trip_point);
>> + fputs(buffer, fp);
>> + fclose(fp);
>> + }
>> + }
>> + } while (entry);
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +static int fwts_battery_get_trip_point_proc_fs(fwts_framework *fw,
>> DIR *dir, int index, int *trip_point)
>> +{
>> + struct dirent *entry;
>> + char *file;
>> + char *field;
>> + int i = 0;
>> +
>> + *trip_point = 0;
>> + file = "alarm";
>> + field = "alarm:";
>> +
>> + do {
>> + entry = readdir(dir);
>> + if (entry&& strlen(entry->d_name)> 2) {
>> + char path[PATH_MAX];
>> + int val;
>> + FILE *fp;
>> + bool match = ((index == FWTS_BATTERY_ALL) || (index == i));
>> +
>> + i++;
>> + if (!match)
>> + continue;
>> +
>> + snprintf(path, sizeof(path), "%s/%s/%s", FWTS_PROC_ACPI_BATTERY,
>> entry->d_name, file);
>> + if ((fp = fopen(path, "rw+")) == NULL) {
>> + fwts_log_info(fw, "Battery %s present but undersupported - no state
>> present.", entry->d_name);
>> + } else {
>> + char buffer[4096];
>> + while (fgets(buffer, sizeof(buffer)-1, fp) != NULL) {
>> + if (strstr(buffer, field)&&
>> + strlen(buffer)> 25) {
>> + sscanf(buffer + 25, "%d",&val);
>> + *trip_point = val;
>> + break;
>> + }
>> + }
>> + fclose(fp);
>> + }
>> + }
>> + } while (entry);
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +int fwts_battery_set_trip_point(fwts_framework *fw, int index, int
>> trip_point)
>> +{
>> + int ret;
>> + DIR *dir;
>> +
>> + if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
>> + ret = fwts_battery_set_trip_point_sys_fs(fw, dir, index, trip_point);
>> + closedir(dir);
>> + } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
>> + ret = fwts_battery_set_trip_point_proc_fs(fw, dir, index, trip_point);
>> + closedir(dir);
>> + } else {
>> + return FWTS_ERROR;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +int fwts_battery_get_trip_point(fwts_framework *fw, int index, int
>> *trip_point)
>> +{
>> + int ret;
>> + DIR *dir;
>> +
>> + if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
>> + ret = fwts_battery_get_trip_point_sys_fs(fw, dir, index, trip_point);
>> + closedir(dir);
>> + } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
>> + ret = fwts_battery_get_trip_point_proc_fs(fw, dir, index, trip_point);
>> + closedir(dir);
>> + } else {
>> + return FWTS_ERROR;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +bool fwts_battery_check_trip_point_support(fwts_framework *fw, int
>> index)
>> +{
>> + int trip_point;
>> +
>> + if (!(fwts_battery_get_trip_point(fw, index,&trip_point) == FWTS_OK))
>> + return false;
>> +
>> + if (trip_point == 0)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> int fwts_battery_get_cycle_count(fwts_framework *fw, int index, int
>> *cycle_count)
>> {
>> int ret;
>