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

Add bluetooth animation in ble download mode #17

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

kienvo
Copy link
Member

@kienvo kienvo commented Jun 10, 2024

This is an animation indicating the badge is in download mode.

Summary by Sourcery

This pull request adds a Bluetooth animation to indicate the badge is in download mode. It replaces the framebuffer (fb) with a bitmap (bm) for handling display data and animations, introduces various animation functions, and implements task management for handling different animation steps and Bluetooth download mode. Additionally, it updates the documentation to correct animation mode descriptions and modifies the build system to include new source files.

  • New Features:
    • Introduced a Bluetooth animation to indicate the badge is in download mode.
  • Enhancements:
    • Replaced framebuffer (fb) with bitmap (bm) for handling display data and animations.
    • Added various animation functions including scroll, fixed, laser, snowflake, and picture animations.
    • Implemented task management for handling different animation steps and Bluetooth download mode.
  • Build:
    • Updated Makefile to include new source files for animation and bitmap handling.
  • Documentation:
    • Updated BadgeBLE.md to correct the animation mode descriptions.

@kienvo kienvo force-pushed the ble-indicate branch 2 times, most recently from f398591 to e4379fd Compare June 12, 2024 16:32
@mariobehling
Copy link
Member

@sourcery-ai review

Copy link

sourcery-ai bot commented Jul 22, 2024

Reviewer's Guide by Sourcery

This pull request introduces a Bluetooth animation to indicate when the badge is in download mode. It also adds functions to handle mode transitions and power off the device. The Makefile has been updated to include new source files required for these features.

File-Level Changes

Files Changes
src/main.c
src/data.c
src/data.h
Replaced framebuffer (fb) related functions and variables with bitmap (bm) equivalents, added new functions for handling animations and mode transitions.
src/ble/peripheral.c
src/ble/setup.h
Added functions to enable and disable Bluetooth advertising.
src/animation.c
src/bmlist.c
src/bmlist.h
src/animation.h
Added new files to handle bitmap list operations and various animation functions.
src/fb.c
src/fb.h
Deleted files as they have been replaced by new bitmap list files.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a 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: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/animation.h Outdated Show resolved Hide resolved
@kienvo kienvo marked this pull request as ready for review July 29, 2024 06:25
Copy link

@sourcery-ai sourcery-ai bot left a 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: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 1 issue found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -198,16 +198,26 @@ static uint16 peripheral_task(uint8 task_id, uint16 events)
return 0;
}

void ble_enable_advertise()
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider adding power management for BLE advertising

The new functions to enable and disable BLE advertising are a good addition. Consider implementing a power management strategy that balances advertising frequency with power consumption, especially for battery-operated devices.

Suggested change
void ble_enable_advertise()
void ble_enable_advertise()
{
uint8 e = TRUE;
// Set advertising interval to balance power consumption
uint16_t advertising_interval = 1000; // Adjust this value as needed
set_advertising_interval(advertising_interval);
}

src/animation.c Outdated
return ++i;
}

void ani_scroll_x(bm_t *bm, uint16_t *fb, int dir)
Copy link

Choose a reason for hiding this comment

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

suggestion (performance): Optimize animation functions for performance

The animation functions, particularly those involving shifts and rotations, could potentially be optimized. Consider using bit manipulation techniques or lookup tables where appropriate to improve performance, especially for resource-constrained devices.

Suggested change
void ani_scroll_x(bm_t *bm, uint16_t *fb, int dir)
void ani_scroll_x(bm_t *bm, uint16_t *fb, int dir)
{
int x = (bm->anim_step & (bm->width + LED_COLS - 1)) - LED_COLS;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

????

Copy link

Choose a reason for hiding this comment

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

It looks like there was some confusion regarding the suggestion. The suggestion was aimed at optimizing the animation functions for better performance, particularly on resource-constrained devices. The idea is to explore using bit manipulation techniques or lookup tables to potentially speed up operations like shifts and rotations within the animation functions.

However, the specific code snippet provided in the suggestion might not have been clear or directly applicable. If you have specific areas in the animation code where performance is a concern, we can focus on those and discuss potential optimizations. Let me know if there's a particular part of the code you'd like to explore further or if you have any questions about the suggestion!

src/main.c Outdated Show resolved Hide resolved
BadgeBLE.md Outdated Show resolved Hide resolved
@mariobehling mariobehling merged commit a8b32e9 into fossasia:master Aug 27, 2024
1 check passed
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.

3 participants