diff --git a/lib/MiLight/MiLightClient.cpp b/lib/MiLight/MiLightClient.cpp index ba2ae17c..6e939252 100644 --- a/lib/MiLight/MiLightClient.cpp +++ b/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 transitionBuilder = nullptr; @@ -446,10 +461,10 @@ 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; @@ -457,11 +472,17 @@ void MiLightClient::handleTransition(GroupStateField field, JsonVariant value, f 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); diff --git a/lib/MiLight/MiLightClient.h b/lib/MiLight/MiLightClient.h index accea717..8146335b 100644 --- a/lib/MiLight/MiLightClient.h +++ b/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); diff --git a/lib/Types/GroupStateField.cpp b/lib/Types/GroupStateField.cpp index cc796017..33434b69 100644 --- a/lib/Types/GroupStateField.cpp +++ b/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; + } +} \ No newline at end of file diff --git a/lib/Types/GroupStateField.h b/lib/Types/GroupStateField.h index 423d7ee3..e58dc2dc 100644 --- a/lib/Types/GroupStateField.h +++ b/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 diff --git a/test/remote/spec/transition_spec.rb b/test/remote/spec/transition_spec.rb index f8d86414..e3d2df46 100644 --- a/test/remote/spec/transition_spec.rb +++ b/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