Add animations. #8

kattni · 2019-12-06T20:59:37Z

Created animations.py and added animations and animation features:

Animations: ColorCycle, Solid, Blink, Comet, Sparkle
AnimationGroup - ability to add multiple LED instances (such as a strip and the CPB onboard LEDs) to a group to animate both simultaneously
AnimationSequence - cycle through animations
Animate features: fill, freeze, resume, change_color

Added RAINBOW color list to color.py.

…animator

…ere it left off.

caternuson · 2019-12-10T00:20:27Z

adafruit_led_animation/animation.py

+    def draw(self):
+        """
+        Animation subclasses must implement draw() to render the animation sequence.
+        """


add a raise NotImplementedError() here instead of leaving it empty

Good catch.

caternuson · 2019-12-10T00:22:09Z

adafruit_led_animation/animation.py

+        self._next_update = monotonic_ns()
+        self.pixel_object.auto_write = False
+        self.color = color
+        self.peers = peers if peers else []


This could be done by just having the default for the parameter be [] instead of None

Pylint hates [] as the default for a param.
W: 70, 4: Dangerous default value [] as argument (dangerous-default-value)

caternuson · 2019-12-10T00:31:14Z

adafruit_led_animation/animation.py

+    def change_color(self, color):
+        """
+        By default changing color uses the color property.
+        """


Why is this needed? Why not just use the color property setter?

It's difficult to override a property setter. I'm trying to remember why I needed it. I think _recompute_color is what needs overriding and I forgot I put it there. Will try to squash this.

Oh, now I remember - it ended up there because I didn't use a property on an animation sequence, time to fix that.

caternuson · 2019-12-10T00:32:06Z

adafruit_led_animation/animation.py

+    def change_color(self, color):
+        """
+        ColorCycle doesn't support change_color.
+        """


Should an exception be raised here?

Code removed.

caternuson · 2019-12-10T00:41:12Z

adafruit_led_animation/animation.py

+        """
+        self._next_update = monotonic_ns() + self._time_left_at_pause
+        self._time_left_at_pause = 0
+        self._paused = False


How about replacing freeze() and resume() with a paused property that can be set or return current state?

# to pause animation.paused = True # to check if animation.paused: print("Animation currently paused.") # to un-pause animation.paused = False

The freeze/resume protocol on the Animation children grew out of the AnimationSequence and AnimationGroup. It would be possible to use a paused property, but it doesn't behave cleanly:

AnimationSequence freezes/resumes the current animation in the sequence.
AnimationGroup freezes/resumes all members of the group.

Querying AnimationSequence.paused would return the sequence paused state so that wouldn't be too bad.

Querying AnimationGroup.paused however would need to return all() or any() on the paused state of all members of the group - as it doesn't itself know if they're paused.

I intentionally made the basic signatures for Animations, AnimationGroup and AnimationSequence identical so you can mix and match them in any order.

So is the only potential confusion when querying the current state of an AnimationGroup? If freeze/resume acts on all members, how do you get to a state where they are mixed?

Having a mixed state in an AnimationGroup seems to defeat the purpose of the Group. If your end goal is a mixed state, you'd want to handle things separately.

@caternuson What is your reasoning behind suggesting this change? I'm trying to understand the purpose of the suggestion.

i guess the more important thing here is what do we want the interface to be for pausing and resuming. a property or two methods? @caternuson @kattni

Was thinking it would be more Pythonic. Also, to have a way to get current state. But this could be refactored later.

Refactoring it later not the right choice as it would change the protocol people use.... So I'll get this refactored ASAP, as @kattni needs this for her latest guide.

I'd say keep as is then. The audioio API has separate functions for play/stop/pause:
https://circuitpython.readthedocs.io/en/latest/shared-bindings/audioio/AudioOut.html
so seems OK.

Reverted back to the freeze/resume methods.

Add Pulse animation

rhooper · 2019-12-10T18:36:23Z

@caternuson @kattni Let me know which you prefer.

caternuson · 2019-12-10T18:39:32Z

@rhooper Now that I've looked at the audioio API, what do you think about mimicking that? Is your animation control similar enough to audio control?

rhooper · 2019-12-10T18:42:25Z

@caternuson I'm not sure, I'll have to look at the AudioOut API when I have time.

caternuson · 2019-12-10T18:45:07Z

@rhooper @kattni If this is time critical, just go with what you had originally, the freeze() and resume() functions.

theacodes · 2019-12-10T18:35:56Z

adafruit_led_animation/animation.py

+        """
+        The animation speed in fractional seconds.
+        """
+        return self._speed_ns / 1000000000


Since this is technically a "magic" number and appears more than once, consider pulling this into a module-level constant, like https://github.com/theacodes/Winterbloom-Sol-software/blob/ce2b8b07f19b5f5c20cd32fa50e2ef7e7007c603/winterbloom_sol/adsr.py#L30

I had thought about that, but I am also concerned about performance. I vaguely remember there's a constant thing in CircuitPython that might be faster than a bare module variable, but I need to benchmark. Having worked a lot in the CircuitPython core with a JLink and gdb lately makes me think about these things :)

I'll look into the performance of both.

From my experience moving things to module-level variables doesn't impact performance, and might even help, considering that CP doesn't need to do an allocation for the module-level variable every time you use it.

If performance is a big concern we should do another pass at this once it's merged. There's lots of use of for x in y, range, and slicing that outright murder performance on CP compared to manipulating index variables.

Another pass at it was already planned. The initial code was written to make it work. Making it better is the next step.

@theacodes there is indeed plenty of room for performance fixes. Two different things happened while writing this: 1) Making it work. 2) A lot of the code I wrote was written with _pixelbuf in mind, which perform faster with slices.. Running the code without _pixelbuf is, as you said, much slower. Once we get this merged, there's a number of issues to file regarding improvements, additions, and design documentation.

I moved this to a constant in the module.

theacodes · 2019-12-10T18:38:38Z

adafruit_led_animation/animation.py

+        self._reverse_comet_colors = None
+        self.reverse = reverse
+        self.bounce = bounce
+        super(Comet, self).__init__(pixel_object, speed, color)


nit: it's unusual to see the superclass __init__ so far down. I've generally seen this done as soon as possible, like in the very first statement in the __init__ method.

I usually do it first, but in this case, when I did this, the color setup work needed doing before the rest of super is run. I'll check if that's still true. If not, a comment would be useful.

Comments added and init for both the base class and this class reorganized slightly.

theacodes · 2019-12-10T18:41:33Z

adafruit_led_animation/animation.py

+        self._dim_color = dim_color
+
+    def draw(self):
+        pixel = random.randint(0, (len(self.pixel_object) - 2))


what if the pixel object has less than 2 pixels?

Then it would crash. Good call. (note: this was based on code from elsewhere, and this animation doesn't support single pixels -- init should check that).

Init now checks that the strip is big enough.

theacodes · 2019-12-10T18:50:05Z

adafruit_led_animation/animation.py

+        self._last_update = now
+        self._cycle_position = (self._cycle_position + time_since_last_draw) % self._period
+        intensity = self.min_intensity + (
+            sin(self._radians_per_second * self._cycle_position) * self._intensity_delta)


sin's range is -1.0 to 1.0, so this could end up setting intensity to a negative number.

For example:

min_intensity = 0.0 max_intensity = 1.0 intensity_delta = max_intensity - min_intensity # 1.0 # assume radians per second and cycle position end up passing 1.5pi to sin. intensity = min_intensity + (sin(1.5 * pi) * intensity_delta) intensity = 0.0 + (-1.0 * 1.0) intensity = -1.0

The pixel colors would then be multiplied by -1 which likely isn't what we want.

You can solve this by moving sin's range to be 0 - 1.0 like this:

def normalized_sin(angle): return 0.5 + (sin(angle) * 0.5)

I'm planning to remove the use of sin() very soon, as the performance is poor. The code, however, shouldn't produce a negative brightness because it's clamped to 0-180 degrees by self._radians_per_second and % self._period set during init.

Sin removed in favour of a linear function for now. Code still needs significant optimization as expected.

This reverts commit 8afcbcf.

kattni · 2019-12-11T18:18:54Z

@theacodes Thank you for the review notes. @rhooper addressed them. As getting this merged is time-sensitive, I'm going to go ahead and merge it. If you have further suggestions, please file an issue. We intend to file separate issues for each of the improvements we want to make to make it easier to discuss them in the thread - please consider doing the same if you decide to file any issues. Thanks!

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 3.1.1 from 3.1.0: > Merge pull request adafruit/Adafruit_CircuitPython_BLE#43 from makermelissa/master Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 1.0.0 from 0.8.2: > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#8 from kattni/animations > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#7 from makermelissa/master > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#5 from demophoon/add-blinka-to-requirements-txt

kattni and others added 30 commits November 29, 2019 16:51

Folder rename, add three animations.

7454f67

Documented simpletest.

a6cd418

Linting.

18d3897

Refactor to remove common code, support integer colours

3c5400f

add a generic color cycler and move blink to use it, and add a solid …

bcf68c0

…animator

don't use self.color

b31bc3d

Fix lint

4d9503d

tidy up but maybe not fix yet?

140b644

Docs updates.

3a03bc1

fix timing issue

245089b

switch to monotonic_ns to handle long uptimes

487bf35

Removing speed arg from Solid color

48fa1f4

fix? int to rgb

ce5a72b

fix init bug and add a dummypixels to test with

1b02cd8

Add docs, default ColorCycle, RAINBOW.

0ae0359

Fix Solid.

41dcfdb

Update docstring in example.

1a673ec

fix lint

cc0fb4c

add animation sequences and groups

86678e3

Fix groups and add sync ability

531065f

fix some bugs

9960c11

make comets reversible

9f0d38a

make it bounceable

1de9ad0

make it ready for kattni to make it beautiful

b09ed37

More docs.

6f5a2ca

add more features, fix auto advance.

a3e3b75

debounce pause/resume for sequences and ensure the pause continues wh…

81a3174

…ere it left off.

Fill fix, more docs.

9010299

Sphinx.

47f1e88

Update travis.yml for directory rename

eccb2fc

rhooper added 9 commits December 9, 2019 12:23

Add Pulse animation.

71cc6f1

add docs

6f522ac

add chase

2db4899

fix chase

4621212

fix reverse flag

7ac5968

add a way to control the color of each item in the chase

9626ad4

fix lint

d5494c2

fix chase bugs

072aad6

use a sine wave and time based positioning

17faa95

caternuson reviewed Dec 10, 2019

View reviewed changes

kattni and others added 4 commits December 9, 2019 20:09

Merge pull request #1 from rhooper/more-anims

6c51625

Add Pulse animation

fix a bug

0f8f539

make it fit 100 chars

1ac6f67

code review changes

ea69347

caternuson approved these changes Dec 10, 2019

View reviewed changes

kattni mentioned this pull request Dec 10, 2019

Circuit Playground Bluefruit NeoPixel Controller adafruit/Adafruit_Learning_System_Guides#957

Merged

switch from freeze/resume to paused property

8afcbcf

theacodes reviewed Dec 10, 2019

View reviewed changes

rhooper added 4 commits December 10, 2019 17:33

Revert "switch from freeze/resume to paused property"

f6da086

This reverts commit 8afcbcf.

address some code review concerns

0d92928

remove use of sin from pulse

eb24e61

remove unnecessary pass

4d00304

kattni merged commit ef103a0 into adafruit:master Dec 11, 2019

kattni deleted the animations branch November 3, 2021 20:51

Add animations. #8

Add animations. #8

Conversation

kattni commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhooper commented Dec 10, 2019

caternuson commented Dec 10, 2019

rhooper commented Dec 10, 2019

caternuson commented Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattni commented Dec 11, 2019

caternuson commented Dec 10, 2019 •

edited

Loading