Merge pull request #527 from sidoh/default_period_in_settings

Add default transition period to settings

docs/openapi.yaml

@@ -582,9 +582,6 @@ components:
         period:
           type: integer
           description: Length of time between updates in a transition, measured in milliseconds
-        num_periods:
-          type: integer
-          description: Number of packets sent over the course of the transition
     TransitionData:
       allOf:
         - $ref: '#/components/schemas/TransitionArgs'
@@ -990,6 +987,11 @@ components:
           example:
             alias1: [1234, 'rgb_cct', 1]
             alias2: [1234, 'rgb_cct', 2]
+        default_transition_period:
+          type: integer
+          description: |
+            Default number of milliseconds between transition packets.  Set this value lower for more granular transitions, or higher if
+            you are having performance issues during transitions.
 
     BooleanResponse:
       type: object

lib/MiLight/MiLightClient.cpp

@@ -605,9 +605,6 @@ bool MiLightClient::handleTransition(JsonObject args, JsonDocument& responseObj)
   if (args.containsKey(FS(TransitionParams::PERIOD))) {
     transitionBuilder->setPeriod(args[FS(TransitionParams::PERIOD)]);
   }
-  if (args.containsKey(FS(TransitionParams::NUM_PERIODS))) {
-    transitionBuilder->setNumPeriods(args[FS(TransitionParams::NUM_PERIODS)]);
-  }
 
   transitions.addTransition(transitionBuilder->build());
   return true;

lib/MiLight/MiLightClient.h

@@ -29,7 +29,6 @@ namespace TransitionParams {
   static const char END_VALUE[] PROGMEM = "end_value";
   static const char DURATION[] PROGMEM = "duration";
   static const char PERIOD[] PROGMEM = "period";
-  static const char NUM_PERIODS[] PROGMEM = "num_periods";
 }
 
 // Used to determine RGB colros that are approximately white

lib/Settings/Settings.cpp

@@ -99,6 +99,7 @@ void Settings::patch(JsonObject parsedSettings) {
   this->setIfPresent(parsedSettings, "wifi_static_ip_netmask", wifiStaticIPNetmask);
   this->setIfPresent(parsedSettings, "packet_repeats_per_loop", packetRepeatsPerLoop);
   this->setIfPresent(parsedSettings, "home_assistant_discovery_prefix", homeAssistantDiscoveryPrefix);
+  this->setIfPresent(parsedSettings, "default_transition_period", defaultTransitionPeriod);
 
   if (parsedSettings.containsKey("wifi_mode")) {
     this->wifiMode = wifiModeFromString(parsedSettings["wifi_mode"]);
@@ -288,6 +289,7 @@ void Settings::serialize(Print& stream, const bool prettyPrint) {
   root["packet_repeats_per_loop"] = this->packetRepeatsPerLoop;
   root["home_assistant_discovery_prefix"] = this->homeAssistantDiscoveryPrefix;
   root["wifi_mode"] = wifiModeToString(this->wifiMode);
+  root["default_transition_period"] = this->defaultTransitionPeriod;
 
   JsonArray channelArr = root.createNestedArray("rf24_channels");
   JsonHelpers::vectorToJsonArr<RF24Channel, String>(channelArr, rf24Channels, RF24ChannelHelpers::nameFromValue);

lib/Settings/Settings.h

@@ -118,6 +118,7 @@ public:
     rf24ListenChannel(RF24Channel::RF24_LOW),
     packetRepeatsPerLoop(10),
     wifiMode(WifiMode::N),
+    defaultTransitionPeriod(500),
     _autoRestartPeriod(0)
   { }
 
@@ -191,6 +192,7 @@ public:
   std::map<uint32_t, BulbId> deletedGroupIdAliases;
   String homeAssistantDiscoveryPrefix;
   WifiMode wifiMode;
+  uint16_t defaultTransitionPeriod;
 
 protected:
   size_t _autoRestartPeriod;

lib/Transitions/ChangeFieldOnFinishTransition.cpp

@@ -7,7 +7,7 @@ ChangeFieldOnFinishTransition::Builder::Builder(
   uint16_t arg,
   std::shared_ptr<Transition::Builder> delegate
 )
-  : Transition::Builder(delegate->id, delegate->bulbId, delegate->callback, delegate->getMaxSteps())
+  : Transition::Builder(delegate->id, delegate->defaultPeriod, delegate->bulbId, delegate->callback, delegate->getMaxSteps())
   , delegate(delegate)
   , field(field)
   , arg(arg)
@@ -15,7 +15,6 @@ ChangeFieldOnFinishTransition::Builder::Builder(
 
 std::shared_ptr<Transition> ChangeFieldOnFinishTransition::Builder::_build() const {
   delegate->setDurationRaw(this->getOrComputeDuration());
-  delegate->setNumPeriods(this->getOrComputeNumPeriods());
   delegate->setPeriod(this->getOrComputePeriod());
 
   return std::make_shared<ChangeFieldOnFinishTransition>(

lib/Transitions/ColorTransition.cpp

@@ -1,8 +1,8 @@
 #include <ColorTransition.h>
 #include <Arduino.h>
 
-ColorTransition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback, const ParsedColor& start, const ParsedColor& end)
-  : Transition::Builder(id, bulbId, callback, calculateMaxDistance(start, end))
+ColorTransition::Builder::Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, const ParsedColor& start, const ParsedColor& end)
+  : Transition::Builder(id, defaultPeriod, bulbId, callback, calculateMaxDistance(start, end))
   , start(start)
   , end(end)
 { }

lib/Transitions/ColorTransition.h

@@ -16,7 +16,7 @@ public:
 
   class Builder : public Transition::Builder {
   public:
-    Builder(size_t id, const BulbId& bulbId, TransitionFn callback, const ParsedColor& start, const ParsedColor& end);
+    Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, const ParsedColor& start, const ParsedColor& end);
 
     virtual std::shared_ptr<Transition> _build() const override;
 

lib/Transitions/FieldTransition.cpp

@@ -2,9 +2,10 @@
 #include <cmath>
 #include <algorithm>
 
-FieldTransition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback, GroupStateField field, uint16_t start, uint16_t end)
+FieldTransition::Builder::Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, GroupStateField field, uint16_t start, uint16_t end)
   : Transition::Builder(
       id,
+      defaultPeriod,
       bulbId,
       callback,
       max(

lib/Transitions/FieldTransition.h

@@ -12,7 +12,7 @@ public:
 
   class Builder : public Transition::Builder {
   public:
-    Builder(size_t id, const BulbId& bulbId, TransitionFn callback, GroupStateField field, uint16_t start, uint16_t end);
+    Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, GroupStateField field, uint16_t start, uint16_t end);
 
     virtual std::shared_ptr<Transition> _build() const override;
 

lib/Transitions/Transition.cpp

@@ -2,8 +2,9 @@
 #include <Arduino.h>
 #include <cmath>
 
-Transition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback, size_t maxSteps)
+Transition::Builder::Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, size_t maxSteps)
   : id(id)
+  , defaultPeriod(defaultPeriod)
   , bulbId(bulbId)
   , callback(callback)
   , duration(0)
@@ -26,11 +27,6 @@ Transition::Builder& Transition::Builder::setPeriod(size_t period) {
   return *this;
 }
 
-Transition::Builder& Transition::Builder::setNumPeriods(size_t numPeriods) {
-  this->numPeriods = numPeriods;
-  return *this;
-}
-
 Transition::Builder& Transition::Builder::setDurationAwarePeriod(size_t period, size_t duration, size_t maxSteps) {
   if ((period * maxSteps) < duration) {
     setPeriod(std::ceil(duration / static_cast<float>(maxSteps)));
@@ -116,14 +112,14 @@ std::shared_ptr<Transition> Transition::Builder::build() {
 
   if (numSet == 0) {
     setDuration(DEFAULT_DURATION);
-    setDurationAwarePeriod(DEFAULT_PERIOD, duration, maxSteps);
+    setDurationAwarePeriod(defaultPeriod, duration, maxSteps);
   } else if (numSet == 1) {
     // If duration is unbound, bind it
     if (! isSetDuration()) {
       setDurationRaw(DEFAULT_DURATION);
     // Otherwise, bind the period
     } else {
-      setDurationAwarePeriod(DEFAULT_PERIOD, duration, maxSteps);
+      setDurationAwarePeriod(defaultPeriod, duration, maxSteps);
     }
   }
 

lib/Transitions/Transition.h

@@ -18,11 +18,10 @@ public:
 
   class Builder {
   public:
-    Builder(size_t id, const BulbId& bulbId, TransitionFn callback, size_t maxSteps);
+    Builder(size_t id, uint16_t defaultPeriod, const BulbId& bulbId, TransitionFn callback, size_t maxSteps);
 
     Builder& setDuration(float duration);
     Builder& setPeriod(size_t period);
-    Builder& setNumPeriods(size_t numPeriods);
 
     /**
      * Users are typically defining transitions using:
@@ -55,6 +54,7 @@ public:
     std::shared_ptr<Transition> build();
 
     const size_t id;
+    const uint16_t defaultPeriod;
     const BulbId& bulbId;
     const TransitionFn callback;
 
@@ -68,14 +68,9 @@ public:
     size_t numSetParams() const;
   };
 
-  // Default time to wait between steps.  Do this rather than having a fixed step size because it's
-  // more capable of adapting to different situations.
-  static const size_t DEFAULT_PERIOD = 225;
-  static const size_t DEFAULT_NUM_PERIODS = 20;
-  static const size_t DEFAULT_DURATION = DEFAULT_PERIOD*DEFAULT_NUM_PERIODS;
-
   // If period goes lower than this, throttle other parameters up to adjust.
   static const size_t MIN_PERIOD = 150;
+  static const size_t DEFAULT_DURATION = 10000;
 
   const size_t id;
   const BulbId bulbId;

lib/Transitions/TransitionController.cpp

@@ -14,8 +14,13 @@ using namespace std::placeholders;
 TransitionController::TransitionController()
   : callback(std::bind(&TransitionController::transitionCallback, this, _1, _2, _3))
   , currentId(0)
+  , defaultPeriod(500)
 { }
 
+void TransitionController::setDefaultPeriod(uint16_t defaultPeriod) {
+  this->defaultPeriod = defaultPeriod;
+}
+
 void TransitionController::clearListeners() {
   observers.clear();
 }
@@ -27,6 +32,7 @@ void TransitionController::addListener(Transition::TransitionFn fn) {
 std::shared_ptr<Transition::Builder> TransitionController::buildColorTransition(const BulbId& bulbId, const ParsedColor& start, const ParsedColor& end) {
   return std::make_shared<ColorTransition::Builder>(
     currentId++,
+    defaultPeriod,
     bulbId,
     callback,
     start,
@@ -37,6 +43,7 @@ std::shared_ptr<Transition::Builder> TransitionController::buildColorTransition(
 std::shared_ptr<Transition::Builder> TransitionController::buildFieldTransition(const BulbId& bulbId, GroupStateField field, uint16_t start, uint16_t end) {
   return std::make_shared<FieldTransition::Builder>(
     currentId++,
+    defaultPeriod,
     bulbId,
     callback,
     field,

lib/Transitions/TransitionController.h

@@ -13,6 +13,7 @@ public:
 
   void clearListeners();
   void addListener(Transition::TransitionFn fn);
+  void setDefaultPeriod(uint16_t period);
 
   std::shared_ptr<Transition::Builder> buildColorTransition(const BulbId& bulbId, const ParsedColor& start, const ParsedColor& end);
   std::shared_ptr<Transition::Builder> buildFieldTransition(const BulbId& bulbId, GroupStateField field, uint16_t start, uint16_t end);
@@ -32,6 +33,7 @@ private:
   LinkedList<std::shared_ptr<Transition>> activeTransitions;
   std::vector<Transition::TransitionFn> observers;
   size_t currentId;
+  uint16_t defaultPeriod;
 
   void transitionCallback(const BulbId& bulbId, GroupStateField field, uint16_t arg);
 };

src/main.cpp

@@ -219,6 +219,8 @@ void applySettings() {
     delete radios;
   }
 
+  transitions.setDefaultPeriod(settings.defaultTransitionPeriod);
+
   radioFactory = MiLightRadioFactory::fromSettings(settings);
 
   if (radioFactory == NULL) {

test/remote/spec/settings_spec.rb

@@ -29,7 +29,8 @@ RSpec.describe 'Settings' do
         'simple_mqtt_client_status' => [true, false],
         'packet_repeats_per_loop' => [10],
         'home_assistant_discovery_prefix' => ['', 'abc', 'a/b/c'],
-        'wifi_mode' => %w(b g n)
+        'wifi_mode' => %w(b g n),
+        'default_transition_period' => [200, 500]
       }.each do |key, values|
         values.each do |v|
           @client.patch_settings({key => v})

test/remote/spec/transition_spec.rb

@@ -680,4 +680,33 @@ RSpec.describe 'Transitions' do
       end
     end
   end
+
+  context 'default parameters in settings' do
+    it 'should respect the default parameter setting key' do
+      [500, 1000, 2000].each do |period|
+        field = 'brightness'
+
+        @client.patch_settings(default_transition_period: period)
+        @client.delete_state(@id_params)
+        @client.patch_state({'status' => 'ON', field => 0}, @id_params)
+        seen_updates = []
+
+        @mqtt_client.on_update(@id_params) do |id, message|
+          seen_updates << message[field] if !message[field].nil?
+          seen_updates.last == 255
+        end
+
+        @client.patch_state({field => 255, 'transition' => 2.0, period: period}, @id_params)
+
+        @mqtt_client.wait_for_listeners
+        @mqtt_client = create_mqtt_client()
+
+        transitions_are_equal(
+          expected: calculate_transition_steps(start_value: 0, end_value: 255, duration: 2000, period: period),
+          seen: seen_updates,
+          allowed_variation: 3
+        )
+      end
+    end
+  end
 end

web/src/js/script.js

@@ -39,6 +39,9 @@ var UI_TABS = [ {
   }, {
     tag: "tab-mqtt",
     friendly: "MQTT"
+  }, {
+    tag: "tab-transitions",
+    friendly: "Transitions"
   }
 ];
 
@@ -342,6 +345,13 @@ var UI_FIELDS = [ {
     help: "Number of times the LED will flash when packets are changing",
     type: "string",
     tab: "tab-led"
+  }, {
+    tag: "default_transition_period",
+    friendly: "Default transition period (milliseconds)",
+    help: "Controls how many milliseconds pass between transition packets. "+
+      "For more granular transitions, set this lower.",
+    type: "string",
+    tab: "tab-transitions"
   }
 ];
 
@@ -1231,6 +1241,27 @@ $(function() {
 
   $('#settings').prepend(settings);
 
+  function saveSettings(settingsEntries) {
+    var entries = settingsEntries.slice(0)
+
+    function saveBatch() {
+      if (entries.length > 0) {
+        var batch = Object.fromEntries(entries.splice(0, 30))
+        $.ajax(
+          "/settings",
+          {
+            method: "PUT",
+            contentType: "application/json",
+            data: JSON.stringify(batch)
+          }
+        )
+        .done(saveBatch)
+      }
+    }
+
+    saveBatch()
+  }
+
   $('#settings').submit(function(e) {
     e.preventDefault();
 
@@ -1262,15 +1293,7 @@ $(function() {
       // Make sure we're submitting a value for group_state_fields (will be empty
       // if no values were selected).
       obj = $.extend({group_state_fields: []}, obj);
-
-      $.ajax(
-        "/settings",
-        {
-          method: 'put',
-          contentType: 'application/json',
-          data: JSON.stringify(obj)
-        }
-      );
+      saveSettings(Object.entries(obj))
     }
 
     $('#settings-modal').modal('hide');