Force period to be longer when number of steps does not allow for a given duration

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)
+  : Transition::Builder(delegate->id, delegate->bulbId, delegate->callback, delegate->getMaxSteps())
   , delegate(delegate)
   , field(field)
   , arg(arg)

lib/Transitions/ColorTransition.cpp

@@ -2,7 +2,7 @@
 #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)
+  : Transition::Builder(id, bulbId, callback, calculateMaxDistance(start, end))
   , start(start)
   , end(end)
 { }
@@ -84,7 +84,7 @@ ColorTransition::ColorTransition(
   stepSizes.b = calculateStepSizePart(db, duration, period);
 }
 
-size_t ColorTransition::calculateColorPeriod(ColorTransition* t, const ParsedColor& start, const ParsedColor& end, size_t stepSize, size_t duration) {
+size_t ColorTransition::calculateMaxDistance(const ParsedColor& start, const ParsedColor& end) {
   int16_t dr = end.r - start.r
         , dg = end.g - start.g
         , db = end.b - start.b;
@@ -93,7 +93,11 @@ size_t ColorTransition::calculateColorPeriod(ColorTransition* t, const ParsedCol
   int16_t min = std::min(std::min(dr, dg), db);
   int16_t maxAbs = std::abs(min) > std::abs(max) ? min : max;
 
-  return Transition::calculatePeriod(maxAbs, stepSize, duration);
+  return maxAbs;
+}
+
+size_t ColorTransition::calculateColorPeriod(ColorTransition* t, const ParsedColor& start, const ParsedColor& end, size_t stepSize, size_t duration) {
+  return Transition::calculatePeriod(calculateMaxDistance(start, end), stepSize, duration);
 }
 
 int16_t ColorTransition::calculateStepSizePart(int16_t distance, size_t duration, size_t period) {

lib/Transitions/ColorTransition.h

@@ -39,6 +39,7 @@ public:
   );
 
   static size_t calculateColorPeriod(ColorTransition* t, const ParsedColor& start, const ParsedColor& end, size_t stepSize, size_t duration);
+  inline static size_t calculateMaxDistance(const ParsedColor& start, const ParsedColor& end);
   inline static int16_t calculateStepSizePart(int16_t distance, size_t duration, size_t period);
   virtual bool isFinished() override;
 

lib/Transitions/FieldTransition.cpp

@@ -3,7 +3,7 @@
 #include <algorithm>
 
 FieldTransition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback, GroupStateField field, uint16_t start, uint16_t end)
-  : Transition::Builder(id, bulbId, callback)
+  : Transition::Builder(id, bulbId, callback, std::ceil(static_cast<int16_t>(end) - start))
   , stepSize(0)
   , field(field)
   , start(start)

lib/Transitions/Transition.cpp

@@ -2,13 +2,14 @@
 #include <Arduino.h>
 #include <cmath>
 
-Transition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback)
+Transition::Builder::Builder(size_t id, const BulbId& bulbId, TransitionFn callback, size_t maxSteps)
   : id(id)
   , bulbId(bulbId)
   , callback(callback)
   , duration(0)
   , period(0)
   , numPeriods(0)
+  , maxSteps(maxSteps)
 { }
 
 Transition::Builder& Transition::Builder::setDuration(float duration) {
@@ -30,6 +31,14 @@ Transition::Builder& Transition::Builder::setNumPeriods(size_t 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)));
+  } else {
+    setPeriod(period);
+  }
+}
+
 size_t Transition::Builder::getNumPeriods() const {
   return this->numPeriods;
 }
@@ -42,6 +51,10 @@ size_t Transition::Builder::getPeriod() const {
   return this->period;
 }
 
+size_t Transition::Builder::getMaxSteps() const {
+  return this->maxSteps;
+}
+
 bool Transition::Builder::isSetDuration() const {
   return this->duration > 0;
 }
@@ -102,14 +115,14 @@ std::shared_ptr<Transition> Transition::Builder::build() {
 
   if (numSet == 0) {
     setDuration(DEFAULT_DURATION);
-    setPeriod(DEFAULT_PERIOD);
+    setDurationAwarePeriod(DEFAULT_PERIOD, duration, maxSteps);
   } else if (numSet == 1) {
     // If duration is unbound, bind it
     if (! isSetDuration()) {
       setDurationRaw(DEFAULT_DURATION);
     // Otherwise, bind the period
     } else {
-      setPeriod(DEFAULT_PERIOD);
+      setDurationAwarePeriod(DEFAULT_PERIOD, duration, maxSteps);
     }
   }
 

lib/Transitions/Transition.h

@@ -18,12 +18,25 @@ public:
 
   class Builder {
   public:
-    Builder(size_t id, const BulbId& bulbId, TransitionFn callback);
+    Builder(size_t id, 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:
+     *   1. The desired end state (and implicitly the start state, assumed to be current)
+     *   2. The duraiton
+     * The user only cares about the period to the degree that it affects the smoothness of
+     * the transition.
+     *
+     * For example, if the user wants to throttle brightness from 0 -> 100 over 5min, the
+     * default period is going to be way too short to enable that.  So we need to force the
+     * period to be longer to fit the duration.
+     */
+    Builder& setDurationAwarePeriod(size_t desiredPeriod, size_t duration, size_t maxSteps);
+
     void setDurationRaw(size_t duration);
 
     bool isSetDuration() const;
@@ -37,6 +50,7 @@ public:
     size_t getDuration() const;
     size_t getPeriod() const;
     size_t getNumPeriods() const;
+    size_t getMaxSteps() const;
 
     std::shared_ptr<Transition> build();
 
@@ -48,6 +62,7 @@ public:
     size_t duration;
     size_t period;
     size_t numPeriods;
+    size_t maxSteps;
 
     virtual std::shared_ptr<Transition> _build() const = 0;
     size_t numSetParams() const;

test/remote/helpers/transition_helpers.rb

@@ -2,9 +2,9 @@ require 'chroma'
 
 module TransitionHelpers
   module Defaults
-    DURATION = 4500
-    PERIOD = 450
-    NUM_PERIODS = 10
+    PERIOD = 225
+    NUM_PERIODS = 20
+    DURATION = PERIOD * NUM_PERIODS
   end
 
   def highlight_value(a, highlight_ix)
@@ -47,7 +47,10 @@ module TransitionHelpers
       s << "  Seen     : #{highlight_value(seen, i)}"
     end
 
-    expect(expected.length).to eq(seen.length)
+    expect(expected.length).to eq(seen.length),
+      "Transition was a different length than expected.\n" <<
+      "  Expected : #{expected}\n" <<
+      "  Seen     : #{seen}"
 
     expected.zip(seen).each_with_index do |x, i|
       a, b = x

test/remote/spec/transition_spec.rb

@@ -181,6 +181,16 @@ RSpec.describe 'Transitions' do
       expect(updates['hue'].length == updates['brightness'].length).to eq(true), "Should have the same number of updates for both fields"
       expect(updates['hue'].length).to eq(expected_updates.length)
     end
+
+    it 'should support creating long transitions' do
+      @client.patch_state({status: 'ON', level: 0}, @id_params)
+      @client.patch_state({level: 100, transition: 60000}, @id_params)
+
+      t = @client.transitions.last
+      calculated_duration = t['period'] * (100.to_f / t['step_size'])
+
+      expect(calculated_duration).to be_within(100).of(60000*1000), "Calculated duration should be close to 600s"
+    end
   end
 
   context 'transition packets' do