-
-
Notifications
You must be signed in to change notification settings - Fork 982
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
alarm: Long press to stop alarm #2222
base: main
Are you sure you want to change the base?
Conversation
Build size and comparison to main:
|
5290eae
to
1959b3a
Compare
This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.
1959b3a
to
026527d
Compare
I haven't had a chance to test this with hardware since the ringing screen has been redesigned. Does the progress bar need to move at all? Maybe it could go between the time and the button in the blank space. Also if it looks a bit blocky (again I need to test), it will render more nicely if it is thinner since there will be fewer pixels to push. |
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.
Especially with the bigger button, I think this is a good addition. Thanks for the contribution! The only suggestion I have is calculating the bar progress more consistently and making sure it's super clear to the user that they have to hold the button
} | ||
|
||
void Alarm::Refresh() { | ||
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) { |
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.
xTaskGetTickCount() - stopBtnPressTime > pdMS_TO_TICKS(150)
is tolerant to tick count wraparound
Should we start showing the bar earlier than 150ms, as you need to hold the button to dismiss it. In fact, would it be better with no delay at all? A long press isn't super intuitive, so giving immediate feedback will make it clearer that holding it is needed IMO
|
||
void Alarm::Refresh() { | ||
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) { | ||
stopPosition += 15; |
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.
If we calculate elapsed ticks with xTaskGetTickCount() - stopBtnPressTime
, I think it'd be better to calculate the bar position using this. So if we want to hold for say 1 second, we can do something like elapsedTicks * 1000 / configTICK_RATE
to get the number of milliseconds the button has been held, and then draw the progress appropriately / stop alerting when more than 1000 milliseconds has passed.
This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.