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

alarm: Long press to stop alarm #2222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vkareh
Copy link
Contributor

@vkareh vkareh commented Jan 9, 2025

This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.

InfiniSim_2025-01-10_121345

Copy link

github-actions bot commented Jan 9, 2025

Build size and comparison to main:

Section Size Difference
text 373264B 256B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@mark9064 mark9064 added the enhancement Enhancement to an existing app/feature label Jan 10, 2025
@vkareh vkareh force-pushed the alarm-long-press branch 3 times, most recently from 5290eae to 1959b3a Compare January 10, 2025 20:19
This change prevents accidentally turning off alarm by ensuring that
there is a deliberate long press, similar to resetting the Timer app.
@mark9064
Copy link
Member

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.

Copy link
Member

@mark9064 mark9064 left a 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)) {
Copy link
Member

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;
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants