Fix bug in patch state (#441)

* Fix embedded tests
* Improve state debugging messages
* Ignore `test/remote` when running embedded tests

lib/MiLightState/GroupState.cpp

@@ -12,13 +12,13 @@ static const char* BULB_MODE_NAMES[] = {
 
 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
+  GroupStateField::STATE,
+  GroupStateField::BRIGHTNESS
 };
 
 static const GroupStateField ALL_SCRATCH_FIELDS[] = {
@@ -946,7 +946,7 @@ void GroupState::debugState(char const *debugMessage) const {
 
   // define fields to show (if count changes, make sure to update count to applyState below)
   GroupStateField fields[] {
-      GroupStateField::BRIGHTNESS,
+      GroupStateField::LEVEL,
       GroupStateField::BULB_MODE,
       GroupStateField::COLOR_TEMP,
       GroupStateField::EFFECT,
@@ -970,6 +970,7 @@ void GroupState::debugState(char const *debugMessage) const {
   Serial.printf("%s: ", debugMessage);
   jsonState.printTo(Serial);
   Serial.println("");
+  Serial.printf("Raw data: %08X %08X\n", state.rawData[0], state.rawData[1]);
 #endif
 }
 

lib/MiLightState/GroupStateStore.cpp

@@ -12,7 +12,12 @@ GroupState* GroupStateStore::get(const BulbId& id) {
 
   if (state == NULL) {
 #if STATE_DEBUG
-    Serial.println(F("Couldn't fetch state from cache, getting it from persistence"));
+    printf(
+      "Couldn't fetch state for 0x%04X / %d / %s in the cache, getting it from persistence\n",
+      id.deviceId,
+      id.groupId,
+      MiLightRemoteConfig::fromType(id.deviceType)->name.c_str()
+    );
 #endif
     trackEviction();
     GroupState loadedState = GroupState::defaultState(id.deviceType);
@@ -39,8 +44,8 @@ GroupState* GroupStateStore::get(const uint16_t deviceId, const uint8_t groupId,
 //
 // Notes:
 //
-// * For device types with groups, group 0 is a "virtual" group.  All devices paired with the same ID will 
-//   respond to group 0.  When state for an individual (i.e., != 0) group is changed, the state for 
+// * For device types with groups, group 0 is a "virtual" group.  All devices paired with the same ID will
+//   respond to group 0.  When state for an individual (i.e., != 0) group is changed, the state for
 //   group 0 becomes out of sync and should be cleared.
 //
 // * If id.groupId == 0, will iterate across all groups and individually save each group (recursively)
@@ -70,7 +75,7 @@ GroupState* GroupStateStore::set(const BulbId &id, const GroupState& state) {
 
     group0State->clearNonMatchingFields(state);
   }
-  
+
   return storedState;
 }
 
@@ -90,6 +95,16 @@ void GroupStateStore::clear(const BulbId& bulbId) {
 void GroupStateStore::trackEviction() {
   if (cache.isFull()) {
     evictedIds.add(cache.getLru());
+
+#ifdef STATE_DEBUG
+    BulbId bulbId = evictedIds.getLast();
+    printf(
+      "Evicting from cache: 0x%04X / %d / %s\n",
+      bulbId.deviceId,
+      bulbId.groupId,
+      MiLightRemoteConfig::fromType(bulbId.deviceType)->name.c_str()
+    );
+#endif
   }
 }
 

platformio.ini

@@ -24,6 +24,7 @@ lib_deps_external =
   CircularBuffer@~1.2.0
 extra_scripts =
   pre:.build_web.py
+test_ignore = remote
 build_flags = !python .get_version.py -DMQTT_MAX_PACKET_SIZE=250 -DHTTP_UPLOAD_BUFLEN=128 -D FIRMWARE_NAME=milight-hub -Idist -Ilib/DataStructures
 # -D STATE_DEBUG
 # -D DEBUG_PRINTF
@@ -41,6 +42,7 @@ extra_scripts = ${common.extra_scripts}
 lib_deps =
   ${common.lib_deps_builtin}
   ${common.lib_deps_external}
+test_ignore = ${common.test_ignore}
 
 [env:d1_mini]
 platform = ${common.platform}
@@ -51,6 +53,7 @@ extra_scripts = ${common.extra_scripts}
 lib_deps =
   ${common.lib_deps_builtin}
   ${common.lib_deps_external}
+test_ignore = ${common.test_ignore}
 
 [env:esp12]
 platform = ${common.platform}
@@ -61,6 +64,7 @@ extra_scripts = ${common.extra_scripts}
 lib_deps =
   ${common.lib_deps_builtin}
   ${common.lib_deps_external}
+test_ignore = ${common.test_ignore}
 
 [env:esp07]
 platform = ${common.platform}
@@ -71,6 +75,7 @@ extra_scripts = ${common.extra_scripts}
 lib_deps =
   ${common.lib_deps_builtin}
   ${common.lib_deps_external}
+test_ignore = ${common.test_ignore}
 
 [env:huzzah]
 platform = ${common.platform}
@@ -81,3 +86,4 @@ extra_scripts = ${common.extra_scripts}
 lib_deps =
   ${common.lib_deps_builtin}
   ${common.lib_deps_external}
+test_ignore = ${common.test_ignore}

test/d1_mini/test.cpp

@@ -42,28 +42,28 @@ void test_fut092_packet_formatter() {
 
   uint8_t onPacket[] = {0x00, 0xDB, 0xE1, 0x24, 0x66, 0xCA, 0x54, 0x66, 0xD2};
   run_packet_test(
-    onPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_RGB_CCT), 
-    "state", 
+    onPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_RGB_CCT),
+    "state",
     "OFF"
   );
 
   uint8_t minColorTempPacket[] = {0x00, 0xDB, 0xE1, 0x24, 0x64, 0x3C, 0x47, 0x66, 0x31};
   run_packet_test(
-    minColorTempPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_RGB_CCT), 
-    "color_temp", 
+    minColorTempPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_RGB_CCT),
+    "color_temp",
     COLOR_TEMP_MIN_MIREDS
   );
 
   uint8_t maxColorTempPacket[] = {0x00, 0xDB, 0xE1, 0x24, 0x64, 0x94, 0x62, 0x66, 0x88};
   run_packet_test(
-    maxColorTempPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_RGB_CCT), 
-    "color_temp", 
+    maxColorTempPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_RGB_CCT),
+    "color_temp",
     COLOR_TEMP_MAX_MIREDS
   );
 }
@@ -73,28 +73,28 @@ void test_fut091_packet_formatter() {
 
   uint8_t onPacket[] = {0x00, 0xDC, 0xE1, 0x24, 0x66, 0xCA, 0xBA, 0x66, 0xB5};
   run_packet_test(
-    onPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_FUT091), 
-    "state", 
+    onPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_FUT091),
+    "state",
     "OFF"
   );
 
   uint8_t minColorTempPacket[] = {0x00, 0xDC, 0xE1, 0x24, 0x64, 0x8D, 0xB9, 0x66, 0x71};
   run_packet_test(
-    minColorTempPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_FUT091), 
-    "color_temp", 
+    minColorTempPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_FUT091),
+    "color_temp",
     COLOR_TEMP_MIN_MIREDS
   );
 
   uint8_t maxColorTempPacket[] = {0x00, 0xDC, 0xE1, 0x24, 0x64, 0x55, 0xB7, 0x66, 0x27};
   run_packet_test(
-    maxColorTempPacket, 
-    &packetFormatter, 
-    BulbId(1, 1, REMOTE_TYPE_FUT091), 
-    "color_temp", 
+    maxColorTempPacket,
+    &packetFormatter,
+    BulbId(1, 1, REMOTE_TYPE_FUT091),
+    "color_temp",
     COLOR_TEMP_MAX_MIREDS
   );
 }
@@ -108,9 +108,9 @@ GroupState color() {
 
   s.setState(MiLightStatus::ON);
   s.setBulbMode(BulbMode::BULB_MODE_COLOR);
-  s.setBrightness(100);
   s.setHue(1);
   s.setSaturation(10);
+  s.setBrightness(100);
 
   return s;
 }
@@ -220,8 +220,7 @@ 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);
+  GroupStateStore store(4, 0);
   GroupStatePersistence persistence;
 
   persistence.clear(id1);
@@ -230,7 +229,7 @@ void test_store() {
   GroupState initState = color();
   GroupState initState2 = color();
   GroupState defaultState = GroupState::defaultState(REMOTE_TYPE_FUT089);
-  initState2.setBrightness(255);
+  initState2.setBrightness(50);
 
   GroupState* storedState;
 
@@ -240,7 +239,7 @@ void test_store() {
   store.set(id1, initState);
   storedState = store.get(id1);
 
-  TEST_ASSERT_TRUE_MESSAGE(*storedState == initState, "Should return cached state");
+  TEST_ASSERT_TRUE_MESSAGE(storedState->isEqualIgnoreDirty(initState), "Should return stored state.  Will not be cached because of internal group 0 lookups");
 
   store.flush();
   storedState = store.get(id1);
@@ -269,7 +268,6 @@ void test_group_0() {
 
   GroupState initState = color();
   GroupState initState2 = color();
-  GroupState defaultState = GroupState::defaultState(REMOTE_TYPE_FUT089);
   GroupState storedState;
   GroupState expectedState;
   GroupState group0State;
@@ -302,9 +300,9 @@ void test_group_0() {
   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
+  // Test that state for group 0 is persisted
   storedState = *store.get(group0Id);
-  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(defaultState), "Group 0 state should not be stored -- should return default state");
+  TEST_ASSERT_TRUE_MESSAGE(storedState.isEqualIgnoreDirty(group0State), "Group 0 state should not be stored -- should return default state");
 
   // Test that states for constituent groups are properly updated
   initState.setHue(0);

test/remote/spec/state_spec.rb

@@ -284,6 +284,31 @@ RSpec.describe 'State' do
       expect(state.keys).to include(*desired_state.keys)
       expect(state.select { |x| desired_state.include?(x) } ).to eq(desired_state)
     end
+
+    it 'should support separate brightness fields for different modes' do
+      desired_state = {
+        'hue' => 0,
+        'level' => 50
+      }
+
+      @client.patch_state(desired_state, @id_params)
+      result = @client.get_state(@id_params)
+      expect(result['bulb_mode']).to eq('color')
+      expect(result['level']).to eq(50)
+
+
+      @client.patch_state({'kelvin' => 100}, @id_params)
+      @client.patch_state({'level' => 70}, @id_params)
+      result = @client.get_state(@id_params)
+      expect(result['bulb_mode']).to eq('white')
+      expect(result['level']).to eq(70)
+
+      @client.patch_state({'hue' => 0}, @id_params)
+      result = @client.get_state(@id_params)
+      expect(result['bulb_mode']).to eq('color')
+      # Should retain previous brightness
+      expect(result['level']).to eq(50)
+    end
   end
 
   context 'increment/decrement commands' do