timer: Add ringing and counter #1971
Conversation
|
Build size and comparison to main:
|
FintasticMan
added
enhancement
vkareh
force-pushed
the
timer-ringing
branch
2 times, most recently
from
0033e6c to
9136594
Compare
vkareh
force-pushed
the
timer-ringing
branch
4 times, most recently
from
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 |
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.
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.
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.
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.
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. |
vkareh
force-pushed
the
timer-ringing
branch
2 times, most recently
from
4ba3e90 to
b3a6d74
Compare
vkareh
force-pushed
the
timer-ringing
branch
4 times, most recently
from
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.
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.
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.
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
force-pushed
the
timer-ringing
branch
2 times, most recently
from
20f135e to
185532b
Compare
vkareh
force-pushed
the
timer-ringing
branch
from
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.