-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Include nvs_commit() on three methods #5309
Conversation
In [Preferences.cpp](https://github.com/espressif/arduino-esp32/blob/master/libraries/Preferences/src/Preferences.cpp), the functions: ``` Preferences::clear() Preferences::remove() Preferences::end() ``` should be revised to include a call to `nvs_commit()` as required per [Non-volatile storage library](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/storage/nvs_flash.html) when using ``` nvs_erase_all() nvs_erase_key() nvs_close() ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @beamholder, it is useful! Some minor requests below...
@igrr As I'm new two this whole Github/ Arduino-ESP32 thing, just wondering if this PR is now all good or is there is something else I need to do before it is merged? Thanks. |
Hi. Has this been merged into the master yet? Anything missing if not? Still figuring out the whole GitHub thing.
On Jul 2, 2021, at 12:26 AM, Ivan Grokhotkov ***@***.***> wrote:
@igrr requested changes on this pull request.
Thanks for the PR @beamholder <https://github.com/beamholder>, it is useful! Some minor requests below...
In libraries/Preferences/src/Preferences.cpp <#5309 (comment)>:
@@ -53,10 +53,14 @@ bool Preferences::begin(const char * name, bool readOnly, const char* partition_
return true;
}
-void Preferences::end(){
+void Preferences::end(){ // modified to add an nvs_commit()
The comment isn't necessary, as the file change history is visible in git log or git blame, and any commit that has modified a file can be found and reverted if needed. The comments describing the changes (// added by XXX 06/2021) only distract from the purpose of the code.
In libraries/Preferences/src/Preferences.cpp <#5309 (comment)>:
@@ -74,14 +78,20 @@ bool Preferences::clear(){
log_e("nvs_erase_all fail: %s", nvs_error(err));
return false;
}
- return true;
+ // return true; // to undo changes: uncomment this line and...
ditto, and please remove this commented out line
In libraries/Preferences/src/Preferences.cpp <#5309 (comment)>:
if(!_started){
return;
}
+ esp_err_t err = nvs_commit(_handle); // to undo changes: delete the lines from here...
Since every other method which does some modifications already calls nvs_commit, is this call actually necessary?
In libraries/Preferences/src/Preferences.cpp <#5309 (comment)>:
}
/*
* Remove a key
* */
…-bool Preferences::remove(const char * key){
+bool Preferences::remove(const char * key){ // modified to add an nvs_commit()
same here, please remove trailing comments and the commented out return statement.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#5309 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATMBFQIIEI4SSKLXBMLN5YDTVVL2FANCNFSM467G4BJA>.
|
@me-no-dev this PR looks good to me, please consider it for the next release. |
@igrr @beamholder done :) |
Whoot! Thank-you.
On Aug 23, 2021, at 12:24 PM, Me No Dev ***@***.***> wrote:
@igrr <https://github.com/igrr> @beamholder <https://github.com/beamholder> done :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#5309 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATMBFQNAT3PYA44FKL2F5I3T6KG5FANCNFSM467G4BJA>.
|
In response to Issue #4972
In Preferences.cpp, the functions:
should be revised to include a call to
nvs_commit()
as required per
Non-volatile storage library
when using