jandelgado / jled

Non-blocking LED controlling library for Arduino and friends.
MIT License
331 stars 55 forks source link

Make breathe function more flexible #78

Closed boraozgen closed 2 years ago

boraozgen commented 2 years ago

Thanks for the great library! Here is a change I find useful:

Instead of just a period argument, breathe function now accepts fade-on, on and fade-off durations for more a more flexible breathe animation.

As this causes a breaking change, we could add this as a separate function instead. Open for suggestions.

boraozgen commented 2 years ago

Any comments on this? I will adapt the tests & docs too if accepted.

jandelgado commented 2 years ago

@boraozgen: Let's make it a non breaking change. For example we could use default parameters for duration_on and duration_fade_off. When set to some constant, then Breathe behaves like before: duration_fade_on == duration_fade_off and duration_on == 0. Otherwise all 3 parameters are taken as specified.

Example:

 static constexpr uint16_t kUndef16 = 0xffff;
  // Set effect to Breathe, with the given period time in ms.

    B& Breathe(uint16_t a, unti16_t b=kUndef16, unti16_t c=kUndef16) {
          if (b==kUndef16)  {
             // old behaviour, just use a
         } else {
            // new behaviour - configure all durations with a,b,c
          }
    }
...

The other option (cleaner) option would be to just introduce a second Breathe method

    B& Breathe(uint16_t duration) {
        return Breathe(duration>>1, 0, duration>>1);
     }
     B& Breathe(uint16_t duration_fade_on, unti16_t duration_on, unti16_t duration_fade_off) {
        // new code
     }

I think this is the better and cleaner approach. What Do you think?

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.08%) to 96.954% when pulling 9096933625d5c877208edf75070951fcec37b2ed on boraozgen:feature/advanced-breathe into 5a6f48c4f1d8b83925954f8267fbc90f93875909 on jandelgado:master.

boraozgen commented 2 years ago

I like the shim idea with two APIs. I find division more expressive than shifting, I believe it would be optimized by the compiler anyway.

Pushed the new version with fixed tests. I also added the new API to readme, but didn't touch the original examples with the old Breathe API for simplicity.

jandelgado commented 2 years ago

Thank you very much the PR!