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 |
|
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?
There was a problem hiding this comment.
Yes, this is redundant, but the builder fails when initializing the *timer screen as a direct clause of the case. Kind of baffled about this one, but I can only build successfully if I check that the current app is the Timer before I try to initialize the Timer screen. 🤷
| 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
There was a problem hiding this comment.
Without calling this, there's no count-up in the timer: it just stays at 00:00 while ringing. Not sure I want to skip this feature - I find it way too useful - and so you need to tell the Timer component what the expiry time is so that the GetTimeRemaining call returns the correct value (otherwise expired remains at 0 and nothing happens).
| 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)
There was a problem hiding this comment.
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)
Is it, though? That's what the xTimer interface is meant to do. This wrapper into a Timer component is only used for the Timer app. That said, I think this Timer class could still work elsewhere in the code as is, if there was a use case for it. If a new use case shows up, and a change is needed, we can do it then - not sure creating a super generic class is useful without knowing what use cases it's meant to address.
That's just my opinion though, and I may be entirely wrong (and I'm just having a hard time finding time to work on this these days - which is why I'm hesitant to get started on a change I probably won't finish).
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
|
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 |
vkareh
force-pushed
the
timer-ringing
branch
from
1ce2d7d to
8987d79
Compare
|
@vkareh this goes for a few of your other PRs as well: I see that the PRs are rebased regularly while review is pending, is there anything I could clarify with review feedback? Hope all's well, totally get it if you're busy :) |
No, I feel that's unnecessary complexity. Let's find a good default and hardcode it.
I'm willing to bump this to 15, 30, or 60 seconds if folks agree. |
Thanks @mark9064 - I'm quite busy these days, so I can only keep up with rebases and minimal changes. That said, if there's clear direction on how to address some of the nuanced change requests, I'm willing to put some time, but I'm saving most of my brain capacity for work, so I can't see myself going through the FreeRTOS API and debug undefined behaviour or things like that 🤯 A lot of your review suggestions sound very reasonable, but everything I've tried/tested points to "this is the only way to make it work, apart from reimplementing the xTimer interface in-app". I use this timer every single day, multiple times a day, in many configurations, all of them real life scenarios, and I have not had a single instance of it behaving any different than what I intended in the code. |
|
For what it's worth, this branch has been my daily driver for several months (thanks @vkareh for always keeping it rebased - you're the MVP). I've not run into any issues whatsoever, and I use the timer quite frequently for cooking, etc. 10 sec of vibration is usually sufficient for my needs, but I would not be opposed to a longer default, like 30. |
|
Loving this @vkareh, great job! One issue I'm having is that the buzzing for the expired time will go forever if the display falls asleep before it reaches 10 seconds, along with it counting past 60 seconds if the displays asleep (I can see it quickly reset the timer once I wake the display). This would be an issue if I started a timer and took my watch off, leaving it to vibrate until I got back. My guess is that the lv_task doesn't run Refresh while the display is sleeping. This isn't an issue while using the always on display. |
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.