lightbar: correctly revert to the S0/S3/S5 patterns

This CL ensures that temporary "one-shot" sequences such as KONAMI, TEST,
TAP, etc. will revert to the previous "normal" sequences even when
interrupted by other one-shot sequences.

This also adds a test for those cases.

BUG=chrome-os-partner:29873
BRANCH=ToT
TEST=manual

make runtests

Change-Id: Ie83908731acdf2f7c9108568a1ba047943175d26
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/213230

6 files changed, 373 insertions, 25 deletions

diff --git a/common/lb_common.c b/common/lb_common.c
index 289421d3bd..8803608328 100644
--- a/common/lb_common.c
+++ b/common/lb_common.c
@@ -68,6 +68,12 @@ static inline uint8_t controller_read(int ctrl_num, uint8_t reg)
 #define MAX_GREEN 0x55
 #define MAX_BLUE  0x67
 #endif
+#ifdef BOARD_HOST
+/* For testing only */
+#define MAX_RED   0xff
+#define MAX_GREEN 0xff
+#define MAX_BLUE  0xff
+#endif
 
 /* How we'd like to see the driver chips initialized. The controllers have some
  * auto-cycling capability, but it's not much use for our purposes. For now,
@@ -114,6 +120,10 @@ static const uint8_t led_to_isc[] = { 0x18, 0x15, 0x18, 0x15 };
 #ifdef BOARD_SAMUS
 static const uint8_t led_to_isc[] = { 0x15, 0x18, 0x15, 0x18 };
 #endif
+#ifdef BOARD_HOST
+/* For testing only */
+static const uint8_t led_to_isc[] = { 0x15, 0x18, 0x15, 0x18 };
+#endif
 
 /* Scale 0-255 into max value */
 static inline uint8_t scale_abs(int val, int max)
diff --git a/common/lightbar.c b/common/lightbar.c
index d08f12cf0b..e817b38da5 100644
--- a/common/lightbar.c
+++ b/common/lightbar.c
@@ -563,7 +563,6 @@ static uint32_t sequence_S5S3(void)
 static uint32_t sequence_S3S5(void)
 {
 	lb_off();
-	WAIT_OR_RET(-1);
 	return 0;
 }
 
@@ -571,6 +570,7 @@ static uint32_t sequence_S3S5(void)
  * nothing to do. We'll just wait here until the state changes. */
 static uint32_t sequence_S5(void)
 {
+	lb_off();
 	WAIT_OR_RET(-1);
 	return 0;
 }
@@ -644,9 +644,10 @@ static uint32_t sequence_PULSE(void)
 	return TASK_EVENT_CUSTOM(msg);
 }
 
-/* The host CPU (or someone) is going to poke at the lightbar directly, so we
- * don't want the EC messing with it. We'll just sit here and ignore all
- * other messages until we're told to continue. */
+/* The AP is going to poke at the lightbar directly, so we don't want the EC
+ * messing with it. We'll just sit here and ignore all other messages until
+ * we're told to continue (or until we think the AP is shutting down).
+ */
 static uint32_t sequence_STOP(void)
 {
 	uint32_t msg;
@@ -654,10 +655,12 @@ static uint32_t sequence_STOP(void)
 	do {
 		msg = TASK_EVENT_CUSTOM(task_wait_event(-1));
 		CPRINTS("LB_stop got pending_msg %d", pending_msg);
-	} while (msg != PENDING_MSG || pending_msg != LIGHTBAR_RUN);
-
-	/* Q: What should we do if the host shuts down? */
-	/* A: Nothing. We could be driving from the EC console. */
+	} while (msg != PENDING_MSG || (
+			 pending_msg != LIGHTBAR_RUN &&
+			 pending_msg != LIGHTBAR_S0S3 &&
+			 pending_msg != LIGHTBAR_S3 &&
+			 pending_msg != LIGHTBAR_S3S5 &&
+			 pending_msg != LIGHTBAR_S5));
 
 	CPRINTS("LB_stop->running");
 	return 0;
@@ -669,7 +672,10 @@ static uint32_t sequence_RUN(void)
 	return 0;
 }
 
-/* We shouldn't come here, but if we do it shouldn't hurt anything */
+/* We shouldn't come here, but if we do it shouldn't hurt anything. This
+ * sequence is to indicate an internal error in the lightbar logic, not an
+ * error with the Chromebook itself.
+ */
 static uint32_t sequence_ERROR(void)
 {
 	lb_init();
@@ -759,25 +765,33 @@ static const struct {
 	{4, 0x00, 0x00, 0x00, 100000},
 };
 
-static uint32_t sequence_KONAMI(void)
+static uint32_t sequence_KONAMI_inner(void)
 {
 	int i;
-	int tmp;
-
-	tmp = lb_get_brightness();
-	lb_set_brightness(255);
 
 	for (i = 0; i < ARRAY_SIZE(konami); i++) {
 		lb_set_rgb(konami[i].led,
 			   konami[i].r, konami[i].g, konami[i].b);
 		if (konami[i].delay)
-			usleep(konami[i].delay);
+			WAIT_OR_RET(konami[i].delay);
 	}
 
-	lb_set_brightness(tmp);
 	return 0;
 }
 
+static uint32_t sequence_KONAMI(void)
+{
+	int tmp;
+	uint32_t r;
+
+	/* Force brightness to max, then restore it */
+	tmp = lb_get_brightness();
+	lb_set_brightness(255);
+	r = sequence_KONAMI_inner();
+	lb_set_brightness(tmp);
+	return r;
+}
+
 /* Returns 0.0 to 1.0 for val in [min, min + ofs] */
 static float range(int val, int min, int ofs)
 {
@@ -846,14 +860,10 @@ static uint32_t sequence_TAP_inner(void)
 				   mult * st.p.color[ci].g,
 				   mult * st.p.color[ci].b);
 		}
-		/*
-		 * TODO: Use a different delay function here. Otherwise,
-		 * it's possible that a new sequence (such as KONAMI) can end
-		 * up with TAP as it's previous sequence. It's okay to return
-		 * early from TAP (or not), but we don't want to end up stuck
-		 * in the TAP sequence.
-		 */
+
 		WAIT_OR_RET(st.p.tap_tick_delay);
+
+		/* Return after some time has elapsed */
 		now = get_time();
 		if (now.le.lo - start.le.lo > st.p.tap_display_time)
 			break;
@@ -867,7 +877,10 @@ static uint32_t sequence_TAP(void)
 	uint32_t r;
 	uint8_t br, save[NUM_LEDS][3];
 
-	/* TODO(crosbug.com/p/29041): do we need more than lb_init() */
+	/* TODO(crosbug.com/p/29041): Do we need more than lb_init()?
+	 * Yes. And then we may need to turn it off again, if the AP is still
+	 * off when we're done.
+	 */
 	lb_init();
 	lb_on();
 
@@ -889,6 +902,12 @@ static uint32_t sequence_TAP(void)
 /* The main lightbar task. It just cycles between various pretty patterns. */
 /****************************************************************************/
 
+/* Distinguish "normal" sequences from one-shot sequences */
+static inline int is_normal_sequence(enum lightbar_sequence seq)
+{
+	return (seq >= LIGHTBAR_S5 && seq <= LIGHTBAR_S3S5);
+}
+
 /* Link each sequence with a command to invoke it. */
 struct lightbar_cmd_t {
 	const char * const string;
@@ -918,7 +937,8 @@ void lightbar_task(void)
 			CPRINTS("LB msg %d = %s", pending_msg,
 				lightbar_cmds[pending_msg].string);
 			if (st.cur_seq != pending_msg) {
-				st.prev_seq = st.cur_seq;
+				if (is_normal_sequence(st.cur_seq))
+					st.prev_seq = st.cur_seq;
 				st.cur_seq = pending_msg;
 			}
 		} else {
diff --git a/test/build.mk b/test/build.mk
index cb1ced36b1..a985a008b0 100644
--- a/test/build.mk
+++ b/test/build.mk
@@ -31,6 +31,7 @@ test-list-host+=thermal flash queue kb_8042 extpwr_gpio console_edit system
 test-list-host+=sbs_charging adapter host_command thermal_falco led_spring
 test-list-host+=bklight_lid bklight_passthru interrupt timer_dos button
 test-list-host+=motion_sense math_util sbs_charging_v2 battery_get_params_smart
+test-list-host+=lightbar
 
 adapter-y=adapter.o
 button-y=button.o
@@ -64,3 +65,4 @@ timer_calib-y=timer_calib.o
 timer_dos-y=timer_dos.o
 utils-y=utils.o
 battery_get_params_smart-y=battery_get_params_smart.o
+lightbar-y=lightbar.o
diff --git a/test/lightbar.c b/test/lightbar.c
new file mode 100644
index 0000000000..32473c9ccf
--- /dev/null
+++ b/test/lightbar.c
@@ -0,0 +1,294 @@
+/* Copyright 2014 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "console.h"
+#include "ec_commands.h"
+#include "lightbar.h"
+#include "host_command.h"
+#include "test_util.h"
+#include "timer.h"
+#include "util.h"
+
+static int get_seq(void)
+{
+	int rv;
+	struct ec_params_lightbar params;
+	struct ec_response_lightbar resp;
+
+	/* Get the state */
+	memset(&resp, 0, sizeof(resp));
+	params.cmd = LIGHTBAR_CMD_GET_SEQ;
+	rv = test_send_host_command(EC_CMD_LIGHTBAR_CMD, 0,
+				    &params, sizeof(params),
+				    &resp, sizeof(resp));
+	if (rv != EC_RES_SUCCESS) {
+		ccprintf("%s:%s(): rv = %d\n", __FILE__, __func__, rv);
+		return -1;
+	}
+
+	return resp.get_seq.num;
+}
+
+static int set_seq(int s)
+{
+	int rv;
+	struct ec_params_lightbar params;
+	struct ec_response_lightbar resp;
+
+	/* Get the state */
+	memset(&resp, 0, sizeof(resp));
+	params.cmd = LIGHTBAR_CMD_SEQ;
+	params.seq.num = s;
+	rv = test_send_host_command(EC_CMD_LIGHTBAR_CMD, 0,
+				    &params, sizeof(params),
+				    &resp, sizeof(resp));
+	if (rv != EC_RES_SUCCESS) {
+		ccprintf("%s:%s(): rv = %d\n", __FILE__, __func__, rv);
+		return -1;
+	}
+
+	return EC_RES_SUCCESS;
+}
+
+static int test_double_oneshots(void)
+{
+	/* Start in S0 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+	/* Invoke the oneshot */
+	TEST_ASSERT(set_seq(LIGHTBAR_TEST) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_TEST);
+	/* Switch to a different oneshot while that one's running */
+	TEST_ASSERT(set_seq(LIGHTBAR_KONAMI) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_KONAMI);
+	/* Afterwards, it should go back to the original normal state */
+	msleep(30 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+
+	/* Same test, but with a bunch more oneshots. */
+	TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+	TEST_ASSERT(set_seq(LIGHTBAR_TEST) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_TEST);
+	TEST_ASSERT(set_seq(LIGHTBAR_KONAMI) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_KONAMI);
+	TEST_ASSERT(set_seq(LIGHTBAR_TAP) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_TAP);
+	TEST_ASSERT(set_seq(LIGHTBAR_PULSE) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_PULSE);
+	TEST_ASSERT(set_seq(LIGHTBAR_KONAMI) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_KONAMI);
+	/* It should still go back to the original normal state */
+	msleep(30 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+
+	/* But if the interruption is a normal state, that should stick. */
+	TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+	TEST_ASSERT(set_seq(LIGHTBAR_TEST) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_TEST);
+	TEST_ASSERT(set_seq(LIGHTBAR_KONAMI) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_KONAMI);
+	/* Here's a normal sequence */
+	TEST_ASSERT(set_seq(LIGHTBAR_S3) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+	/* And another one-shot */
+	TEST_ASSERT(set_seq(LIGHTBAR_TAP) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_TAP);
+	TEST_ASSERT(set_seq(LIGHTBAR_PULSE) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_PULSE);
+	TEST_ASSERT(set_seq(LIGHTBAR_KONAMI) == EC_RES_SUCCESS);
+	msleep(MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_KONAMI);
+	/* It should go back to the new normal sequence */
+	msleep(30 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+
+	return EC_SUCCESS;
+}
+
+static int test_oneshots_norm_msg(void)
+{
+	/* Revert to the next state when interrupted with a normal message. */
+	enum lightbar_sequence seqs[] = {
+		LIGHTBAR_PULSE,
+		LIGHTBAR_TEST,
+		LIGHTBAR_KONAMI,
+		LIGHTBAR_TAP,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(seqs); i++) {
+		/* Start in S0 */
+		TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		/* Invoke the oneshot */
+		TEST_ASSERT(set_seq(seqs[i]) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		/* Interrupt with S0S3 */
+		TEST_ASSERT(set_seq(LIGHTBAR_S0S3) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		/* It should be back right away */
+		TEST_ASSERT(get_seq() == LIGHTBAR_S0S3);
+		/* And transition on to the correct value */
+		msleep(30 * MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+	}
+
+	return EC_SUCCESS;
+}
+
+static int test_stop_timeout(void)
+{
+	int i;
+
+	for (i = 0; i < LIGHTBAR_NUM_SEQUENCES; i++) {
+		/* Start in S0 */
+		TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		/* Tell it to stop */
+		TEST_ASSERT(set_seq(LIGHTBAR_STOP) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_STOP);
+		/* Try to interrupt it */
+		TEST_ASSERT(set_seq(i) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		/* What happened? */
+		if (i == LIGHTBAR_RUN ||
+		    i == LIGHTBAR_S0S3 || i ==  LIGHTBAR_S3 ||
+		    i ==  LIGHTBAR_S3S5 || i ==  LIGHTBAR_S5)
+			/* RUN or shutdown sequences should stop it */
+			TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+		else
+			/* All other sequences should be ignored */
+			TEST_ASSERT(get_seq() == LIGHTBAR_STOP);
+
+		/* Let it RUN again for the next iteration */
+		TEST_ASSERT(set_seq(LIGHTBAR_RUN) == EC_RES_SUCCESS);
+		msleep(MSEC);
+	}
+
+	TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+	return EC_SUCCESS;
+}
+
+static int test_oneshots_timeout(void)
+{
+	/* These should revert to the previous state after running */
+	enum lightbar_sequence seqs[] = {
+		LIGHTBAR_RUN,
+		LIGHTBAR_TEST,
+		LIGHTBAR_KONAMI,
+		LIGHTBAR_TAP,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(seqs); i++) {
+		TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+		msleep(MSEC);
+		TEST_ASSERT(set_seq(seqs[i]) == EC_RES_SUCCESS);
+		/* Assume the oneshot sequence takes at least a second (except
+		 * for LIGHTBAR_RUN, which returns immediately) */
+		if (seqs[i] != LIGHTBAR_RUN) {
+			msleep(MSEC);
+			TEST_ASSERT(get_seq() == seqs[i]);
+		}
+		msleep(30 * MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+	}
+
+	return EC_SUCCESS;
+}
+
+static int test_transition_states(void)
+{
+	/* S5S3 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S5S3) == EC_RES_SUCCESS);
+	msleep(10 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+
+	/* S3S0 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S3S0) == EC_RES_SUCCESS);
+	msleep(10 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+
+	/* S0S3 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S0S3) == EC_RES_SUCCESS);
+	msleep(10 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+
+	/* S3S5 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S3S5) == EC_RES_SUCCESS);
+	msleep(10 * MSEC);
+	TEST_ASSERT(get_seq() == LIGHTBAR_S5);
+
+	return EC_SUCCESS;
+}
+
+static int test_stable_states(void)
+{
+	int i;
+
+	/* Wait for the lightbar task to initialize */
+	msleep(500);
+
+	/* It should come up in S5 */
+	TEST_ASSERT(get_seq() == LIGHTBAR_S5);
+
+	/* It should stay there */
+	for (i = 0; i < 30; i++) {
+		msleep(MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_S5);
+	}
+
+	/* S3 is sticky, too */
+	TEST_ASSERT(set_seq(LIGHTBAR_S3) == EC_RES_SUCCESS);
+	for (i = 0; i < 30; i++) {
+		msleep(MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_S3);
+	}
+
+	/* And S0 */
+	TEST_ASSERT(set_seq(LIGHTBAR_S0) == EC_RES_SUCCESS);
+	for (i = 0; i < 30; i++) {
+		msleep(MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_S0);
+	}
+
+	/* PULSE is stable too, but nobody should care. Test it anyway. */
+	TEST_ASSERT(set_seq(LIGHTBAR_PULSE) == EC_RES_SUCCESS);
+	for (i = 0; i < 30; i++) {
+		msleep(MSEC);
+		TEST_ASSERT(get_seq() == LIGHTBAR_PULSE);
+	}
+
+	return EC_SUCCESS;
+}
+
+void run_test(void)
+{
+	RUN_TEST(test_stable_states);
+	RUN_TEST(test_transition_states);
+	RUN_TEST(test_oneshots_timeout);
+	RUN_TEST(test_stop_timeout);
+	RUN_TEST(test_oneshots_norm_msg);
+	RUN_TEST(test_double_oneshots);
+	test_print_result();
+}
diff --git a/test/lightbar.tasklist b/test/lightbar.tasklist
new file mode 100644
index 0000000000..71a89d6e07
--- /dev/null
+++ b/test/lightbar.tasklist
@@ -0,0 +1,18 @@
+/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+/**
+ * List of enabled tasks in the priority order
+ *
+ * The first one has the lowest priority.
+ *
+ * For each task, use the macro TASK_TEST(n, r, d, s) where :
+ * 'n' in the name of the task
+ * 'r' in the main routine of the task
+ * 'd' in an opaque parameter passed to the routine at startup
+ * 's' is the stack size in bytes; must be a multiple of 8
+ */
+#define CONFIG_TEST_TASK_LIST \
+        TASK_TEST(LIGHTBAR, lightbar_task, NULL, LARGER_TASK_STACK_SIZE)
diff --git a/test/test_config.h b/test/test_config.h
index adcda73578..299e3e7930 100644
--- a/test/test_config.h
+++ b/test/test_config.h
@@ -112,5 +112,9 @@ int board_discharge_on_ac(int enabled);
 #define I2C_PORT_CHARGER 1
 #endif
 
+#ifdef TEST_LIGHTBAR
+#define I2C_PORT_LIGHTBAR 1
+#endif
+
 #endif  /* TEST_BUILD */
 #endif  /* __CROS_EC_TEST_CONFIG_H */