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
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.
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.