| Message ID | 1337668019-6875-1-git-send-email-alex.hung@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
Thanks Alex, a useful test, just a few very minor quibbles: On 22/05/12 07:26, Alex Hung wrote: > Signed-off-by: Alex Hung <alex.hung@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; > + } minor { positioning, use only four "=" signs in message and some re-phrasing. if (!fwts_battery_check_trip_point_support(fw, index)) { fwts_printf("fw, "==== Not supported - skipping 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; is there any possibility to sanity check the trip point just in case it is set incorrectly by default? Are there any invalid values that we can check for? if not, no worries. > + > + 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"); how about using the following to keep it consistent with the other battery test messages: fwts_printf(fw, "==== Please PLUG IN 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"; can we remove file.. > + > + 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); ..and change the snprintf() to include alarm: snprintf(path, sizeof(path), "%s/%s/alarm", FWTS_PROC_ACPI_BATTERY, just makes it a little tidier IMHO. > + 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:"; same again, we can just put file and field as literals below: > + > + 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); snprintf(path, sizeof(path), "%s/%s/alarm", > + 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) && if (strstr(buffer, "alarm:") && > + 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; > I don't have any kit that is capable of being testing with this, I presume you've tested it? Apart from these minor changes, it looks OK to me. Thanks! Colin
On 05/22/2012 02:26 PM, Alex Hung wrote: > Signed-off-by: Alex Hung<alex.hung@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 ? 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;
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@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; >
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; + + 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;
Signed-off-by: Alex Hung <alex.hung@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(+)