new experience on charging #3382
Conversation
Summary of ChangesHello @aaravgarg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience by refining the device's LED charging indicator. The changes introduce a dynamic blinking pattern that clearly communicates both charging status and connection state simultaneously, making the device's operational status more intuitive at a glance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
bro who added this to the repo lol, remove gemini assist pls thanks |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the user experience for the charging indicator by introducing a blinking pattern. This is a great change for clarity. However, the implementation has introduced a potential issue where the LED blinks even when the device is off, which might be confusing. Additionally, the new code is a bit complex and has some duplication. I've provided a detailed comment with a suggested refactoring to simplify the logic and correct the behavior when the device is off.
| static bool charge_blink_state = false; | ||
|
|
||
| // Set LED state based on connection and charging status | ||
| if (is_charging) { | ||
| set_led_green(true); | ||
| // When charging, alternate blink between green and connection status color | ||
| charge_blink_state = !charge_blink_state; | ||
|
|
||
| if (charge_blink_state) { | ||
| // Green phase | ||
| set_led_green(true); | ||
| set_led_red(false); | ||
| set_led_blue(false); | ||
| } else { | ||
| // Connection status color phase | ||
| set_led_green(false); | ||
| if (is_connected) { | ||
| set_led_blue(true); | ||
| set_led_red(false); | ||
| } else { | ||
| set_led_red(true); | ||
| set_led_blue(false); | ||
| } | ||
| } | ||
| } else { | ||
| // Not charging - reset blink state and turn off green | ||
| charge_blink_state = false; | ||
| set_led_green(false); | ||
| } | ||
|
|
||
| // If device is off, turn off all status LEDs except charging indicator | ||
| if (is_off) { | ||
| set_led_red(false); | ||
| set_led_blue(false); | ||
| return; | ||
| } | ||
| // If device is off, turn off all status LEDs | ||
| if (is_off) { | ||
| set_led_red(false); | ||
| set_led_blue(false); | ||
| return; | ||
| } | ||
|
|
||
| if (is_connected) { | ||
| set_led_blue(true); | ||
| set_led_red(false); | ||
| return; | ||
| } | ||
| if (is_connected) { | ||
| set_led_blue(true); | ||
| set_led_red(false); | ||
| return; | ||
| } | ||
|
|
||
| // Not connected - RED | ||
| if (!is_connected) { | ||
| set_led_red(true); | ||
| set_led_blue(false); | ||
| return; | ||
| // Not connected - RED | ||
| if (!is_connected) { | ||
| set_led_red(true); | ||
| set_led_blue(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in this function has become quite complex and introduces a potential behavioral change when the device is off.
is_offbehavior: Whenis_chargingandis_offare both true, the LED will now blink. The previous behavior was to show a solid green light. A blinking light might incorrectly suggest to the user that the device is on. It's better to handle theis_offcase explicitly to provide a clear and consistent user experience.- Code Complexity & Duplication: The logic to determine the connection status color (red/blue) is duplicated. The overall structure can be simplified to be more readable and maintainable.
I suggest refactoring the function to address these points. The suggestion below first handles the is_off case, and then simplifies the logic for when the device is on, removing code duplication.
static bool charge_blink_state = false;
// Handle device off state first: solid green if charging, otherwise all off.
if (is_off) {
set_led_green(is_charging);
set_led_red(false);
set_led_blue(false);
charge_blink_state = false; // Reset blink state for when device is turned on
return;
}
// Device is on.
if (is_charging) {
charge_blink_state = !charge_blink_state;
if (charge_blink_state) {
// Green phase of charging blink
set_led_green(true);
set_led_red(false);
set_led_blue(false);
return;
}
// For the other blink phase, fall through to show connection status color.
} else {
// Not charging, so reset blink state.
charge_blink_state = false;
}
// Show connection status (used for non-charging state and one phase of charging blink).
set_led_green(false);
if (is_connected) {
set_led_blue(true);
set_led_red(false);
} else {
set_led_red(true);
set_led_blue(false);
}
|
@aaravgarg is it ready, man? |
before:
now