-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
base: main
Are you sure you want to change the base?
Conversation
Build size and comparison to main:
|
0033e6c
to
9136594
Compare
6bd64b5
to
ebc5b50
Compare
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 |
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.
I see why you changed this (count up after ringing), but we probably want to avoid relying on UB
src/components/timer/Timer.cpp
Outdated
return std::chrono::milliseconds(remainingTime * 1000 / configTICK_RATE_HZ); | ||
remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount(); | ||
} else { | ||
remainingTime = xTaskGetTickCount() - xTimerGetExpiryTime(timer); |
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 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.
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.
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?
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.
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
?
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.
Finally got around to fixing this.
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. |
4ba3e90
to
b3a6d74
Compare
9de08c1
to
6930303
Compare
if (currentApp != Apps::Timer) { | ||
LoadNewScreen(Apps::Timer, DisplayApp::FullRefreshDirections::Up); | ||
} | ||
// Once loaded, set the timer to ringing mode | ||
if (currentApp == Apps::Timer) { |
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.
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(); |
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.
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(); |
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.
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)
20f135e
to
185532b
Compare
8860e95
to
2ad96aa
Compare
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.
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. What do other users of InfiniTime think about this? P.S. I am not InfiniTime maintainer, I am InfiniTime user. |
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 |
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.