Apply patched updates to individual groups when updating group 0 (fixes #284) (#287)

* Set up unit tests

* Add method to patch state with set fields from another state

* add methods to help with test

* Patch member state with virtual "all groups" state instead of clobbering it

* Don't persist group 0 state, except for devices that don't have groups

* Stateful brightness for RGB

lib/MiLight/RgbPacketFormatter.cpp

@@ -47,11 +47,15 @@ void RgbPacketFormatter::updateColorRaw(uint8_t value) {
 }
 
 void RgbPacketFormatter::updateBrightness(uint8_t value) {
+  const GroupState& state = this->stateStore->get(deviceId, groupId, MiLightRemoteType::REMOTE_TYPE_RGB);
+  int8_t knownValue = state.isSetBrightness() ? state.getBrightness() : -1;
+
   valueByStepFunction(
     &PacketFormatter::increaseBrightness,
     &PacketFormatter::decreaseBrightness,
     RGB_INTERVALS,
-    value / RGB_INTERVALS
+    value / RGB_INTERVALS,
+    knownValue / RGB_INTERVALS
   );
 }
 
@@ -104,6 +108,10 @@ BulbId RgbPacketFormatter::parsePacket(const uint8_t* packet, JsonObject& result
     result["command"] = "mode_speed_down";
   } else if (command == RGB_SPEED_UP) {
     result["command"] = "mode_speed_up";
+  } else if (command == RGB_BRIGHTNESS_DOWN) {
+    result["command"] = "brightness_down";
+  } else if (command == RGB_BRIGHTNESS_UP) {
+    result["command"] = "brightness_up";
   } else {
     result["button_id"] = command;
   }

lib/MiLightState/GroupState.cpp

@@ -4,6 +4,15 @@
 #include <RGBConverter.h>
 
 const BulbId DEFAULT_BULB_ID;
+static const GroupStateField ALL_PHYSICAL_FIELDS[] = {
+  GroupStateField::BRIGHTNESS,
+  GroupStateField::BULB_MODE,
+  GroupStateField::HUE,
+  GroupStateField::KELVIN,
+  GroupStateField::MODE,
+  GroupStateField::SATURATION,
+  GroupStateField::STATE
+};
 
 // Number of units each increment command counts for
 static const uint8_t INCREMENT_COMMAND_VALUE = 10;
@@ -89,6 +98,36 @@ GroupState::GroupState() {
   scratchpad.fields._kelvinScratch          = 0;
 }
 
+GroupState& GroupState::operator=(const GroupState& other) {
+  memcpy(state.rawData, other.state.rawData, DATA_LONGS * sizeof(uint32_t));
+  scratchpad.rawData = other.scratchpad.rawData;
+}
+
+GroupState::GroupState(const GroupState& other) {
+  memcpy(state.rawData, other.state.rawData, DATA_LONGS * sizeof(uint32_t));
+  scratchpad.rawData = other.scratchpad.rawData;
+}
+
+bool GroupState::operator==(const GroupState& other) const {
+  return memcmp(state.rawData, other.state.rawData, DATA_LONGS * sizeof(uint32_t)) == 0;
+}
+
+bool GroupState::isEqualIgnoreDirty(const GroupState& other) const {
+  GroupState meCopy = *this;
+  GroupState otherCopy = other;
+
+  meCopy.clearDirty();
+  meCopy.clearMqttDirty();
+  otherCopy.clearDirty();
+  otherCopy.clearMqttDirty();
+
+  return meCopy == otherCopy;
+}
+
+void GroupState::print(Stream& stream) const {
+  stream.printf("State: %08X %08X\n", state.rawData[0], state.rawData[1]);
+}
+
 bool GroupState::isSetField(GroupStateField field) const {
   switch (field) {
     case GroupStateField::COMPUTED_COLOR:
@@ -492,6 +531,16 @@ bool GroupState::applyIncrementCommand(GroupStateField field, IncrementDirection
   return false;
 }
 
+bool GroupState::patch(const GroupState& other) {
+  for (size_t i = 0; i < size(ALL_PHYSICAL_FIELDS); ++i) {
+    GroupStateField field = ALL_PHYSICAL_FIELDS[i];
+
+    if (other.isSetField(field)) {
+      setFieldValue(field, other.getFieldValue(field));
+    }
+  }
+}
+
 /*
   Update group state to reflect a packet state
 

lib/MiLightState/GroupState.h

@@ -46,6 +46,13 @@ class GroupState {
 public:
 
   GroupState();
+  GroupState(const GroupState& other);
+  GroupState& operator=(const GroupState& other);
+
+  bool operator==(const GroupState& other) const;
+  bool isEqualIgnoreDirty(const GroupState& other) const;
+  void print(Stream& stream) const;
+
 
   bool isSetField(GroupStateField field) const;
   uint16_t getFieldValue(GroupStateField field) const;
@@ -108,6 +115,12 @@ public:
   inline bool setMqttDirty();
   bool clearMqttDirty();
 
+  // Patches this state with ONLY the set fields in the other. Returns 
+  // true if there were any changes.
+  bool patch(const GroupState& other);
+
+  // Patches this state with the fields defined in the JSON state.  Returns 
+  // true if there were any changes.
   bool patch(const JsonObject& state);
 
   // It's a little weird to need to pass in a BulbId here.  The purpose is to
@@ -141,7 +154,7 @@ public:
   static const GroupState& defaultState(MiLightRemoteType remoteType);
 
 private:
-  static const size_t DATA_LONGS = 3;
+  static const size_t DATA_LONGS = 2;
   union StateData {
     uint32_t rawData[DATA_LONGS];
     struct Fields {

lib/MiLightState/GroupStateStore.cpp

@@ -13,9 +13,20 @@ GroupState& GroupStateStore::get(const BulbId& id) {
   if (state == NULL) {
     trackEviction();
     GroupState loadedState = GroupState::defaultState(id.deviceType);
-    persistence.get(id, loadedState);
 
-    state = cache.set(id, loadedState);
+    // For device types with groups, group 0 is a "virtual" group.  All devices paired with the same ID will respond
+    // to group 0.  So it doesn't make sense to store group 0 state by itself.
+    //
+    // For devices that don't have groups, we made the unfortunate decision to represent state using the fake group
+    // 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 {
+      state = &loadedState;
+    }
   }
 
   return *state;
@@ -26,7 +37,7 @@ GroupState& GroupStateStore::get(const uint16_t deviceId, const uint8_t groupId,
   return get(bulbId);
 }
 
-// save state for a bulb.  If id.groupId == 0, will iternate across all groups
+// save state for a bulb.  If id.groupId == 0, will iterate across all groups
 // and individually save each group (recursively)
 GroupState& GroupStateStore::set(const BulbId &id, const GroupState& state) {
   GroupState& storedState = get(id);
@@ -38,7 +49,9 @@ GroupState& GroupStateStore::set(const BulbId &id, const GroupState& state) {
 
     for (size_t i = 1; i <= remote->numGroups; i++) {
       individualBulb.groupId = i;
-      set(individualBulb, state);
+
+      GroupState& individualState = get(individualBulb);
+      individualState.patch(state);
     }
   }
   

src/main.cpp

@@ -1,3 +1,5 @@
+#ifndef UNIT_TEST
+
 #include <SPI.h>
 #include <WiFiManager.h>
 #include <ArduinoJson.h>
@@ -344,3 +346,5 @@ void loop() {
     ESP.restart();
   }
 }
+
+#endif

test/d1_mini/test_state.cpp

@@ -0,0 +1,252 @@
+// #if defined(ARDUINO) && defined(UNIT_TEST)
+
+#include <FS.h>
+#include <Arduino.h>
+#include "unity.h"
+
+#include <GroupState.h>
+#include <GroupStateStore.h>
+#include <GroupStateCache.h>
+#include <GroupStatePersistence.h>
+
+GroupState color() {
+  GroupState s;
+
+  s.setState(MiLightStatus::ON);
+  s.setBulbMode(BulbMode::BULB_MODE_COLOR);
+  s.setBrightness(100);
+  s.setHue(1);
+  s.setSaturation(10);
+
+  return s;
+}
+
+void test_init_state() {
+  GroupState s;
+
+  TEST_ASSERT_EQUAL(s.getBulbMode(), BulbMode::BULB_MODE_WHITE);
+  TEST_ASSERT_EQUAL(s.isSetBrightness(), false);
+}
+
+void test_state_updates() {
+  GroupState s = color();
+
+  // Make sure values are packed and unpacked correctly
+  TEST_ASSERT_EQUAL(s.getBulbMode(), BulbMode::BULB_MODE_COLOR);
+  TEST_ASSERT_EQUAL(s.getBrightness(), 100);
+  TEST_ASSERT_EQUAL(s.getHue(), 1);
+  TEST_ASSERT_EQUAL(s.getSaturation(), 10);
+
+  // Make sure brightnesses are tied to mode
+  s.setBulbMode(BulbMode::BULB_MODE_WHITE);
+  s.setBrightness(0);
+
+  TEST_ASSERT_EQUAL(s.getBulbMode(), BulbMode::BULB_MODE_WHITE);
+  TEST_ASSERT_EQUAL(s.getBrightness(), 0);
+
+  s.setBulbMode(BulbMode::BULB_MODE_COLOR);
+
+  TEST_ASSERT_EQUAL(s.getBrightness(), 100);
+}
+
+void test_cache() {
+  BulbId id1(1, 1, REMOTE_TYPE_FUT089);
+  BulbId id2(1, 2, REMOTE_TYPE_FUT089);
+
+  GroupState s = color();
+  s.clearDirty();
+  s.clearMqttDirty();
+
+  GroupStateCache cache(1);
+  GroupState* storedState = cache.get(id2);
+
+  TEST_ASSERT_NULL_MESSAGE(storedState, "Should not retrieve value which hasn't been stored");
+
+  cache.set(id1, s);
+  storedState = cache.get(id1);
+
+  TEST_ASSERT_NOT_NULL_MESSAGE(storedState, "Should retrieve a value");
+  TEST_ASSERT_TRUE_MESSAGE(s == *storedState, "State should be the same when retrieved");
+
+  cache.set(id2, s);
+  storedState = cache.get(id2);
+
+  TEST_ASSERT_NOT_NULL_MESSAGE(storedState, "Should retrieve a value");
+  TEST_ASSERT_TRUE_MESSAGE(s == *storedState, "State should be the same when retrieved");
+
+  storedState = cache.get(id1);
+
+  TEST_ASSERT_NULL_MESSAGE(storedState, "Should evict old entry from cache");
+}
+
+void test_persistence() {
+  BulbId id1(1, 1, REMOTE_TYPE_FUT089);
+  BulbId id2(1, 2, REMOTE_TYPE_FUT089);
+
+  GroupStatePersistence persistence;
+
+  persistence.clear(id1);
+  persistence.clear(id2);
+
+  GroupState storedState;
+  GroupState s = color();
+  s.clearDirty();
+  s.clearMqttDirty();
+
+  GroupState defaultState = GroupState::defaultState(REMOTE_TYPE_FUT089);
+
+  persistence.get(id1, storedState);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Should start with clean state");
+  persistence.get(id2, storedState);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Should start with clean state");
+
+  persistence.set(id1, s);
+
+  persistence.get(id2, storedState);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Should return default for state that hasn't been stored");
+
+  persistence.get(id1, storedState);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(s), "Should retrieve state from flash without modification");
+
+  GroupState newState = s;
+  newState.setBulbMode(BulbMode::BULB_MODE_WHITE);
+  newState.setBrightness(255);
+  persistence.set(id2, newState);
+
+  persistence.get(id1, storedState);
+
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(s), "Should retrieve unmodified state");
+
+  persistence.get(id2, storedState);
+
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(newState), "Should retrieve modified state");
+}
+
+void test_store() {
+  BulbId id1(1, 1, REMOTE_TYPE_FUT089);
+  BulbId id2(1, 2, REMOTE_TYPE_FUT089);
+
+  // cache 1 item, flush immediately
+  GroupStateStore store(1, 0);
+  GroupStatePersistence persistence;
+
+  persistence.clear(id1);
+  persistence.clear(id2);
+
+  GroupState initState = color();
+  GroupState initState2 = color();
+  GroupState defaultState = GroupState::defaultState(REMOTE_TYPE_FUT089);
+  initState2.setBrightness(255);
+
+  GroupState* storedState;
+
+  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);
+
+  TEST_ASSERT_TRUE_MESSAGE(*storedState == initState, "Should return cached state");
+
+  store.flush();
+  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);
+  TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(defaultState), "Should return cached state");
+
+  storedState = &store.get(id1);
+  TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(initState), "Should return persisted state");
+}
+
+void test_group_0() {
+  BulbId group0Id(1, 0, REMOTE_TYPE_FUT089);
+  BulbId id1(1, 1, REMOTE_TYPE_FUT089);
+  BulbId id2(1, 2, REMOTE_TYPE_FUT089);
+
+  // cache 1 item, flush immediately
+  GroupStateStore store(10, 0);
+  GroupStatePersistence persistence;
+
+  persistence.clear(id1);
+  persistence.clear(id2);
+
+  GroupState initState = color();
+  GroupState initState2 = color();
+  GroupState defaultState = GroupState::defaultState(REMOTE_TYPE_FUT089);
+  GroupState storedState;
+  GroupState expectedState;
+  GroupState group0State;
+
+  initState2.setBrightness(255);
+  group0State.setHue(100);
+
+  store.set(id1, initState);
+  store.set(id2, initState2);
+
+  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);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(initState), "Should fetch persisted state");
+
+  storedState = store.get(id2);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(initState2), "Should fetch persisted state");
+
+  store.set(group0Id, group0State);
+
+  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);
+  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);
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Group 0 state should not be stored -- should return default state");
+
+  // Should persist group 0 for device types with 0 groups
+  BulbId rgbId(1, 0, REMOTE_TYPE_RGB);
+  GroupState rgbState = GroupState::defaultState(REMOTE_TYPE_RGB);
+  rgbState.setHue(100);
+  rgbState.setBrightness(100);
+
+  store.set(rgbId, rgbState);
+  store.flush();
+
+  storedState = store.get(rgbId);
+
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(rgbState), "Should persist group 0 for device type with no groups");
+}
+
+// setup connects serial, runs test cases (upcoming)
+void setup() {
+  delay(2000);
+  SPIFFS.begin();
+  Serial.begin(9600);
+
+  UNITY_BEGIN();
+
+  RUN_TEST(test_init_state);
+  RUN_TEST(test_state_updates);
+  RUN_TEST(test_cache);
+  RUN_TEST(test_persistence);
+  RUN_TEST(test_store);
+  RUN_TEST(test_group_0);
+
+  UNITY_END();
+}
+
+void loop() {
+  // nothing to be done here.
+}
+
+
+// #endif