Change status: ON transition behavior: will always transition from 0 …

…brightness, rather than last known brightness.

lib/MiLight/MiLightClient.cpp

@@ -10,6 +10,8 @@
 
 using namespace std::placeholders;
 
+static const uint8_t STATUS_UNDEFINED = 255;
+
 const char* MiLightClient::FIELD_ORDERINGS[] = {
   // These are handled manually
   // GroupStateFieldNames::STATE,
@@ -318,16 +320,23 @@ void MiLightClient::update(JsonObject request) {
 
   // Always turn on first
   if (parsedStatus == ON) {
-    if (
-         transition == 0
-      // Do not generate a transition if a brightness field is also set, since that will also
-      // generate a transition.
-      || request.containsKey(GroupStateFieldNames::BRIGHTNESS)
-      || request.containsKey(GroupStateFieldNames::LEVEL)
-    ) {
+    if (transition == 0) {
       this->updateStatus(ON);
     } else {
-      handleTransition(GroupStateField::STATUS, status, transition);
+      JsonVariant brightness = request[GroupStateFieldNames::BRIGHTNESS];
+      JsonVariant level = request[GroupStateFieldNames::LEVEL];
+
+      // The behavior for status transitions is to ramp up to max or down to min brightness.  If a
+      // brightness is specified, we shold ramp up or down to that value instead.
+      if (!brightness.isUndefined()) {
+        this->updateStatus(ON);
+        handleTransition(GroupStateField::BRIGHTNESS, brightness, transition, 0);
+      } else if (!level.isUndefined()) {
+        this->updateStatus(ON);
+        handleTransition(GroupStateField::LEVEL, level, transition, 0);
+      } else {
+        handleTransition(GroupStateField::STATUS, status, transition, 0);
+      }
     }
   }
 
@@ -337,14 +346,20 @@ void MiLightClient::update(JsonObject request) {
       JsonVariant value = request[fieldName];
 
       if (handler != FIELD_SETTERS.end()) {
-        if (transition != 0) {
-          handleTransition(
-            GroupStateFieldHelpers::getFieldByName(fieldName),
-            value,
-            transition
-          );
-        } else {
+        // No transition -- set field directly
+        if (transition == 0) {
           handler->second(this, value);
+        } else {
+          // Do not generate a brightness transition if a status field was specified.  Status will
+          // generate its own brightness transition, and generating another one will cause conflicts.
+          GroupStateField field = GroupStateFieldHelpers::getFieldByName(fieldName);
+
+          if (    !GroupStateFieldHelpers::isBrightnessField(field) // If field isn't brightness
+               || parsedStatus == STATUS_UNDEFINED                  // or if there was not a status field
+                                                                    // in the command
+          ) {
+            handleTransition(field, value, transition);
+          }
         }
       }
     }
@@ -421,7 +436,7 @@ void MiLightClient::handleCommand(JsonVariant command) {
   }
 }
 
-void MiLightClient::handleTransition(GroupStateField field, JsonVariant value, float duration) {
+void MiLightClient::handleTransition(GroupStateField field, JsonVariant value, float duration, int16_t startValue) {
   BulbId bulbId = currentRemote->packetFormatter->currentBulbId();
   GroupState* currentState = stateStore->get(bulbId);
   std::shared_ptr<Transition::Builder> transitionBuilder = nullptr;
@@ -446,22 +461,28 @@ void MiLightClient::handleTransition(GroupStateField field, JsonVariant value, f
       endColor
     );
   } else if (field == GroupStateField::STATUS || field == GroupStateField::STATE) {
-    uint8_t startLevel = 0;
+    uint8_t startLevel;
     MiLightStatus status = parseMilightStatus(value);
 
-    if (currentState->isSetBrightness()) {
+    if (startValue == FETCH_VALUE_FROM_STATE) {
       startLevel = currentState->getBrightness();
     } else if (status == ON) {
       startLevel = 0;
     } else {
       startLevel = 100;
     }
 
-    transitionBuilder = transitions.buildStatusTransition(bulbId, parseMilightStatus(value), startLevel);
+    transitionBuilder = transitions.buildStatusTransition(bulbId, status, startLevel);
   } else {
-    uint16_t currentValue = currentState->getParsedFieldValue(field);
+    uint16_t currentValue;
     uint16_t endValue = value;
 
+    if (startValue == FETCH_VALUE_FROM_STATE) {
+      currentValue = currentState->getParsedFieldValue(field);
+    } else {
+      currentValue = startValue;
+    }
+
     transitionBuilder = transitions.buildFieldTransition(
       bulbId,
       field,
@@ -605,7 +626,7 @@ JsonVariant MiLightClient::extractStatus(JsonObject object) {
 
 uint8_t MiLightClient::parseStatus(JsonVariant val) {
   if (val.isUndefined()) {
-    return 255;
+    return STATUS_UNDEFINED;
   }
 
   return parseMilightStatus(val);

lib/MiLight/MiLightClient.h

@@ -37,6 +37,9 @@ namespace TransitionParams {
 
 class MiLightClient {
 public:
+  // Used to indicate that the start value for a transition should be fetched from current state
+  static const int16_t FETCH_VALUE_FROM_STATE = -1;
+
   MiLightClient(
     RadioSwitchboard& radioSwitchboard,
     PacketSender& packetSender,
@@ -93,7 +96,7 @@ class MiLightClient {
   void handleCommand(JsonVariant command);
   void handleCommands(JsonArray commands);
   bool handleTransition(JsonObject args, JsonDocument& responseObj);
-  void handleTransition(GroupStateField field, JsonVariant value, float duration);
+  void handleTransition(GroupStateField field, JsonVariant value, float duration, int16_t startValue = FETCH_VALUE_FROM_STATE);
   void handleEffect(const String& effect);
 
   void onUpdateBegin(EventHandler handler);

lib/Types/GroupStateField.cpp

@@ -40,3 +40,13 @@ const char* GroupStateFieldHelpers::getFieldName(GroupStateField field) {
   }
   return STATE_NAMES[0];
 }
+
+bool GroupStateFieldHelpers::isBrightnessField(GroupStateField field) {
+  switch (field) {
+    case GroupStateField::BRIGHTNESS:
+    case GroupStateField::LEVEL:
+      return true;
+    default:
+      return false;
+  }
+}

lib/Types/GroupStateField.h

@@ -52,6 +52,7 @@ class GroupStateFieldHelpers {
 public:
   static const char* getFieldName(GroupStateField field);
   static GroupStateField getFieldByName(const char* name);
+  static bool isBrightnessField(GroupStateField field);
 };
 
 #endif

test/remote/spec/transition_spec.rb

@@ -321,7 +321,32 @@
       )
     end
 
-    it 'should transition from off -> on with known last brightness' do
+    it 'should transition from off -> on from 0 to a provided brightness, event when there is a last known brightness' do
+      seen_updates = {}
+      @client.patch_state({status: 'ON', brightness: 99}, @id_params)
+      @client.patch_state({status: 'OFF'}, @id_params)
+
+      @mqtt_client.on_update(@id_params) do |id, message|
+        message.each do |k, v|
+          seen_updates[k] ||= []
+          seen_updates[k] << v
+        end
+        seen_updates['brightness'] && seen_updates['brightness'].last == 128
+      end
+
+      @client.patch_state({status: 'ON', brightness: 128, transition: 1.0}, @id_params)
+
+      @mqtt_client.wait_for_listeners
+
+      transitions_are_equal(
+        expected: calculate_transition_steps(start_value: 0, end_value: 128, duration: 1000),
+        seen: seen_updates['brightness'],
+        # Allow some variation for the lossy level -> brightness conversion
+        allowed_variation: 4
+      )
+    end
+
+    it 'should transition from off -> on from 0 to 100, even when there is a last known brightness' do
       seen_updates = {}
       @client.patch_state({status: 'ON', brightness: 99}, @id_params)
       @client.patch_state({status: 'OFF'}, @id_params)
@@ -339,7 +364,7 @@
       @mqtt_client.wait_for_listeners
 
       transitions_are_equal(
-        expected: calculate_transition_steps(start_value: 99, end_value: 255, duration: 1000),
+        expected: calculate_transition_steps(start_value: 0, end_value: 255, duration: 1000),
         seen: seen_updates['brightness'],
         # Allow some variation for the lossy level -> brightness conversion
         allowed_variation: 4