Alarm persist to flash #1367
Alarm persist to flash #1367
Conversation
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.
Introduce `SaveAlarm()` member function which saves to file only if the alarm setting is changed. The `alarmChanged` boolean flag mimics the behavior of `settingsChanged` flag from `Settings.h` Also mark the `SaveAlarmToFile()` function const, as it doesn't change anything in the `AlarmController` object.
NeroBurner
force-pushed
the
alarm-persist-to-flash-neroburner
branch
from
ac410fa
to
3969a4a
Compare
| lfs_file_t alarmFile; | ||
| AlarmData alarmBuffer; | ||
|
|
||
| if (fs.FileOpen(&alarmFile, "/alarm.dat", LFS_O_RDONLY) != LFS_ERR_OK) { |
There was a problem hiding this comment.
When we add features that allow users to access the filesystem, we need to decide if the root of the filesystem will be generally accessible for the user, or if there'll be a "home directory". If the root will be accessible, system files must be placed in a directory.
I think the current structure used for the resources isn't really optimal if the root will be generally accessible. I think I'd prefer creating a safe "home directory" and limit general access to under that directory, which wouldn't require changes in this PR necessarily.
There was a problem hiding this comment.
Currently, the filesystem is used to store
- user settings
- BLE bonding info
- external resources since 1.11
The end users are not supposed to access those files directly : infinitime manages the settings/bonding data and the resources are uploaded following a specified procedure. But, they can, for example, customize the external resources if they want to (and if they know what they are doing).
What do you mean by "generally accessible" ? I don't think the users will ever need to create and manage their own file on the FS. This is at least not the case for now. Which files would you put in the "home directory"?
I'm OK to define a folder structure to keep the FS clean & tidy. For example:
- system : settings + bonding info
- resources : external resources (with subdir for fonts, pictures,...)
- apps : config and data files specific for each application that need persistance
There was a problem hiding this comment.
I imagine users would need to be able to access the filesystem to install apps, translations, files for viewing with a gallery app, maybe customization patches, it could be anything.
There was a problem hiding this comment.
But I guess those files will have to be stored at the exact location where InfiniTime and the apps will look for them, the users won't be able to upload those file anywhere they want in their home directory. For example, InfiniTime will look for translations in a specific folder and the gallery app will look for pictures in another specific folder.
So how will we specify which file must be stored in /home rather than in another "system" folder?
There was a problem hiding this comment.
It's getting a bit off topic, but I think we should never place anything in the user directory, so only user uploaded files would be there. The thing I'm concerned with here is the user having to navigate among system files. Only giving access to a user directory (by default) would be my preferred way to resolve the issue. If we instead allow root access by default, then I think there should be no files directly at root.
There was a problem hiding this comment.
I don't want the user to encounter system files.
Let's imagine adding files to the filesystem. On my phone there are directories to put your music, pictures, etc.. On the PineTime with root access, you would see alarm.dat, settings.dat, etc., which doesn't seem right to me.
There are two solutions to the problem, and we can even implement both.
One is not placing any files in the root and making sure the user doesn't mistakingly enter system directories.
Second is not allowing access to the root in the first place. I suppose this would be done by restricting the BLE FS API, but I'm thinking about the issue more conceptually.
There was a problem hiding this comment.
Yes, Ok, I understand. Currently, the users never interact directly with the FS. It's possible for them to replace a resource with a custom one if they want to, but that's not expected. But yes, in the future, with apps like the Gallery, they'll probably have more opportunities to use the filesystem.
Our current BLE FS API doesn't prevent anyone from accessing any file in the FS, and I don't think we should restrict the BLEFSAPI to a specific folder. Companion apps might want to provide an easy way to edit the settings, the alarm or even the bond info (or clear them). They also need to access the FS to upload the resources.
So yes, we can create folders for specific folders like I say in a previous comment (with an additional home folder for user customizable files, maybe), but I don't think we'll be able to prevent the users from accessing other folder than the home folder.
It is also possible for companion apps to implement UIs specific to some apps/files so that the user does not have to manually upload and update the file, this will be implemented by the companion app.
There was a problem hiding this comment.
What if we prepended all system directories with a dot and asked companion apps to hide directories beginning with a dot by default? I propose moving the alarm file to .system/alarm.dat.
There was a problem hiding this comment.
That's actually a good idea! Yep, I approve :)
There was a problem hiding this comment.
What should we do about the existing files and directories? @JF002
rebase of #1333 with additional
SaveAlarm()function as described belowSave 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
or a firmware update
Introduce
SaveAlarm()member function which saves to file only if thealarm setting is changed.
The
alarmChangedboolean flag mimics the behavior ofsettingsChangedflag from
Settings.hAlso mark the
SaveAlarmToFile()function const, as it doesn't changeanything in the
AlarmControllerobject.This PR needs InfiniTimeOrg/InfiniSim#67 to work on
InfiniSim