nvs: Check if an item is modified before writing out an identical copy (IDFGH-977)

[tim-nordell-nimbelink]

This prevents wear and tear on the flash, and it also is faster in some
cases since the read-out of flash is a cheaper operation than the erasure
of flash. Some library modules (such as the esp_wifi) write out to NVS
upon every initialization without checking first that the existing value
is the same, and this speeds up initialization of modules that make
these design choices and moves it into a centralized place.

The comparison functions are based on the read-out functions of the same
name, and changes out the memcpy(...) operations for memcmp(...)
operations.

Signed-off-by: Tim Nordell [email protected]

[tim-nordell-nimbelink]

I updated the pull request to fix a couple of things with the changeset:

• Remove potential erasures that would happen regardless in the callee functions. The comparison functions don't need to clean up after the error case.
• Replaced an assertion that the read size mismatched with a return condition that the contents were different for cmpMultiPageBlob(...).

[igrr]

@tim-nordell-nimbelink Thank you for the PR, it looks very useful. I have left a few mostly cosmetic comments.

Since the logic added in cmpItem and cmpMultiPageBlob is not exactly trivial, may I ask you to add a couple of unit tests for these functions in https://github.com/espressif/esp-idf/blob/master/components/nvs_flash/test_nvs_host/test_nvs.cpp, and a test that writes are not happening when item content is the same (SpiFlashEmulator::getWriteOps can be used for this).
If not, I can add the tests on top of your commit.

[tim-nordell-nimbelink]

@tim-nordell-nimbelink Thank you for the PR, it looks very useful.

No problem! It helps save wear and tear on the flash on our board (and start-up time out of deep sleep due to extra erases of flash).

I have left a few mostly cosmetic comments.

Sorry, it's easy for me to fall back to the coding style I usually use. I'll go back and fix-up through to match espressif's coding style.

Since the logic added in cmpItem and cmpMultiPageBlob is not exactly trivial, may I ask you to add a couple of unit tests for these functions

It shouldn't be too bad. The other thought I had was that really Page::cmp... and Page::read... are virtually identical. They could utilize a visitor pattern, but that would have a slightly deeper stack depth than the two separate routines. The benefit would be code maintenance, but it would touch more code within the NVS routine.

If not, I can add the tests on top of your commit.

I take it you prefer the tests to be added within the same commit?

-- Tim

[tim-nordell-nimbelink]

@igrr -

I've gone ahead and switched the logic around to return ESP_OK instead when the comparison is the same. I had to touch a couple of the tests for duplicates that assumed a new write occurred each time a value was written even if it was the same value. (I didn't run the test suite previously, but I had now that I added in the tests for this change set.) Hopefully I got all of the cosmetic changes, too.

-- Tim