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

Support NVS image generation from CMake (IDFGH-10702) #11926

Closed
wants to merge 3 commits into from
Closed

Support NVS image generation from CMake (IDFGH-10702) #11926

wants to merge 3 commits into from

Conversation

higaski
Copy link
Contributor

@higaski higaski commented Jul 21, 2023

In the spirit of spiffs_create_partition_image I thought it might be a good idea to create a similar function for NVS storage. In my particular case it releases me from the burden to explicitly check in code if certain settings are either

  1. initialized at all or
  2. lie within certain bounds

The API is strongly based on the SPIFFS variant. Users simply pass the partition, the CSV file containing the entries and an optional FLASH_IN_PROJECT parameter to add a dependency to idf.py flash.

nvs_create_partition_image(nvs ../sample_val.csv FLASH_IN_PROJECT)

I've used the sample_val.csv file to test this in an example here.

I would also write the documentation for it, but ask for guidelines where the relevant part is desired.

/edit
The english documentation that is, obviously. 😄

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jul 21, 2023
@github-actions github-actions bot changed the title Support NVS image generation from CMake Support NVS image generation from CMake (IDFGH-10702) Jul 21, 2023
@igrr
Copy link
Member

igrr commented Jul 21, 2023

Thanks! Could you take a look at another PR #11785?

@higaski
Copy link
Contributor Author

higaski commented Jul 21, 2023

Thanks! Could you take a look at another PR #11785?

Well, looks almost identical although you cant pass a version there.

I now also wonder if all the encryption options should be considered...?

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Jul 31, 2023
@adokitkat
Copy link
Collaborator

adokitkat commented Jul 31, 2023

Hello. Thank you for the contribution! Your PR will be used because it is based on our already existing APIs, but I will also add @nebkat from #11785 as a co-author since his PR was created earlier and it is very similar.

@adokitkat
Copy link
Collaborator

sha=1827a968c29b3dbcc30ede3b0a3082b48081f2af

@adokitkat adokitkat added the PR-Sync-Merge Pull request sync as merge commit label Jul 31, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Selected for Development Issue is selected for development labels Jul 31, 2023
@higaski
Copy link
Contributor Author

higaski commented Jul 31, 2023

Hello. Thank you for the contribution! Your PR will be used because it is based on our already existing APIs, but I will also add @nebkat from #11785 as a co-author since his PR was created earlier and it is very similar.

Glad the PR got accepted. What about documentation though?

I've thought about adding some below the NVS Partition Generator Utility entry.

NVS Partition Generator Utility
-------------------------------
This utility helps generate NVS partition binary files which can be flashed separately on a dedicated partition via a flashing utility. Key-value pairs to be flashed onto the partition can be provided via a CSV file. For more details, please refer to :doc:`nvs_partition_gen`.

@nebkat
Copy link
Contributor

nebkat commented Jul 31, 2023

Apologies for not continuing my PR, have been on vacation.

Either PR is fine of course, I would just mention - I believe the add_custom_command instead of (or in in addition to) add_custom_target allows the image to be built only when needed due to the OUTPUT clause.

In SPIFFS generation we have to create the image every time as there is no way to have a dependency on an entire directory, but that is not the case here.

It is very quick either way so not a major problem, but nice to avoid additional build steps whenever possible.

@higaski
Copy link
Contributor Author

higaski commented Jul 31, 2023

Either PR is fine of course, I would just mention - I believe the add_custom_command instead of (or in in addition to) add_custom_target allows the image to be built only when needed due to the OUTPUT clause.

You're right, I've copied your version which uses add_custom_command. Not only is it one build step less, it also outputs less noise on the command line, specially when doing incremental builds with otherwise minor changes.

I've also added a some docs.

@nebkat
Copy link
Contributor

nebkat commented Jul 31, 2023

Thanks @higaski.

For docs: if FLASH_IN_PROJECT is not specified you can flash with ${partition}-flash, no need for a custom build target.

@higaski
Copy link
Contributor Author

higaski commented Aug 1, 2023

Thanks @higaski.

For docs: if FLASH_IN_PROJECT is not specified you can flash with ${partition}-flash, no need for a custom build target.

Thanks, updated.

@adokitkat
Copy link
Collaborator

sha=0edf332062a2254e1aaa4f39158fca4c5bb8a110

@adokitkat adokitkat added the PR-Sync-Update Pull request sync fetch new changes label Aug 1, 2023
@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed and removed Status: In Progress Work is in progress labels Aug 1, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Aug 14, 2023
@higaski higaski deleted the nvs_create_partition_image branch August 18, 2023 11:01
@adokitkat adokitkat added Resolution: Done Issue is done internally and removed Resolution: NA Issue resolution is unavailable labels Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit PR-Sync-Update Pull request sync fetch new changes Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants