chgstv2: Add unit test for battery sustainer

This patch adds a unit test for the battery sustainer.

BUG=b:188457962
BRANCH=None
TEST=make run-sbs_charging_v2

Change-Id: Ica227cf4ee3f71a746150fb6a5f4e40ab8ca0720
Signed-off-by: Daisuke Nojiri <[email protected]>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2987734
Reviewed-by: Vincent Palatin <[email protected]>

Author: dnojiri

common/charge_state_v2.c

@@ -15,8 +15,6 @@
 #include "common.h"
 #include "console.h"
 #include "ec_commands.h"
-#include "ec_ec_comm_client.h"
-#include "ec_ec_comm_server.h"
 #include "extpower.h"
 #include "gpio.h"
 #include "hooks.h"
@@ -205,7 +203,11 @@ static void problem(enum problem_type p, int v)
 	problems_exist = 1;
 }
 
-#ifdef CONFIG_EC_EC_COMM_BATTERY_MASTER
+
+enum ec_charge_control_mode get_chg_ctrl_mode(void)
+{
+	return chg_ctl_mode;
+}
 static int battery_sustainer_set(int8_t lower, int8_t upper)
 {
 	if (lower == -1 || upper == -1) {
@@ -238,7 +240,7 @@ static bool battery_sustainer_enabled(void)
 	return sustain_soc.lower != -1 && sustain_soc.upper != -1;
 }
 
-#ifdef CONFIG_EC_EC_COMM_BATTERY_CLIENT
+#ifdef CONFIG_EC_EC_COMM_BATTERY_MASTER
 /*
  * Parameters for dual-battery policy.
  * TODO(b:71881017): This should be made configurable by AP in the future.
@@ -1130,6 +1132,9 @@ static void dump_charge_state(void)
 #define DUMP_CHG(FLD, FMT) ccprintf("\t" #FLD " = " FMT "\n", curr.chg. FLD)
 #define DUMP_BATT(FLD, FMT) ccprintf("\t" #FLD " = " FMT "\n", curr.batt. FLD)
 #define DUMP_OCPC(FLD, FMT) ccprintf("\t" #FLD " = " FMT "\n", curr.ocpc. FLD)
+
+	enum ec_charge_control_mode cmode = get_chg_ctrl_mode();
+
 	ccprintf("state = %s\n", state_list[curr.state]);
 	DUMP(ac, "%d");
 	DUMP(batt_is_charging, "%d");
@@ -1185,8 +1190,8 @@ static void dump_charge_state(void)
 	DUMP(input_voltage, "%dmV");
 #endif
 	ccprintf("chg_ctl_mode = %s (%d)\n",
-		 chg_ctl_mode < CHARGE_CONTROL_COUNT
-		 	? mode_text[chg_ctl_mode] : "UNDEF", chg_ctl_mode);
+		 cmode < CHARGE_CONTROL_COUNT ? mode_text[cmode] : "UNDEF",
+		 cmode);
 	ccprintf("manual_voltage = %d\n", manual_voltage);
 	ccprintf("manual_current = %d\n", manual_current);
 	ccprintf("user_current_limit = %dmA\n", user_current_limit);
@@ -1204,6 +1209,7 @@ static void dump_charge_state(void)
 static void show_charging_progress(void)
 {
 	int rv = 0, minutes, to_full, chgnum = 0;
+	int dsoc;
 
 #ifdef CONFIG_BATTERY_SMART
 	/*
@@ -1237,19 +1243,17 @@ static void show_charging_progress(void)
 	}
 #endif
 
+	dsoc = charge_get_display_charge();
 	if (rv)
 		CPRINTS("Battery %d%% (Display %d.%d %%) / ??h:?? %s%s",
 			curr.batt.state_of_charge,
-			curr.batt.display_charge / 10,
-			curr.batt.display_charge % 10,
+			dsoc / 10, dsoc % 10,
 			to_full ? "to full" : "to empty",
 			is_full ? ", not accepting current" : "");
 	else
 		CPRINTS("Battery %d%% (Display %d.%d %%) / %dh:%d %s%s",
 			curr.batt.state_of_charge,
-			curr.batt.display_charge / 10,
-			curr.batt.display_charge % 10,
-			minutes / 60, minutes % 60,
+			dsoc / 10, dsoc % 10, minutes / 60, minutes % 60,
 			to_full ? "to full" : "to empty",
 			is_full ? ", not accepting current" : "");
 
@@ -1675,7 +1679,7 @@ static int battery_outside_charging_temperature(void)
 
 static void sustain_battery_soc(void)
 {
-	enum ec_charge_control_mode mode = chg_ctl_mode;
+	enum ec_charge_control_mode mode = get_chg_ctrl_mode();
 	int soc;
 	int rv;
 
@@ -1686,7 +1690,7 @@ static void sustain_battery_soc(void)
 
 	soc = charge_get_display_charge() / 10;
 
-	switch (chg_ctl_mode) {
+	switch (mode) {
 	case CHARGE_CONTROL_NORMAL:
 		/* Going up */
 		if (sustain_soc.upper < soc)
@@ -1707,7 +1711,7 @@ static void sustain_battery_soc(void)
 		return;
 	}
 
-	if (mode == chg_ctl_mode)
+	if (mode == get_chg_ctrl_mode())
 		return;
 
 	rv = set_chg_ctrl_mode(mode);
@@ -1994,7 +1998,7 @@ void charger_task(void *u)
 		/* Okay, we're on AC and we should have a battery. */
 
 		/* Used for factory tests. */
-		if (chg_ctl_mode != CHARGE_CONTROL_NORMAL) {
+		if (get_chg_ctrl_mode() != CHARGE_CONTROL_NORMAL) {
 			set_charge_state(ST_IDLE);
 			goto wait_for_it;
 		}
@@ -2090,7 +2094,7 @@ void charger_task(void *u)
 
 wait_for_it:
 #ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
-		if (chg_ctl_mode == CHARGE_CONTROL_NORMAL) {
+		if (get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL) {
 			sleep_usec = charger_profile_override(&curr);
 			if (sleep_usec < 0)
 				problem(PR_CUSTOM, sleep_usec);
@@ -2148,7 +2152,7 @@ void charger_task(void *u)
 #endif
 		    (is_full != prev_full) ||
 		    (curr.state != prev_state) ||
-		    (curr.batt.display_charge != prev_disp_charge)) {
+		    (charge_get_display_charge() != prev_disp_charge)) {
 			sustain_battery_soc();
 			show_charging_progress();
 			prev_charge = curr.batt.state_of_charge;
@@ -2489,7 +2493,7 @@ uint32_t charge_get_flags(void)
 {
 	uint32_t flags = 0;
 
-	if (chg_ctl_mode != CHARGE_CONTROL_NORMAL)
+	if (get_chg_ctrl_mode() != CHARGE_CONTROL_NORMAL)
 		flags |= CHARGE_FLAG_FORCE_IDLE;
 	if (curr.ac)
 		flags |= CHARGE_FLAG_EXTERNAL_POWER;
@@ -2510,7 +2514,7 @@ int charge_get_percent(void)
 	return is_full ? 100 : curr.batt.state_of_charge;
 }
 
-int charge_get_display_charge(void)
+test_mockable int charge_get_display_charge(void)
 {
 	return curr.batt.display_charge;
 }
@@ -2709,7 +2713,7 @@ charge_command_charge_control(struct host_cmd_handler_args *args)
 
 	if (args->version >= 2) {
 		if (p->cmd == EC_CHARGE_CONTROL_CMD_SET) {
-			if (chg_ctl_mode == CHARGE_CONTROL_NORMAL) {
+			if (get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL) {
 				rv = battery_sustainer_set(
 						p->sustain_soc.lower,
 						p->sustain_soc.upper);
@@ -2721,7 +2725,7 @@ charge_command_charge_control(struct host_cmd_handler_args *args)
 				battery_sustainer_disable();
 			}
 		} else if (p->cmd == EC_CHARGE_CONTROL_CMD_GET) {
-			r->mode = chg_ctl_mode;
+			r->mode = get_chg_ctrl_mode();
 			r->sustain_soc.lower = sustain_soc.lower;
 			r->sustain_soc.upper = sustain_soc.upper;
 			args->response_size = sizeof(*r);
@@ -2735,22 +2739,6 @@ charge_command_charge_control(struct host_cmd_handler_args *args)
 	if (rv != EC_SUCCESS)
 		return EC_RES_ERROR;
 
-	if (args->version >= 2) {
-		/*
-		 * If charge mode is explicitly set (e.g. DISCHARGE), make sure
-		 * sustain charge is disabled. To go back to normal mode (and
-		 * disable sustain charge), set mode=NORMAL, lower=-1, upper=-1.
-		 */
-		if (chg_ctl_mode == CHARGE_CONTROL_NORMAL) {
-			rv = sustain_soc_set(p->sustain_charge.lower,
-						p->sustain_charge.upper);
-			if (rv)
-				return EC_RES_INVALID_PARAM;
-		} else {
-			sustain_soc_disable();
-		}
-	}
-
 #ifdef CONFIG_CHARGER_DISCHARGE_ON_AC
 #ifdef CONFIG_CHARGER_DISCHARGE_ON_AC_CUSTOM
 	rv = board_discharge_on_ac(p->mode == CHARGE_CONTROL_DISCHARGE);
@@ -3072,7 +3060,7 @@ int charge_get_charge_state_debug(int param, uint32_t *value)
 {
 	switch (param) {
 	case CS_PARAM_DEBUG_CTL_MODE:
-		*value = chg_ctl_mode;
+		*value = get_chg_ctrl_mode();
 		break;
 	case CS_PARAM_DEBUG_MANUAL_CURRENT:
 		*value = manual_current;

include/ec_commands.h

@@ -4173,6 +4173,8 @@ enum ec_charge_control_mode {
 	CHARGE_CONTROL_NORMAL = 0,
 	CHARGE_CONTROL_IDLE,
 	CHARGE_CONTROL_DISCHARGE,
+	/* Add no more entry below. */
+	CHARGE_CONTROL_COUNT,	
 };
 
 #define EC_CHARGE_MODE_TEXT { \

test/sbs_charging_v2.c

@@ -19,11 +19,14 @@
 #define WAIT_CHARGER_TASK 600
 #define BATTERY_DETACH_DELAY 35000
 
+enum ec_charge_control_mode get_chg_ctrl_mode(void);
+
 static int mock_chipset_state = CHIPSET_STATE_ON;
 static int is_shutdown;
 static int is_force_discharge;
 static int is_hibernated;
 static int override_voltage, override_current, override_usec;
+static int display_soc;
 
 /* The simulation doesn't really hibernate, so we must reset this ourselves */
 extern timestamp_t shutdown_target_time;
@@ -114,10 +117,15 @@ static int charge_control(enum ec_charge_control_mode mode)
 {
 	struct ec_params_charge_control params;
 	params.mode = mode;
-	return test_send_host_command(EC_CMD_CHARGE_CONTROL, 1, &params,
+	return test_send_host_command(EC_CMD_CHARGE_CONTROL, 2, &params,
 				      sizeof(params), NULL, 0);
 }
 
+__override int charge_get_display_charge(void)
+{
+	return display_soc;
+}
+
 /* Setup init condition */
 static void test_setup(int on_ac)
 {
@@ -711,7 +719,78 @@ static int test_low_battery_hostevents(void)
 	return EC_SUCCESS;
 }
 
+static int test_battery_sustainer(void)
+{
+	struct ec_params_charge_control p;
+	struct ec_response_charge_control r;
+	int rv;
+
+	test_setup(1);
+
+	/* Enable sustainer */
+	p.cmd = EC_CHARGE_CONTROL_CMD_SET;
+	p.mode = CHARGE_CONTROL_NORMAL;
+	p.sustain_soc.lower = 79;
+	p.sustain_soc.upper = 80;
+	rv = test_send_host_command(EC_CMD_CHARGE_CONTROL, 2,
+				    &p, sizeof(p), NULL, 0);
+	TEST_ASSERT(rv == EC_RES_SUCCESS);
+
+	p.cmd = EC_CHARGE_CONTROL_CMD_GET;
+	rv = test_send_host_command(EC_CMD_CHARGE_CONTROL, 2,
+				    &p, sizeof(p), &r, sizeof(r));
+	TEST_ASSERT(rv == EC_RES_SUCCESS);
+	TEST_ASSERT(r.sustain_soc.lower == 79);
+	TEST_ASSERT(r.sustain_soc.upper == 80);
+
+	/* Check mode transition as the SoC changes. */
+
+	/* SoC < lower < upper */
+	display_soc = 780;
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL);
+
+	/* lower < upper < SoC */
+	display_soc = 810;
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_DISCHARGE);
 
+	/* Unplug AC. Sustainer gets deactivated. */
+	gpio_set_level(GPIO_AC_PRESENT, 0);
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL);
+
+	/* Replug AC. Sustainer gets re-activated. */
+	gpio_set_level(GPIO_AC_PRESENT, 1);
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_DISCHARGE);
+
+	/* lower < SoC < upper */
+	display_soc = 799;
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_IDLE);
+
+	/* SoC < lower < upper */
+	display_soc = 789;
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL);
+
+	/* Disable sustainer */
+	p.cmd = EC_CHARGE_CONTROL_CMD_SET;
+	p.mode = CHARGE_CONTROL_NORMAL;
+	p.sustain_soc.lower = -1;
+	p.sustain_soc.upper = -1;
+	rv = test_send_host_command(EC_CMD_CHARGE_CONTROL, 2,
+				    &p, sizeof(p), NULL, 0);
+	TEST_ASSERT(rv == EC_RES_SUCCESS);
+
+	/* This time, mode will stay in NORMAL even when upper < SoC. */
+	display_soc = 810;
+	wait_charging_state();
+	TEST_ASSERT(get_chg_ctrl_mode() == CHARGE_CONTROL_NORMAL);
+
+	return EC_SUCCESS;
+}
 
 void run_test(int argc, char **argv)
 {
@@ -724,6 +803,7 @@ void run_test(int argc, char **argv)
 	RUN_TEST(test_hc_charge_state);
 	RUN_TEST(test_hc_current_limit);
 	RUN_TEST(test_low_battery_hostevents);
+	RUN_TEST(test_battery_sustainer);
 
 	test_print_result();
 }