Skip to content

Conversation

@rghaddab
Copy link
Contributor

@rghaddab rghaddab commented Mar 19, 2025

use settings_load_one function to load one key/value settings entry instead of settings_load_subtree_direct.
This will avoid loading all the keys for some storage backends. Simplify as well the Delete function by skipping loading the value before deleting it.

NCS PR: nrfconnect/sdk-nrf/pull/20691

ReadEntry entry{ value, value_size, 0, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND };
settings_load_subtree_direct(fullKey, LoadEntryCallback, &entry);
// Found requested key.
const ssize_t bytesRead = settings_load_one(fullKey, value, value_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value_size < kEmptyValueSize, this function will fail even though it should be possible to get empty value even with value_size == 0.

@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch 2 times, most recently from 048b54b to 7293889 Compare March 28, 2025 05:47
@rghaddab rghaddab marked this pull request as ready for review March 31, 2025 08:49
@rghaddab rghaddab requested a review from a team as a code owner March 31, 2025 08:49
@rghaddab rghaddab added the DNM label Mar 31, 2025
@rghaddab
Copy link
Contributor Author

should be merged after this one nrfconnect/sdk-zephyr#2615

@LuDuda LuDuda requested a review from Damian-Nordic March 31, 2025 22:09
@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch 3 times, most recently from eab34c5 to 0f562e4 Compare April 2, 2025 11:36
@LuDuda LuDuda requested a review from kkasperczyk-no April 2, 2025 22:59
char fullKey[SETTINGS_MAX_NAME_LEN + 1];
ReturnErrorOnFailure(MakeFullKey(fullKey, key));

printk("Riadh Put %s size %u\n", fullKey, value_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging log leftover

if (len > 0)
{
*val_len = len;
return true;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return true;;
return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

{
ssize_t bytesRead = settings_load_one(fullkey, dest_buf, dest_size);

printk("Riadh loading key %s bytesRead %u\n", fullkey, bytesRead);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging log leftover

*read_bytes_size = entry.readSize;
*read_bytes_size = readSize;
}
printk("Riadh result : %" CHIP_ERROR_FORMAT "\n", result.Format());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log leftover

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that, all debug logs leftover fixed

@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch 3 times, most recently from 3db67c8 to 195ea4c Compare April 3, 2025 12:05
@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch from 195ea4c to 9757f61 Compare April 3, 2025 12:19
@kkasperczyk-no
Copy link
Contributor

@rghaddab you were missing the sdk-nrf PR link in a PR description to run the unit tests on your sdk-zephyr changes. Damian has added it for you, but you have to force push/re-run failing test to make it green.

@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch from 9757f61 to e965680 Compare April 7, 2025 08:03
@kkasperczyk-no
Copy link
Contributor

kkasperczyk-no commented Apr 7, 2025

@rghaddab I've analyzed the logs from unit tests. Actually it shows the failure related to your change. But unfortunately the log was truncated, so I had to download the package and analyze it locally. Here are the failures:

First
2025-04-07T09:08:48.1753767Z INFO �[31m�[1mERR�[0m TestKeyValueStoreMgr.cpp:68: Failure
2025-04-07T09:08:48.1753949Z INFO �[31m�[1mERR�[0m Expected: readSize == kTestValueLen
2025-04-07T09:08:48.1754092Z INFO �[31m�[1mERR�[0m Actual: 5 == 0
2025-04-07T09:08:48.1754273Z INFO �[31m�[1mERR�[0m TestKeyValueStoreMgr.cpp:72: Failure
2025-04-07T09:08:48.1754455Z INFO �[31m�[1mERR�[0m Expected: readSize == kTestValueLen
2025-04-07T09:08:48.1754588Z INFO �[31m�[1mERR�[0m Actual: 5 == 0
2025-04-07T09:08:48.1754903Z INFO �[31m�[1mERR�[0m [ FAILED ] TestKeyValueStoreMgr.EmptyString

And second
2025-04-07T09:08:48.1757266Z INFO �[31m�[1mERR�[0m TestKeyValueStoreMgr.cpp:227: Failure
2025-04-07T09:08:48.1757454Z INFO �[31m�[1mERR�[0m Expected: readSize == sizeof(readValue)
2025-04-07T09:08:48.1757597Z INFO �[31m�[1mERR�[0m Actual: 10 == 9
2025-04-07T09:08:48.1757775Z INFO �[31m�[1mERR�[0m TestKeyValueStoreMgr.cpp:228: Failure
2025-04-07T09:08:48.1758020Z INFO �[31m�[1mERR�[0m Expected: memcmp(kTestValue, readValue, readSize) == 0
2025-04-07T09:08:48.1758154Z INFO �[31m�[1mERR�[0m Actual: 1 == 0
2025-04-07T09:08:48.1758380Z INFO �[31m�[1mERR�[0m [ FAILED ] TestKeyValueStoreMgr.TooSmallBufferRead

@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch from e965680 to a8f01b1 Compare April 7, 2025 23:24
@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch from a8f01b1 to 02ae23e Compare April 8, 2025 09:49
Use settings_load_one function to load one key/value settings entry
instead of settings_load_subtree_direct.
This will avoid loading all the keys for some storage backends.
Simplify as well the Delete function by using settings_get_val_len()
function.

Signed-off-by: Riadh Ghaddab <[email protected]>
To maintain backward compatibility, keep the support for the legacy ZMS
backend for Settings.

Signed-off-by: Riadh Ghaddab <[email protected]>
@rghaddab rghaddab force-pushed the rghaddab/use-settings-load-one branch from 02ae23e to 5aae50c Compare April 11, 2025 10:16
@carlescufi carlescufi merged commit 153abdf into nrfconnect:master Apr 12, 2025
9 checks passed
@carlescufi carlescufi removed the DNM label Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants