Fix scratch state regression (#428)

* Add failing test
* Fix bug
* Rename field
* Remove trailing whitespace

lib/MiLightState/GroupState.cpp

@@ -14,6 +14,11 @@ static const GroupStateField ALL_PHYSICAL_FIELDS[] = {
   GroupStateField::STATE
 };
 
+static const GroupStateField ALL_SCRATCH_FIELDS[] = {
+  GroupStateField::BRIGHTNESS,
+  GroupStateField::KELVIN
+};
+
 // Number of units each increment command counts for
 static const uint8_t INCREMENT_COMMAND_VALUE = 10;
 
@@ -68,15 +73,6 @@ bool BulbId::operator==(const BulbId &other) {
     && deviceType == other.deviceType;
 }
 
-GroupState::GroupState() {
-  initFields();
-}
-
-GroupState::GroupState(const JsonObject& jsonState) {
-  initFields();
-  patch(jsonState);
-}
-
 void GroupState::initFields() {
   state.fields._state                = 0;
   state.fields._brightness           = 0;
@@ -112,11 +108,31 @@ GroupState& GroupState::operator=(const GroupState& other) {
   scratchpad.rawData = other.scratchpad.rawData;
 }
 
-GroupState::GroupState(const GroupState& other) {
+GroupState::GroupState()
+  : previousState(NULL)
+{
+  initFields();
+}
+
+GroupState::GroupState(const GroupState& other)
+  : previousState(NULL)
+{
   memcpy(state.rawData, other.state.rawData, DATA_LONGS * sizeof(uint32_t));
   scratchpad.rawData = other.scratchpad.rawData;
 }
 
+GroupState::GroupState(const GroupState* previousState, const JsonObject& jsonState)
+  : previousState(previousState)
+{
+  initFields();
+
+  if (previousState != NULL) {
+    this->scratchpad = previousState->scratchpad;
+  }
+
+  patch(jsonState);
+}
+
 bool GroupState::operator==(const GroupState& other) const {
   return memcmp(state.rawData, other.state.rawData, DATA_LONGS * sizeof(uint32_t)) == 0;
 }
@@ -591,16 +607,17 @@ bool GroupState::applyIncrementCommand(GroupStateField field, IncrementDirection
   int8_t dirValue = static_cast<int8_t>(dir);
 
   // If there's already a known value, update it
-  if (isSetField(field)) {
-    int8_t currentValue = static_cast<int8_t>(getFieldValue(field));
+  if (previousState != NULL && previousState->isSetField(field)) {
+    int8_t currentValue = static_cast<int8_t>(previousState->getFieldValue(field));
     int8_t newValue = currentValue + (dirValue * INCREMENT_COMMAND_VALUE);
 
 #ifdef STATE_DEBUG
-    debugState("Updating field from increment command");
+    previousState->debugState("Updating field from increment command");
 #endif
 
     // For now, assume range for both brightness and kelvin is [0, 100]
     setFieldValue(field, constrain(newValue, 0, 100));
+
     return true;
   // Otherwise start or update scratch state
   } else {
@@ -668,6 +685,14 @@ bool GroupState::patch(const GroupState& other) {
       setFieldValue(field, other.getFieldValue(field));
     }
   }
+
+  for (size_t i = 0; i < size(ALL_SCRATCH_FIELDS); ++i) {
+    GroupStateField field = ALL_SCRATCH_FIELDS[i];
+
+    if (other.isSetScratchField(field)) {
+      setScratchFieldValue(field, other.getScratchFieldValue(field));
+    }
+  }
 }
 
 /*
@@ -728,8 +753,10 @@ bool GroupState::patch(const JsonObject& state) {
       changes |= applyIncrementCommand(GroupStateField::BRIGHTNESS, IncrementDirection::DECREASE);
     } else if (isOn() && command == "temperature_up") {
       changes |= applyIncrementCommand(GroupStateField::KELVIN, IncrementDirection::INCREASE);
+      changes |= setBulbMode(BULB_MODE_WHITE);
     } else if (isOn() && command == "temperature_down") {
       changes |= applyIncrementCommand(GroupStateField::KELVIN, IncrementDirection::DECREASE);
+      changes |= setBulbMode(BULB_MODE_WHITE);
     }
   }
 

lib/MiLightState/GroupState.h

@@ -49,8 +49,9 @@ public:
   GroupState(const GroupState& other);
   GroupState& operator=(const GroupState& other);
 
-  // Convenience constructor that patches defaults with JSON state
-  GroupState(const JsonObject& jsonState);
+  // Convenience constructor that patches transient state from a previous GroupState,
+  // and defaults with JSON state
+  GroupState(const GroupState* previousState, const JsonObject& jsonState);
 
   void initFields();
 
@@ -211,6 +212,12 @@ private:
   StateData state;
   TransientData scratchpad;
 
+  // State is constructed from individual command packets.  A command packet is parsed in
+  // isolation, and the result is patched onto previous state.  There are a few cases where
+  // it's necessary to know some things from the previous state, so we keep a reference to
+  // it here.
+  const GroupState* previousState;
+
   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"}

lib/MiLightState/GroupStateStore.cpp

@@ -11,6 +11,9 @@ GroupState* GroupStateStore::get(const BulbId& id) {
   GroupState* state = cache.get(id);
 
   if (state == NULL) {
+#if STATE_DEBUG
+    Serial.println(F("Couldn't fetch state from cache, getting it from persistence"));
+#endif
     trackEviction();
     GroupState loadedState = GroupState::defaultState(id.deviceType);
 

src/main.cpp

@@ -109,7 +109,9 @@ void onPacketSentHandler(uint8_t* packet, const MiLightRemoteConfig& config) {
 
   // update state to reflect changes from this packet
   GroupState* groupState = stateStore->get(bulbId);
-  const GroupState stateUpdates(result);
+
+  // pass in previous scratch state as well
+  const GroupState stateUpdates(groupState, result);
 
   if (groupState != NULL) {
     groupState->patch(stateUpdates);

test/remote/spec/state_spec.rb

@@ -250,4 +250,74 @@ RSpec.describe 'State' do
       expect(state.select { |x| desired_state.include?(x) } ).to eq(desired_state)
     end
   end
+
+  context 'increment/decrement commands' do
+    it 'should assume state after sufficiently many down commands' do
+      id = @id_params.merge(type: 'cct')
+      @client.delete_state(id)
+
+      @client.patch_state({status: 'on'}, id)
+
+      expect(@client.get_state(id)).to_not include('brightness', 'kelvin')
+
+      10.times do
+        @client.patch_state(
+          { commands: ['level_down', 'temperature_down'] },
+          id
+        )
+      end
+
+      state = @client.get_state(id)
+      expect(state).to          include('level', 'kelvin')
+      expect(state['level']).to eq(0)
+      expect(state['kelvin']).to eq(0)
+    end
+
+    it 'should assume state after sufficiently many up commands' do
+      id = @id_params.merge(type: 'cct')
+      @client.delete_state(id)
+
+      @client.patch_state({status: 'on'}, id)
+
+      expect(@client.get_state(id)).to_not include('level', 'kelvin')
+
+      10.times do
+        @client.patch_state(
+          { commands: ['level_up', 'temperature_up'] },
+          id
+        )
+      end
+
+      state = @client.get_state(id)
+      expect(state).to          include('level', 'kelvin')
+      expect(state['level']).to eq(100)
+      expect(state['kelvin']).to eq(100)
+    end
+
+    it 'should affect known state' do
+      id = @id_params.merge(type: 'cct')
+      @client.delete_state(id)
+
+      @client.patch_state({status: 'on'}, id)
+
+      expect(@client.get_state(id)).to_not include('level', 'kelvin')
+
+      10.times do
+        @client.patch_state(
+          { commands: ['level_up', 'temperature_up'] },
+          id
+        )
+      end
+
+      @client.patch_state(
+        { commands: ['level_down', 'temperature_down'] },
+        id
+      )
+
+      state = @client.get_state(id)
+      expect(state).to           include('level', 'kelvin')
+      expect(state['level']).to  eq(90)
+      expect(state['kelvin']).to eq(90)
+    end
+  end
 end