[PATCH] acpi: battery: added trip point tests for acpi batteries.
Colin Ian King
colin.king at canonical.com
Tue May 22 08:37:34 UTC 2012
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 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;
> + }
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
More information about the fwts-devel
mailing list