Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make breathe function more flexible #78

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

boraozgen
Copy link
Contributor

@boraozgen boraozgen commented Oct 6, 2021

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
Copy link
Contributor Author

boraozgen commented Oct 12, 2021

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

@jandelgado
Copy link
Owner

jandelgado commented Oct 13, 2021

@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
Copy link

coveralls commented Oct 18, 2021

Coverage Status

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

@boraozgen
Copy link
Contributor Author

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.

@boraozgen boraozgen closed this Oct 18, 2021
@boraozgen boraozgen deleted the feature/advanced-breathe branch October 18, 2021 08:45
@boraozgen boraozgen restored the feature/advanced-breathe branch October 18, 2021 08:46
@boraozgen boraozgen reopened this Oct 18, 2021
src/jled_base.h Outdated
: fadeon_func(period_ - t, periodh);
auto ret = kZeroBrightness;
if (t < duration_fade_on_)
ret = fadeon_func(t, duration_fade_on_);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove variable ret and just return the calculated value, instead of overwriting it (all occurences)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/jled_base.h Outdated
ret = kFullBrightness;
else if (t < Period())
ret = fadeon_func(Period() - t, duration_fade_off_);
return ret;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else return kZeroBirghtness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

In addition to just a period argument, the Breathe method
now accepts fade-on, on and fade-off durations for a
more flexible breathe animation. The old API is preserved
with a shim layer.
@jandelgado
Copy link
Owner

Thank you very much the PR!

@jandelgado jandelgado merged commit d0d8617 into jandelgado:master Oct 19, 2021
@boraozgen boraozgen deleted the feature/advanced-breathe branch October 20, 2021 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants