Fix group0 state (#382)

* Add debugging / remove extraneous fields from debug log

* Always force re-initialization of group 0 state

* Always re-set cached value for group 0

* Copy state before setting it to avoid group 0 re-initialization clobbering it

* add comment

* Document test running

* Add tests for group0

README.md

@@ -268,6 +268,18 @@ You can add an arbitrary number of UDP gateways through the REST API or through
 
 You can select between versions 5 and 6 of the UDP protocol (documented [here](http://www.limitlessled.com/dev/)). Version 6 has support for the newer RGB+CCT bulbs and also includes response packets, which can theoretically improve reliability. Version 5 has much smaller packets and is probably lower latency.
 
+## Development
+
+#### Running tests
+
+Run unit tests with this command:
+
+```
+pio test -e d1_mini
+```
+
+substituting `d1_mini` for the environment of your choice.
+
 ## Acknowledgements
 
 * @WoodsterDK added support for LT8900 radios.

lib/MiLightState/GroupState.cpp

@@ -533,6 +533,11 @@ bool GroupState::applyIncrementCommand(GroupStateField field, IncrementDirection
 }
 
 bool GroupState::patch(const GroupState& other) {
+#ifdef STATE_DEBUG
+  other.debugState("Patching existing state with: ");
+  Serial.println();
+#endif
+
   for (size_t i = 0; i < size(ALL_PHYSICAL_FIELDS); ++i) {
     GroupStateField field = ALL_PHYSICAL_FIELDS[i];
 
@@ -615,7 +620,7 @@ bool GroupState::patch(const JsonObject& state) {
   return changes;
 }
 
-void GroupState::applyColor(ArduinoJson::JsonObject& state) {
+void GroupState::applyColor(ArduinoJson::JsonObject& state) const {
   uint8_t rgb[3];
   RGBConverter converter;
   converter.hsvToRgb(
@@ -628,14 +633,14 @@ void GroupState::applyColor(ArduinoJson::JsonObject& state) {
   applyColor(state, rgb[0], rgb[1], rgb[2]);
 }
 
-void GroupState::applyColor(ArduinoJson::JsonObject& state, uint8_t r, uint8_t g, uint8_t b) {
+void GroupState::applyColor(ArduinoJson::JsonObject& state, uint8_t r, uint8_t g, uint8_t b) const {
   JsonObject& color = state.createNestedObject("color");
   color["r"] = r;
   color["g"] = g;
   color["b"] = b;
 }
 
-void GroupState::applyOhColor(ArduinoJson::JsonObject& state) {
+void GroupState::applyOhColor(ArduinoJson::JsonObject& state) const {
   uint8_t rgb[3];
   RGBConverter converter;
   converter.hsvToRgb(
@@ -651,7 +656,7 @@ void GroupState::applyOhColor(ArduinoJson::JsonObject& state) {
 }
 
 // gather partial state for a single field; see GroupState::applyState to gather many fields
-void GroupState::applyField(JsonObject& partialState, const BulbId& bulbId, GroupStateField field) {
+void GroupState::applyField(JsonObject& partialState, const BulbId& bulbId, GroupStateField field) const {
   if (isSetField(field)) {
     switch (field) {
       case GroupStateField::STATE:
@@ -750,7 +755,7 @@ void GroupState::applyField(JsonObject& partialState, const BulbId& bulbId, Grou
 }
 
 // helper function to debug the current state (in JSON) to the serial port
-void GroupState::debugState(char const *debugMessage) {
+void GroupState::debugState(char const *debugMessage) const {
 #ifdef STATE_DEBUG
   // using static to keep large buffers off the call stack
   static StaticJsonBuffer<500> jsonBuffer;
@@ -759,17 +764,14 @@ void GroupState::debugState(char const *debugMessage) {
   GroupStateField fields[] {
       GroupStateField::BRIGHTNESS,
       GroupStateField::BULB_MODE,
-      GroupStateField::COLOR,
       GroupStateField::COLOR_TEMP,
-      GroupStateField::COMPUTED_COLOR,
       GroupStateField::EFFECT,
       GroupStateField::HUE,
       GroupStateField::KELVIN,
-      GroupStateField::LEVEL,
       GroupStateField::MODE,
       GroupStateField::SATURATION,
-      GroupStateField::STATE,
-      GroupStateField::STATUS };
+      GroupStateField::STATE
+  };
 
   // since our buffer is reused, make sure to clear it every time
   jsonBuffer.clear();
@@ -779,7 +781,7 @@ void GroupState::debugState(char const *debugMessage) {
   BulbId id;
 
   // use applyState to build JSON of all fields (from above)
-  applyState(jsonState, id, fields, 13);
+  applyState(jsonState, id, fields, size(fields));
   // convert to string and print
   Serial.printf("%s: ", debugMessage);
   jsonState.printTo(Serial);
@@ -789,7 +791,7 @@ void GroupState::debugState(char const *debugMessage) {
 
 // build up a partial state representation based on the specified GrouipStateField array.  Used
 // to gather a subset of states (configurable in the UI) for sending to MQTT and web responses.
-void GroupState::applyState(JsonObject& partialState, const BulbId& bulbId, GroupStateField* fields, size_t numFields) {
+void GroupState::applyState(JsonObject& partialState, const BulbId& bulbId, GroupStateField* fields, size_t numFields) const {
   for (size_t i = 0; i < numFields; i++) {
     applyField(partialState, bulbId, fields[i]);
   }

lib/MiLightState/GroupState.h

@@ -127,8 +127,8 @@ public:
   // support fields like DEVICE_ID, which aren't otherweise available to the
   // state in this class.  The alternative is to have every GroupState object
   // keep a reference to its BulbId, which feels too heavy-weight.
-  void applyField(JsonObject& state, const BulbId& bulbId, GroupStateField field);
-  void applyState(JsonObject& state, const BulbId& bulbId, GroupStateField* fields, size_t numFields);
+  void applyField(JsonObject& state, const BulbId& bulbId, GroupStateField field) const;
+  void applyState(JsonObject& state, const BulbId& bulbId, GroupStateField* fields, size_t numFields) const;
 
   // Attempt to keep track of increment commands in such a way that we can
   // know what state it's in.  When we get an increment command (like "increase 
@@ -149,7 +149,7 @@ public:
   void load(Stream& stream);
   void dump(Stream& stream) const;
 
-  void debugState(char const *debugMessage);
+  void debugState(char const *debugMessage) const;
 
   static const GroupState& defaultState(MiLightRemoteType remoteType);
 
@@ -201,10 +201,10 @@ private:
   StateData state;
   TransientData scratchpad;
 
-  void applyColor(JsonObject& state, uint8_t r, uint8_t g, uint8_t b);
-  void applyColor(JsonObject& state);
+  void applyColor(JsonObject& state, uint8_t r, uint8_t g, uint8_t b) const;
+  void applyColor(JsonObject& state) const;
   // Apply OpenHAB-style color, e.g., {"color":"0,0,0"}
-  void applyOhColor(JsonObject& state);
+  void applyOhColor(JsonObject& state) const;
 };
 
 extern const BulbId DEFAULT_BULB_ID;

lib/MiLightState/GroupStateStore.cpp

@@ -10,7 +10,8 @@ GroupStateStore::GroupStateStore(const size_t maxSize, const size_t flushRate)
 GroupState* GroupStateStore::get(const BulbId& id) {
   GroupState* state = cache.get(id);
 
-  if (state == NULL) {
+  // Always force re-initialization of group 0 state
+  if (id.groupId == 0 || state == NULL) {
     trackEviction();
     GroupState loadedState = GroupState::defaultState(id.deviceType);
 
@@ -21,12 +22,15 @@ GroupState* GroupStateStore::get(const BulbId& id) {
     // ID 0, so we can't always ignore group 0.
     const MiLightRemoteConfig* remoteConfig = MiLightRemoteConfig::fromType(id.deviceType);
 
-    if (id.groupId != 0 || remoteConfig == NULL || remoteConfig->numGroups == 0) {
-      persistence.get(id, loadedState);
-      state = cache.set(id, loadedState);
-    } else {
+    if (remoteConfig == NULL) {
       return NULL;
     }
+
+    if (id.groupId != 0 || remoteConfig->numGroups == 0) {
+      persistence.get(id, loadedState);
+    }
+
+    state = cache.set(id, loadedState);
   }
 
   return state;
@@ -47,6 +51,11 @@ GroupState* GroupStateStore::set(const BulbId &id, const GroupState& state) {
     const MiLightRemoteConfig* remote = MiLightRemoteConfig::fromType(id.deviceType);
     BulbId individualBulb(id);
 
+#ifdef STATE_DEBUG
+    Serial.printf_P(PSTR("Fanning out group 0 state for device ID 0x%04X (%d groups in total)\n"), id.deviceId, remote->numGroups);
+    state.debugState("group 0 state = ");
+#endif
+
     for (size_t i = 1; i <= remote->numGroups; i++) {
       individualBulb.groupId = i;
 

src/main.cpp

@@ -108,7 +108,9 @@ void onPacketSentHandler(uint8_t* packet, const MiLightRemoteConfig& config) {
 
   if (groupState != NULL) {
     groupState->patch(result);
-    stateStore->set(bulbId, *groupState);
+
+    // Copy state before setting it to avoid group 0 re-initialization clobbering it
+    stateStore->set(bulbId, GroupState(*groupState));
   }
 
   if (mqttClient) {

test/d1_mini/test.cpp

@@ -26,7 +26,7 @@ void run_packet_test(uint8_t* packet, PacketFormatter* packetFormatter, const Bu
   DynamicJsonBuffer jsonBuffer;
   JsonObject& result = jsonBuffer.createObject();
 
-  packetFormatter->prepare(0, 0, &stateStore, &settings);
+  packetFormatter->prepare(0, 0);
   BulbId bulbId = packetFormatter->parsePacket(packet, result);
 
   TEST_ASSERT_EQUAL_INT_MESSAGE(expectedBulbId.deviceId, bulbId.deviceId, "Should get the expected device ID");
@@ -234,24 +234,24 @@ void test_store() {
 
   GroupState* storedState;
 
-  storedState = &store.get(id2);
+  storedState = store.get(id2);
   TEST_ASSERT_TRUE_MESSAGE(*storedState == defaultState, "Should return default for state that hasn't been stored");
 
   store.set(id1, initState);
-  storedState = &store.get(id1);
+  storedState = store.get(id1);
 
   TEST_ASSERT_TRUE_MESSAGE(*storedState == initState, "Should return cached state");
 
   store.flush();
-  storedState = &store.get(id1);
+  storedState = store.get(id1);
   TEST_ASSERT_FALSE_MESSAGE(storedState->isDirty(), "Should not be dirty after flushing");
   TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(initState), "Should return cached state after flushing");
 
   store.set(id2, defaultState);
-  storedState = &store.get(id2);
+  storedState = store.get(id2);
   TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(defaultState), "Should return cached state");
 
-  storedState = &store.get(id1);
+  storedState = store.get(id1);
   TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(initState), "Should return persisted state");
 }
 
@@ -283,29 +283,51 @@ void test_group_0() {
   TEST_ASSERT_FALSE_MESSAGE(group0State.isEqualIgnoreDirty(initState), "group0 state should be different than initState");
   TEST_ASSERT_FALSE_MESSAGE(group0State.isEqualIgnoreDirty(initState2), "group0 state should be different than initState2");
 
-  storedState = store.get(id1);
+  storedState = *store.get(id1);
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(initState), "Should fetch persisted state");
 
-  storedState = store.get(id2);
+  storedState = *store.get(id2);
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(initState2), "Should fetch persisted state");
 
   store.set(group0Id, group0State);
 
-  storedState = store.get(id1);
+  storedState = *store.get(id1);
   expectedState = initState;
   expectedState.setHue(group0State.getHue());
 
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(expectedState), "Saving group 0 should only update changed field");
 
-  storedState = store.get(id2);
+  storedState = *store.get(id2);
   expectedState = initState2;
   expectedState.setHue(group0State.getHue());
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(expectedState), "Saving group 0 should only update changed field");
 
   // Test that state for group 0 is not persisted
-  storedState = store.get(group0Id);
+  storedState = *store.get(group0Id);
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Group 0 state should not be stored -- should return default state");
 
+  // Test that states for constituent groups are properly updated
+  initState.setHue(0);
+  initState2.setHue(100);
+  initState.setBrightness(50);
+  initState2.setBrightness(70);
+  store.set(id1, initState);
+  store.set(id2, initState2);
+
+  storedState = *store.get(group0Id);
+  storedState.setHue(200);
+  TEST_ASSERT_FALSE_MESSAGE(storedState.isSetBrightness(), "Should not have a set field for group 0 brightness");
+
+  store.set(group0Id, storedState);
+
+  storedState = *store.get(id1);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.getBrightness() == 50, "UNSET field in group 0 update SHOULD NOT overwrite constituent group field");
+  TEST_ASSERT_TRUE_MESSAGE(storedState.getHue() == 200, "SET field in group 0 update SHOULD overwrite constituent group field");
+
+  storedState = *store.get(id2);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.getBrightness() == 70, "UNSET field in group 0 update SHOULD NOT overwrite constituent group field");
+  TEST_ASSERT_TRUE_MESSAGE(storedState.getHue() == 200, "SET field in group 0 update SHOULD overwrite constituent group field");
+
   // Should persist group 0 for device types with 0 groups
   BulbId rgbId(1, 0, REMOTE_TYPE_RGB);
   GroupState rgbState = GroupState::defaultState(REMOTE_TYPE_RGB);
@@ -315,7 +337,7 @@ void test_group_0() {
   store.set(rgbId, rgbState);
   store.flush();
 
-  storedState = store.get(rgbId);
+  storedState = *store.get(rgbId);
 
   TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(rgbState), "Should persist group 0 for device type with no groups");
 }