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

Small fixes to LED Animation system #474

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Conversation

Procdox
Copy link
Contributor

@Procdox Procdox commented Aug 28, 2023

Fix for #431, animation state breaks for cycling animations when they are made too fast.
Also includes a fix to a potential memory corruption via Animation pointers in the AnimationStation.
Another small change to the RBG struct constructors to make them Trivial classes.

Please note, as my hitbox has not yet arrived I am unable to test these changes past simply ensuring they compile.

Cause: The Rainbow and Chase animations could have their cycle time lowered to a negative number. make_timeout_time_ms returns int64 max if given a negative value. Another frame would never be processed while in the animation. Changing animations resets the timestamp to 0, which is why switching animations after raising the cycle fixed it.

Fix: Clamp the cycle time within a safe range.
Potential memory corruption in HandleEvent via unitialized animation pointers: added a guard.
Changed default and copy ctor of RGB to be defaulted so that the memset complaint goes away and redundant vector or array initialization is avoided.
Compiler fixes this regardless, but its one less warning in the log.
@InfraredAces
Copy link
Contributor

Testing with FlatboxRev5RGB

This issue and behavior stated in #431 are still present. This is the case even after flash_nuking the board and then flashing the build firmware from the workflow run for this PR.

Bug is still being reported, clamp to 10...max/16 and check before timeout is set in animate call
Copy link
Contributor

@arntsonl arntsonl left a comment

Choose a reason for hiding this comment

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

Should prevent crashes on animation change! Thanks!

@arntsonl arntsonl merged commit b37781f into OpenStickCommunity:main Sep 1, 2023
50 checks passed
@Procdox
Copy link
Contributor Author

Procdox commented Sep 2, 2023

I was able to consistently reproduce the issue before my changes. I was not able to do so with my changes.
However, @InfraredAces reports that they are still seeing the same or a similar issue post changes. So #431 requires further investigation.

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