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

timer: Add ringing and counter #1971

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

Conversation

vkareh
Copy link
Contributor

@vkareh vkareh commented Jan 15, 2024

The timer app currently issues a short buzz once and then disappears. There is no trace left that the timer finished or how long ago. This change makes the motor start ringing and presents a timer counter. The motor will continue the ringing pattern for 10 seconds and the timer counter will finally reset after 60 seconds.

Copy link

github-actions bot commented Jan 15, 2024

Build size and comparison to main:

Section Size Difference
text 374864B 256B
data 948B 0B
bss 63512B 8B

@FintasticMan FintasticMan added enhancement Enhancement to an existing app/feature UI/UX User interface/User experience labels Jan 17, 2024
@vkareh vkareh force-pushed the timer-ringing branch 2 times, most recently from 0033e6c to 9136594 Compare January 24, 2024 15:35
@vkareh vkareh force-pushed the timer-ringing branch 4 times, most recently from 6bd64b5 to ebc5b50 Compare February 19, 2024 01:11
@mark9064
Copy link
Member

Works great, huge usability improvement for the timer. Been running this for a while and it makes me a more consistent cook for sure!

I think it would be nice if the alarm and timer had different vibration patterns, but vibration needs to be overhauled anyway so this is good to go for now IMO

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.

I see why you changed this (count up after ringing), but we probably want to avoid relying on UB

return std::chrono::milliseconds(remainingTime * 1000 / configTICK_RATE_HZ);
remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount();
} else {
remainingTime = xTaskGetTickCount() - xTimerGetExpiryTime(timer);
Copy link
Member

Choose a reason for hiding this comment

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

If the timer is not running, xTimerGetExpiryTime is undefined according to the docs.

If the timer is running then the time in ticks at which the timer will next expire is returned. If the timer is not running then the return value is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh interesting. The reason it has worked for me so far is probably because RTOS hasn't allocated data to that space and so the subtraction yields the expected value.

All it takes is another xTask running to mess the timer count-up...

I see three possible paths forward then:

  • Drop the count-up feature (and show some other text, etc.)
  • Implement a counter at the application level to not rely on the xTimer
  • Change the timer application so that the ringing is based on math, but the xTimer remains active until the user dismisses it. I think this is a no-go, as it adds a lot more unnecessary complexity.

I'm considering just dropping the count-up feature, since a count-up that resets at 60 seconds is only marginally useful (and bumping that to something like 10 minutes sounds unnecessary, but I may be wrong). But I'm open to implementing it at the application level if folks are interested in this.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'm a big fan of the counting up and I'd love to see it stay. Could record the tick at which the timer expires alongside the timer and then do (current tick - expiry tick) / tick rate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got around to fixing this.

@rnwgnr
Copy link

rnwgnr commented Aug 15, 2024

Tested this today while cooking and this is indeed a great usability improvement for the timer.

Until now i missed a lot of the alarms, the vibration is just too short. Great work.

@vkareh vkareh force-pushed the timer-ringing branch 2 times, most recently from 4ba3e90 to b3a6d74 Compare August 22, 2024 15:53
@vkareh vkareh force-pushed the timer-ringing branch 4 times, most recently from 9de08c1 to 6930303 Compare September 23, 2024 19:00
if (currentApp != Apps::Timer) {
LoadNewScreen(Apps::Timer, DisplayApp::FullRefreshDirections::Up);
}
// Once loaded, set the timer to ringing mode
if (currentApp == Apps::Timer) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this check redundant as timer is now loaded before?

secondCounter.HideControls();
lv_label_set_text_static(txtPlayPause, "Reset");
lv_obj_set_style_local_bg_color(btnPlayPause, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_RED);
timer.SetExpiredTime();
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I think this is still UB as the timer is no longer running at this point

if (IsRunning()) {
TickType_t remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount();
return std::chrono::milliseconds(remainingTime * 1000 / configTICK_RATE_HZ);
remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount();
Copy link
Member

Choose a reason for hiding this comment

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

On further reflection, we shouldn't change the semantics of this method

The Timer component is meant to be a generic component that multiple applications can be use (it should be just a thin wrapper around FreeRTOS timers)

One idea:

  • When setting a timer, the expiry tick time is stored in this class
  • New method GetTimeSinceExpired which returns xtaskgettickcount - expiry, or 0 (if running or stopped)
  • This method remains pretty much unchanged
  • Inside the timer screen we calculate the value as GetTimeRemaining if the timer is running, else with GetTimeSinceExpired

The only part of this I don't really like is the semantics of GetTimeSinceExpired when it hasn't expired yet or has been stopped, and also GetTimeRemaining when stopped or expired

Another idea:
Method GetTimerStatus which returns a variant of

  • Stopped
  • Running: time to expiry
  • Expired: time since expiry

This way semantics are always clear

Curious to know what you're thinking, any of these make sense? Not sure I've got any perfect solutions here, though I quite like the second one so far (haven't thought about either for too long)

@vkareh vkareh force-pushed the timer-ringing branch 2 times, most recently from 20f135e to 185532b Compare October 28, 2024 14:46
The timer app issues a short buzz once and then disappears. There is no
trace left that the timer finished or how long ago. This change makes
the motor start ringing and presents a timer counter.

The timer stops buzzing after 10 seconds, and finally resets after
1 minute.
@DavisNT
Copy link

DavisNT commented Dec 5, 2024

There are many cases when 10 seconds of vibration might not be very noticeable (e.g. when playing sports games, using some vehicles, doing woodworking etc.). Also attention is an important key factor (it is much easier to notice an alarm when all the attention is on testing that alarm; the same alarm might go unnoticed if attention is focused on something else).

Could it be possible to introduce a setting which would allow to select vibration length (e.g. 10 seconds, 60 seconds and infinite) for Timer and Alarm Clock?

Also, I think that timers, alarm clocks and similar reminders should be very reliable, noticeable and persistent, because consequences of reminders failing to go off or going off unnoticed are much worse than the consequences of reminders always needing manual action to silence them.
I.e. I think that timer and alarm clock should continue to vibrate until the button is pressed, otherwise there will be cases when people miss important things due to missing the alarms.

What do other users of InfiniTime think about this?

P.S. I am not InfiniTime maintainer, I am InfiniTime user.

@marigoldfish
Copy link

I like the idea of a settings menu where we can choose between different vibration durations and "until a button is pressed." I'd probably set mine to something like 30 seconds. That being said, the 10-second ring is still a lot more useful to me than the single buzz. This PR is at the top of my wishlist, for sure

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 UI/UX User interface/User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants