system/settings: general code cleaning #3110

jeanthom · 2025-06-25T13:52:44Z

Summary

Remove duplicate checks, use switch/case whenever possible
Remove assertions just before if () condition checking the exact same thing
Replace if (condition) assert(0) with assert(condition)

Impact

No impact on users.

Testing

Ran the example "settings" app using sim:nsh NuttX 12.9.0 configuration.

Before:

NuttShell (NSH) NuttX-12.9.0
nsh> settings
Example of settings usage to file: /tmp/settings.bin and /tmp/settings.txt:--------------------------------------------------------------
No existing binary storage file found. Creating it.
No existing text storage file found. Creating it.
Retrieved settings value (v1) with value:default value
Trying to (re)create a setting that already exists (v1)
Retrieved setting type is: 4
Trying to change setting (v1) to integer type
Deliberate fail: settings change invalid: -13
Deliberate fail: non-existent setting requested. Error:-2
Trying to change setting (v1) from int to string: I'm a string
Creating a string settings value (s1):I'm a string
Retrieved string settings value (s1) with value:I'm a string
Changing setting to an IP value (s1) with value:192.168.100.1
Retrieved IP address settings value (s1) with value:0xc0a86401
syncing storages
exiting settings example app

After:

NuttShell (NSH) NuttX-12.9.0
nsh> settings
Example of settings usage to file: /tmp/settings.bin and /tmp/settings.txt:--------------------------------------------------------------
No existing binary storage file found. Creating it.
No existing text storage file found. Creating it.
Retrieved settings value (v1) with value:default value
Trying to (re)create a setting that already exists (v1)
Retrieved setting type is: 4
Trying to change setting (v1) to integer type
Deliberate fail: settings change invalid: -13
Deliberate fail: non-existent setting requested. Error:-2
Trying to change setting (v1) from int to string: I'm a string
Creating a string settings value (s1):I'm a string
Retrieved string settings value (s1) with value:I'm a string
Changing setting to an IP value (s1) with value:192.168.100.1
Retrieved IP address settings value (s1) with value:0xc0a86401
syncing storages
exiting settings example app

* Remove duplicate checks, use switch/case whenever possible * Remove assertions just before if () condition checking the exact same thing * Replace if (condition) assert(0) with assert(condition) Signed-off-by: Jean THOMAS <jean@lambdaconcept.com>

system/settings/settings.c

cederom

Thank you @jeanthom :-)

Please take a look at my notes in the code to make sure all will work as expected after change :-)
There is a slight behavioral change introduced for instance no asserts but error code return which seems okay :-)
How did you verify the change? Could you please provide testing logs from a real hardware before and after?

jeanthom · 2025-06-25T15:33:59Z

Hi @cederom and thanks for your review. I applied the required suggestions to the code. Regarding the behavioral change, I think it is sensible: I was always told asserts should be used to verify internal state of a program whereas error code should be preferred to notify an issue with user input.

Regarding tests: ran the example "settings" app using sim:nsh NuttX configuration.

Before:

NuttShell (NSH) NuttX-12.9.0
nsh> settings
Example of settings usage to file: /tmp/settings.bin and /tmp/settings.txt:--------------------------------------------------------------
No existing binary storage file found. Creating it.
No existing text storage file found. Creating it.
Retrieved settings value (v1) with value:default value
Trying to (re)create a setting that already exists (v1)
Retrieved setting type is: 4
Trying to change setting (v1) to integer type
Deliberate fail: settings change invalid: -13
Deliberate fail: non-existent setting requested. Error:-2
Trying to change setting (v1) from int to string: I'm a string
Creating a string settings value (s1):I'm a string
Retrieved string settings value (s1) with value:I'm a string
Changing setting to an IP value (s1) with value:192.168.100.1
Retrieved IP address settings value (s1) with value:0xc0a86401
syncing storages
exiting settings example app

After:

NuttShell (NSH) NuttX-12.9.0
nsh> settings
Example of settings usage to file: /tmp/settings.bin and /tmp/settings.txt:--------------------------------------------------------------
No existing binary storage file found. Creating it.
No existing text storage file found. Creating it.
Retrieved settings value (v1) with value:default value
Trying to (re)create a setting that already exists (v1)
Retrieved setting type is: 4
Trying to change setting (v1) to integer type
Deliberate fail: settings change invalid: -13
Deliberate fail: non-existent setting requested. Error:-2
Trying to change setting (v1) from int to string: I'm a string
Creating a string settings value (s1):I'm a string
Retrieved string settings value (s1) with value:I'm a string
Changing setting to an IP value (s1) with value:192.168.100.1
Retrieved IP address settings value (s1) with value:0xc0a86401
syncing storages
exiting settings example app

acassis · 2025-06-25T16:59:39Z

@TimJTi since you are the original author, please take a look too!

TimJTi · 2025-06-25T17:11:36Z

@TimJTi since you are the original author, please take a look too!

Actually - I didn't write it but did "port" it to NuttX and am using it regularly. Once it is reviewed by others and "ready to merge" give me the heads up and all merge the PR in to my code and make sure it works OK for me still. Or are we there now?

The original author was very keen I messed with as little as possible when I was making it "suitable for NuttX", and got me to "undo" things I'd changed (perhaps including better use of select/case rather than if/elseif/else etc!!! Can't please everybodt all the time LOL.

As far as assert vs error code: having both means that if NDEBUG is set, disabling asserts, there are still error messages, but the program doesn't "crash" as such.

I have seen this used in quite a few places and it seemed a good idea at the time, and is "benign"; but if its frowned upon here, so be it - although nobody complained during the reviews when I submitted the PR!!

cederom

Thanks @jeanthom for all the updates and feedback, just one bracket inside case and we are ready to go :-)

TimJTi · 2025-06-26T12:11:55Z

@acassis @jerpelea I have been asked to review this, which I would be delighted to do!

Can either of you - or anyone else - suggest an efficient way to do this with some tips, so I don't waste tons of time doing things the hard way?

cederom

Thank you @jeanthom :-)

cederom · 2025-06-26T14:36:18Z

@TimJTi: @acassis @jerpelea I have been asked to review this, which I would be delighted to do!

Can either of you - or anyone else - suggest an efficient way to do this with some tips, so I don't waste tons of time doing things the hard way?

Tim you can clone the source repo switch to branch that constitutes this PR build and see if that works for you :-) If you have existing application that uses settings you may want to build that too and see if anything is broken and works as expected - all should work fine with your existing application. Thank you :-)

I have dedicated nuttx/pr/nuttx.git and nuttx/pr/nuttx-apps.git clones where I add remotes to be tested, here is an example:

mkdir nuttx-pr - create pr test workbench.
cd nuttx-pr - go to pr test workbench.
git clone https://github.com/apache/nuttx.git nuttx - clone nuttx repo.
git clone https://github.com/apache/nuttx-apps.git - clone nuttx-apps repo.
cd nuttx-apps - go to cloned apps repo.
git remote add lambdaconcept https://github.com/lambdaconcept/nuttx-apps.git - add remote where pr source resides.
git fetch --all - get changes from all remotes.
git checkout lambdaconcept/system-settings-cleanup - switch to branch that sources this PR.
you are ready to test :-)

TimJTi · 2025-06-26T14:43:31Z

@TimJTi: @acassis @jerpelea I have been asked to review this, which I would be delighted to do!
Can either of you - or anyone else - suggest an efficient way to do this with some tips, so I don't waste tons of time doing things the hard way?

Tim you can clone the source repo switch to branch that constitutes this PR build and see if that works for you :-) If you have existing application that uses settings you may want to build that too and see if anything is broken and works as expected - all should work fine with your existing application. Thank you :-)

I have dedicated nuttx/pr/nuttx.git and nuttx/pr/nuttx-apps.git clones where I add remotes to be tested, here is an example:

mkdir nuttx-pr - create pr test workbench.

cd nuttx-pr - go to pr test workbench.

git clone https://github.com/apache/nuttx.git nuttx - clone nuttx repo.

git clone https://github.com/apache/nuttx-apps.git - clone nuttx-apps repo.

cd nuttx-apps - go to cloned apps repo.

git remote add lambdaconcept https://github.com/lambdaconcept/nuttx-apps.git - add remote where pr source resides.

git fetch --all - get changes from all remotes.

git checkout lambdaconcept/system-settings-cleanup - switch to branch that sources this PR.

you are ready to test :-)

Thanks @cederom

And I see nicely "quoted" added/deleted lines that are then commented on (and in the past I have seen suggested changes that can be accepted here). Is that done manually from the git change log or is there a "trick" to that too?

cederom · 2025-06-26T14:50:41Z

@TimJTi: And I see nicely "quoted" added/deleted lines that are then commented on (and in the past I have seen suggested changes that can be accepted here). Is that done manually from the git change log or is there a "trick" to that too?

If you are talking about discussion here on GitHub regarding specific lines changed then this is part of the PR processing interface on GitHub website :-) You go to "files changed" top tab under PR, then when you point to a line number either on the - or + side you can mark specific line or lines range where comments can be added, these comments then show up as discussion here :-)

acassis · 2025-06-26T15:05:42Z

@TimJTi normally I do this way: https://acassis.wordpress.com/2022/02/24/how-to-test-a-pr-before-it-get-integrated-on-nuttx/

TimJTi

Much neater than the original that I worked from! Nice work 👍

Nothing broken that I've found with brief testing in my project- I can raise a new issue or PR if I do find anything not right in the future but it looks to be good to go.

acassis · 2025-06-26T16:23:02Z

Thank you @jeanthom !

github-actions bot added Area: System Size: M labels Jun 25, 2025