AlarmController: Add saving alarm time to file #1333

ght · 2022-09-14T23:24:31Z

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)

Fix dismissing a ringing alarm
Test on PineTime

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. Size impact: +204 (400332 -> 400536)

minacode · 2022-09-15T09:09:26Z

👍 for implementing this so fast. Did you test it? Should be easy to reboot once and check.

ght · 2022-09-15T11:18:50Z

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…

minacode · 2022-09-15T17:38:59Z

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.

minacode · 2022-09-15T17:41:58Z

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.

minacode · 2022-09-15T22:26:37Z

Also, #1278 gets rid of log-functions. I assume that you should follow that.

ght · 2022-09-16T00:11:56Z

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.

minacode · 2022-09-16T08:01:04Z

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.

toastom · 2022-09-17T17:59:29Z

@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

ght · 2022-09-17T18:58:22Z

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.

Also reverts the attempt at deferring saving the alarm data for reduced flash writes, in order to reduce complexity. Split `AlarmController.State()` into `.IsAlerting()` and `.IsEnabled()`, since the two properties are conceptually independent and it makes code much cleaner: ```diff - if (alarmController.State() == Controllers::AlarmController::AlarmState::Alerting) { ... + if (alarmController.IsAlerting()) { ... - if (alarmController.State() == AlarmController::AlarmState::Set) { ... + if (alarmController.IsEnabled()) { ... ``` See the removed `switch` for another example. Size impact: +44 (400536 -> 400580)

ght · 2022-09-18T01:12:52Z

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.

@ght

Support for InfiniTimeOrg/InfiniTime#1333 by @ght The PR adds saving and restoring alarm state to the SPI NOR flash. So the AlarmController needs the FileSystem-controller to work.

NeroBurner

I like the persistent Alarm implementation, thanks for the PR

src/displayapp/screens/Alarm.cpp

src/components/alarm/AlarmController.cpp

src/components/alarm/AlarmController.h

This avoids writing the time to flash for every single change. Size impact: +16 (400580 -> 400596)

NeroBurner · 2022-09-26T20:42:21Z

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: ght#1

@ght please review

ght · 2022-09-27T09:37:05Z

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

minacode · 2022-09-27T13:24:18Z

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

NeroBurner · 2022-09-28T06:24:51Z

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 #1333 (comment) ). 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.

minacode · 2022-09-28T10:27:34Z

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.

@ght

Support for InfiniTimeOrg/InfiniTime#1333 by @ght The PR adds saving and restoring alarm state to the SPI NOR flash. So the AlarmController needs the FileSystem-controller to work.

NeroBurner · 2022-10-12T20:33:35Z

Opened a rebased PR #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

NeroBurner · 2022-10-12T20:35:39Z

@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

JF002 · 2023-01-05T14:12:25Z

@NeroBurner Ok to merge this? I think those changes require changes in InfiniSim too ?

@ght

… to file Support for InfiniTimeOrg/InfiniTime#1367 which is a rebase/improvement of InfiniTimeOrg/InfiniTime#1333 by @ght The PR adds saving and restoring alarm state to the SPI NOR flash. So the AlarmController needs the FileSystem-controller to work. To support both the current `main` branch and the "save-alarm-to-file" PR pass the Filesystem controller conditionally, only if `Controller::FS` is mentioned in the `AlarmController.h` file

@ght

… to file Support for InfiniTimeOrg/InfiniTime#1367 which is a rebase/improvement of InfiniTimeOrg/InfiniTime#1333 by @ght The PR adds saving and restoring alarm state to the SPI NOR flash. So the AlarmController needs the FileSystem-controller to work. To support both the current `main` branch and the "save-alarm-to-file" PR pass the Filesystem controller conditionally, only if `Controller::FS` is mentioned in the `AlarmController.h` file

NeroBurner · 2024-09-19T20:05:46Z

closing in favor of #1367

ght force-pushed the alarm-persist-to-flash branch from 872c719 to d2341c1 Compare September 14, 2022 23:27

AlarmController: Add saving alarm time to file

9574ea5

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. Size impact: +204 (400332 -> 400536)

ght force-pushed the alarm-persist-to-flash branch from d2341c1 to 9574ea5 Compare September 14, 2022 23:30

ght force-pushed the alarm-persist-to-flash branch from 30fb6d4 to 2af96ce Compare September 16, 2022 00:10

ght force-pushed the alarm-persist-to-flash branch from 2af96ce to 6fed68a Compare September 18, 2022 00:59

NeroBurner added the feature request label Sep 19, 2022

NeroBurner added this to the 1.11.0 milestone Sep 19, 2022

NeroBurner mentioned this pull request Sep 19, 2022

Conditionally pass Controllers::FS to AlarmController to support save to file InfiniTimeOrg/InfiniSim#67

Merged

NeroBurner requested changes Sep 19, 2022

View reviewed changes

ght added 2 commits September 20, 2022 16:06

AlarmController: Code style fixes

9ece9e3

Alarm: Only set time when enabling alarm and when exiting

ce296ec

This avoids writing the time to flash for every single change. Size impact: +16 (400580 -> 400596)

ght requested a review from NeroBurner September 24, 2022 20:25

This was referenced Sep 26, 2022

Alarm persist to flash neroburner #1345

Closed

Alarm persist to flash neroburner ght/InfiniTime#1

Open

NeroBurner mentioned this pull request Oct 12, 2022

Alarm persist to flash #1367

Merged

NeroBurner modified the milestones: 1.11.0, 1.12.0 Oct 16, 2022

JF002 removed this from the 1.12.0 milestone Apr 2, 2023

kvaellning mentioned this pull request Sep 15, 2023

Multiple Alarms #950

Open

1 task

NeroBurner closed this Sep 19, 2024

Uh oh!

AlarmController: Add saving alarm time to file #1333

AlarmController: Add saving alarm time to file #1333

Uh oh!

Conversation

ght commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minacode commented Sep 15, 2022

Uh oh!

ght commented Sep 15, 2022

Uh oh!

minacode commented Sep 15, 2022

Uh oh!

minacode commented Sep 15, 2022

Uh oh!

minacode commented Sep 15, 2022

Uh oh!

ght commented Sep 16, 2022

Uh oh!

minacode commented Sep 16, 2022

Uh oh!

toastom commented Sep 17, 2022

Uh oh!

ght commented Sep 17, 2022

Uh oh!

ght commented Sep 18, 2022

Uh oh!

NeroBurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NeroBurner commented Sep 26, 2022

Uh oh!

ght commented Sep 27, 2022

Uh oh!

minacode commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NeroBurner commented Sep 28, 2022

Uh oh!

minacode commented Sep 28, 2022

Uh oh!

NeroBurner commented Oct 12, 2022

Uh oh!

NeroBurner commented Oct 12, 2022

Uh oh!

JF002 commented Jan 5, 2023

Uh oh!

NeroBurner commented Sep 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ght commented Sep 14, 2022 •

edited

Loading

minacode commented Sep 27, 2022 •

edited

Loading