Support using the filesystem as backend for settings subsystem#121
Support using the filesystem as backend for settings subsystem#121medarion wants to merge 3 commits intoedgehog-device-manager:masterfrom
Conversation
Signed-off-by: Martin Oemus <martin.oemus@seco.com>
43f8e8f to
27d90f6
Compare
Signed-off-by: Martin Oemus <martin.oemus@seco.com>
1abebf1 to
0b0e3cc
Compare
Signed-off-by: Martin Oemus <martin.oemus@seco.com>
133b64d to
c125265
Compare
sorru94
left a comment
There was a problem hiding this comment.
Rebase on master as we merged a fix to the formatting that was breaking the CI.
| const char *path = f.mp->mnt_point; | ||
| fs_close(&f); | ||
|
|
||
| struct fs_statvfs s; |
There was a problem hiding this comment.
Do not use single-character variables. Minimum is three chars according to our clang-tidy configuration.
| fs_file_t_init(&f); | ||
| if (fs_open(&f, CONFIG_SETTINGS_FILE_PATH, 0) != 0) { | ||
| return; | ||
| } | ||
| const char *path = f.mp->mnt_point; | ||
| fs_close(&f); |
There was a problem hiding this comment.
In general, this whole open close cycle intended to find the partition mount point should not be required.
Passing CONFIG_SETTINGS_FILE_PATH directly to fs_statvfs should work fine. I have not tested this myself, but looking at the codebase of Zephyr it seems to be used this way in multiple spots.
See, for example, the test test_file_statvfs.
|
To answer the question in the body of the PR, this one looks good as a quick fix to support when the settings use a file system. The choice of reporting only the free space within the edgehog_partition was due to the complexity of fetching at runtime the information on all other partitions. Edgehog can be added as a library to a project with an arbitrary number of partitions of different types. It results difficult to enumerate all partitions, inferring the type of each one, and then calculating how much free space they have. Even if we find a way to enumerate all the partitions present on the device and infer their type, we would still need to make sure there is no issue reading from them due to concurrent threads. An idea could be to shift the control to the user by accepting a list of partition references with an associated type (and probably an access control structure to avoid access conflicts). I have not looked into this myself, so I am not sure what the best approach would be. |
Currently the code assumes that the settings subsystem always uses NVS. If this is not the case, the storage usage calculation is wrong.
This PR
nvs.handnvs.cifCONFIG_SETTINGS_FILEis set, as it is not guaranteed thatedgehog_partitionorstorage_partitionexist in that caseI see a few options to further improve the PR:
CONFIG_EDGEHOG_DEVICE_USE_EDGEHOG_PARTITIONand the hardcoded partition selection and useDT_CHOSEN(zephyr_settings_partition)to deduce it directly from thezephyr,settings-partitiondevice tree propertyastarte_partitionwhich is currently hardcoded in the Astarte device codeWhat do you think?