Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Settings Utility and example app #2228

Merged

Conversation

TimJTi
Copy link
Contributor

@TimJTi TimJTi commented Dec 11, 2023

Summary

Last year @fjpanag offered to donate some code to the repo. This is my (second) attempt at making his "settings" functions NuttX compatible along with an example app to check it out

Impact

None. This is new.

Testing

On my custom board (SAMA5D27C-D1G).

There may still be some style and/or NuttX-convention errors/inconsistencies of course. Also, a reviewer previously reported that the dump_cache function was not safe to call from a signal handler but neither I nor the original author can understand why?

@TimJTi TimJTi marked this pull request as ready for review December 11, 2023 15:00
system/settings/settings.c Show resolved Hide resolved
system/settings/settings.c Outdated Show resolved Hide resolved
system/settings/settings.c Outdated Show resolved Hide resolved
system/settings/settings.c Outdated Show resolved Hide resolved
system/settings/settings.c Outdated Show resolved Hide resolved
system/settings/settings.c Outdated Show resolved Hide resolved
system/settings/storage_eeprom.c Outdated Show resolved Hide resolved
system/settings/storage_eeprom.c Outdated Show resolved Hide resolved
system/settings/storage_eeprom.c Outdated Show resolved Hide resolved
include/system/storage.h Outdated Show resolved Hide resolved
@fjpanag
Copy link
Contributor

fjpanag commented Dec 12, 2023

@TimJTi If you need to reduce the memory use of the map, why don't you try to use an RLE or a KVL serializer?

@TimJTi
Copy link
Contributor Author

TimJTi commented Dec 12, 2023

@TimJTi If you need to reduce the memory use of the map, why don't you try to use an RLE or a KVL serializer?

@fjpanag

See all answers above.

As far as this suggestion is concerned, it wouldn't fit very well with the existing storage_binary function which assumes every setting is the full sizeof settings_t? It would move the EEPROM storage outside of the entire settings feature whereas what I have done does fit in, and works, albeit needing some changes? If it is an overall objection to a third storage type that is, when all said and done, very close to storage_bin, could it perhaps be a Kconfig option to MINIMISE_BINSIZE, with the necessary differences included in storage_bin?

Also, to use KLV or RLE the encoding has to be written/ported/added to NuttX too? Is there an overarching reason to move the changes, that I feel are beneficial for EEPROM devices, outside of the settings feature itself and to "offload" them to the user app?

@TimJTi TimJTi force-pushed the Add-Settings-Utility-and-example-app branch 2 times, most recently from 7d67d3d to d2d7e59 Compare December 14, 2023 14:55
examples/settings/Kconfig Outdated Show resolved Hide resolved
examples/settings/Kconfig Outdated Show resolved Hide resolved
examples/settings/settings_main.c Show resolved Hide resolved
@TimJTi TimJTi force-pushed the Add-Settings-Utility-and-example-app branch from d2d7e59 to 7b11ab0 Compare February 27, 2024 16:00
@TimJTi
Copy link
Contributor Author

TimJTi commented Feb 27, 2024

I have not been able to work for 6 weeks after emergency eye surgery and I have only just been able to revisit this.

I have made changes as per all comments and don't seem to have broken anything!

@TimJTi TimJTi force-pushed the Add-Settings-Utility-and-example-app branch from 7b11ab0 to e766aa3 Compare February 27, 2024 16:34
@xiaoxiang781216 xiaoxiang781216 merged commit 169beae into apache:master Mar 2, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants