-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add animations #15
Add animations #15
Conversation
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request introduces initial support for animations, including a splash screen animation and various scrolling animations. It also updates the build configuration to include new animation-related source files. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kienvo - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -67,6 +69,13 @@ static void fb_transition() | |||
{ | |||
fblist_gonext(); | |||
} | |||
void play_splash(xbm_t *xbm, int col, int row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider making play_splash
static.
If play_splash
is only used within this file, it would be better to make it static to limit its scope and avoid potential naming conflicts.
void play_splash(xbm_t *xbm, int col, int row) | |
static void play_splash(xbm_t *xbm, int col, int row) |
0a659eb
to
2f98dba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kienvo - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding synchronization mechanisms for the shared
fb
array to prevent potential race conditions. - It might be beneficial to add a comment explaining the rationale behind the transition from the
fb
system to the newbmlist
system.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
laser_out(bm, fb, c - LED_COLS * 2, frame); | ||
} | ||
|
||
static uint32_t b16dialate(uint16_t w, int from, int to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Optimize bit manipulation for better performance
The bit manipulation in b16dialate
and similar functions could potentially be optimized. Consider using lookup tables or more efficient bit twiddling techniques to improve performance, especially important for embedded systems.
There are just 2 animations here as implemented for Bluetooth mode indication and splash but there will be more on this PR.
Summary by Sourcery
This pull request introduces animations for Bluetooth mode indication and splash screen, refactors framebuffer handling to bitmap handling, and implements a task management system for animations. It also updates the documentation and build configuration to support these changes.