InfiniTimeOrg
AlarmController: Add saving alarm time to file
Save the set alarm time to the SPI NOR flash, so it does not reset to the default value when the watch resets, e.g. due to watchdog timeout.
Fixes #1330
Size impact: +248 (400332 -> 400580)
- [x] Fix dismissing a ringing alarm
- [x] Test on PineTime
👍 for implementing this so fast. Did you test it? Should be easy to reboot once and check.
Yes, both in the simulator and on the PineTime (that's when I caught the typo fixed in the first force-push).
I assumed it was mandatory for a PR (that is not marked as draft) to be tested :)
If you want to test it too, do not forget to verify the firmware before you reboot. I might have had to flash twice…
If you want to test it too, do not forget to verify the firmware before you reboot. I might have had to flash twice…
Lol, this makes it quite dangerous to test those kind of features, right? At least, you are not tinkering with Bluetooth.
Since this was an issue in another issue: did you see my comments specific lines?
If not, tldr: you can use the data array for everything and omit the attributes. Also, pack activation and recurrence status also in the data struct because the are important to recover an active alarm.
Also, #1278 gets rid of log-functions. I assume that you should follow that.
Lol, this makes it quite dangerous to test those kind of features, right? At least, you are not tinkering with Bluetooth.
Not sure what you mean by dangerous. All that happened was that I first forgot to verify before I rebooted the watch, so the bootloader dutifully reverted to the previous firmware and I had to wait another 10 minutes while flashing again (via BLE DFU) for the next try.
Since this was an issue in another issue: did you see my comments specific lines?
Did you comment on the commit somewhere? I didn't get any notifications besides the ones for your comments in this thread and can't find any comments elsewhere, sorry.
If not, tldr: you can use the data array for everything and omit the attributes. Also, pack activation and recurrence status also in the data struct because the are important to recover an active alarm.
Good point, not sure how I missed that on the first version. I now added saving the recurrence and the enabled state along with the alarm time. The saved alarm is properly scheduled again after being loaded after boot if it was enabled
Also, #1278 gets rid of log-functions. I assume that you should follow that.
IMHO that is the wrong way to go and I commented there. Should there be consensus to merge that PR however, I will remove the logging in this PR.
Not sure what you mean by dangerous.
Rebooting to the last verified image is a nice fallback, when something goes wrong. But when testing features requires a reboot one has to verify the test firmware and loses that fallback.
Did you comment on the commit somewhere?
Apparently I used that feature wrong. Sorry for the confusion.
@minacode Verifying a firmware you're currently testing isn't as dangerous as it sounds at first. This is because if there is some issue with it, you can press and hold the button to reboot until you see the pinecone turn blue. This will rollback to the good now-secondary firmware even after you have validated the bad firmware. Source: about-software.md
Adding the enabled state to persistence has made dismissing an alarm glitchy, I'll investigate more and fix later when I'm at my PC. Persistence works, but when the alarm rings, you have to exit the alarm app by long pressing the button as a workaround, as swiping down hangs the app.
The problems dismissing the ringing alarm should be fixed now; I'll mark the tasks in the first post as done once the fix is tested and confirmed working.
The idea about saving only on toggle and on changes didn't let me go. So I implemented the behavior and tested the firmware sizes to see the impact of my changes. It surprised me, but my desired behavior didn't increase the size anymore
text data bss dec hex filename
399524 940 53400 453864 6ece8 pinetime-app-1.10.0.out - c853681d Reduce duplication in Twos (#1274)
399756 940 53408 454104 6edd8 pinetime-app-1.10.0.out - 9ece9e3c AlarmController: Code style fixes
399788 940 53408 454136 6edf8 pinetime-app-1.10.0.out - ce296ece Alarm: Only set time when enabling alarm and when exiting ce296ece8335b31fe014d9ec0098689647172fea
399788 940 53408 454136 6edf8 pinetime-app-1.10.0.out - neroburner-change, save only on enable toggle and change
Opened a PR to make it easy to get those changes into your branch: https://github.com/ght/InfiniTime/pull/1
@ght please review
@NeroBurner Thanks for trying it out! Cool to see that it does not necessarily increase binary size. This approach however has an API change I don't like: now the AlarmController no longer saves the alarm when data could be lost otherwise, but has to be told to do so explicitly by the caller. And not even consistently: in case of SetAlarmTime and SetRecurrence the caller has to remember to manually SaveAlarm, but in case of ScheduleAlarm, DisableAlarm, and StopAlerting it is saved automatically.
This gives me hope that it's doable without a size increase while keeping a clean API though. I'll look into it again too.
I really do not want the caller to have to remember to save. But the alternative is that the caller has to remember not to save unnecessarily, to avoid wearing out the flash.
What do you think of adding a timer to the alarm controller? Instead of committing every change to flash immediately, the timer is set to count down e.g. 10s or even just 5s and writing to flash only happens when the timer fires. That way it doesn't matter if the alarm app calls SetAlarmTime for every change, e.g. when + is held down to spin the minute up by 30.
What real-life problem do you want to solve? The app saving once in the destructor should be enough for everything except when the app is never left. There are two scenarios where this happens:
- The watch crashes while the app is still open. This should leave the user with the desire to check if everything went well and open the app again. I guess most people wouldn't trust an alarm that crashed in front of their eyes.
- The user did not leave the app and the watch went to sleep with the app still open.
This is the interesting part. I propose a callback function
OnSleepthat every app has to implement (possibly empty and inherited) and that is called by the DisplayApp when the watch goes to sleep. We could then save there once. I believe that there are or will be more apps that would profit from such a callback.
Persistence is a feature that will come for more and more apps. And this behavior should be as standardized as possible or every app will tinker on basically the same problem.
The SaveAlarm function is just like the SaveSettings function in Settings.h. With the modification, that important changes (like alarm on/off) are saved immediately (as you've deemed important earlier https://github.com/InfiniTimeOrg/InfiniTime/pull/1333#discussion_r975739665 ). Everything else (like time and if it is repeating) is saved once the Alarm Screen is closed
SaveAlarm can't be overused, because it only does work if one of the settings did change (again just like done in Settings.h). This is a simple and consistent API change in my honest opinion.
Ok, I like this behavior. We just have to keep in mind that every value change has to disable the alarm. Maybe this should be documented somewhere.
Opened a rebased PR https://github.com/InfiniTimeOrg/InfiniTime/pull/1367 with simplified SaveAlarm() functionality.
No need to work around bugs in the same firmware with saving the alarm. Instead such bugs should be fixed. This lets us keep the remaining code more simple
@ght I hope it is ok I hijack your PR, trying to champion your idea to get it into the firmware as soon as possible
@NeroBurner Ok to merge this? I think those changes require changes in InfiniSim too ?
