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

settings: add support for static data #80360

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Laczen
Copy link
Collaborator

@Laczen Laczen commented Oct 24, 2024

When settings is to be used in combination with a configuration system there is a need to create a interface between the compile time generated subsystem configuration and the settings subsystem. The PR proposes such a coupling by allowing compile time generated configuration to be added to a linker section of static data. This data can then be used
by the subsystems h_set() routine that is called by settings_load().

A typical use case for this solution would be a firmware that contains multiple configurations.

If the linker section would be placed in a separate (internal) flash page this could also be used for provisioning.
No this wouldn't work, it needs both the data and the linker section to reside on the separate flash page

ssize_t rc = (len > static_data->size) ? -EINVAL : len;

if (rc > 0) {
memcpy(data, static_data->data, len);
Copy link

@ghost ghost Oct 24, 2024

Choose a reason for hiding this comment

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

What if we had a "zero-copy" option of the read callback to further reduce the overall memory and CPU overhead of using the settings subsystem vs. using the macro target? This callback would only be made available by backends that actually have this capability so that the client (e.g. the configured subsystem) knows how to handle memory ownership on its side.

The static backend obviously has this capability...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test is only showing the basic idea. The exchanged data could (in a configuration solution) be limited to pointers to the config data. The copy operation would then only be a copy of the pointers, not of the data.

tejlmand
tejlmand previously approved these changes Oct 25, 2024
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

minor nit, but nothing blocking.

@@ -26,6 +26,11 @@ config SETTINGS_DYNAMIC_HANDLERS
help
Enables the use of dynamic settings handlers

config SETTINGS_STATIC_DATA
bool "settings static data"
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit: we have dynamic settings handlers right above:

bool "dynamic settings handlers"

should we perhaps change prompt to:

Suggested change
bool "settings static data"
bool "static settings data"

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

same comment as @tejlmand re: nit, and the typo in the README should be fixed as this is the kind of stuff that can cause confusion for people copy-pasting command to their CLI without thinking twice :)

This application can be built and executed on native_sim as follows:

.. code-block:: console
$ ./scripts/twister -p native_sim -T tests/subsys/settings_commit_prio
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in test path

config SETTINGS_STATIC_DATA
bool "settings static data"
help
Enables the use of settings static data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Enables the use of settings static data
Enables the use of static settings data

When settings is to be used in combination with a configuration system
there is a need to create a interface between the compile time generated
subsystem configuration and the settings subsystem. The PR proposes
such a coupling by allowing compile time generated configuration to be
added to a linker section of static data. This data is then loaded
when `settings_load()` is called.

Signed-off-by: Laczen JMS <[email protected]>
@Laczen
Copy link
Collaborator Author

Laczen commented Nov 29, 2024

@kartben, I have updated the PR. I don't know if anyone is still interested in this since the configuration discussion died.

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Can never remember how settings works with all the functions, is the idea that this allows changing of the value during runtime, and if so saving that changed value somewhere so it will be automatically loaded?

zephyr_iterable_section(NAME settings_static_data KVMA RAM_REGION GROUP RODATA_REGION SUBALIGN ${CONFIG_LINKER_ITERABLE_SUBALIGN})
endif()


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

* SETTINGS_STATIC_DATA_DEFINE().
*/
struct settings_static_data {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +632 to +639
const char *name;
/**< Name of data. */

const void *data;
/**< Pointer to data */

size_t size;
/**< Data size */
Copy link
Collaborator

Choose a reason for hiding this comment

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

description goes above type

* This creates a variable _sname prepended by settings_static_data_.
*
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

* @param _size data size
*
* This creates a variable _sname prepended by settings_static_data_.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*

@@ -26,6 +26,11 @@ config SETTINGS_DYNAMIC_HANDLERS
help
Enables the use of dynamic settings handlers

config SETTINGS_STATIC_DATA
bool "static settings data"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool "static settings data"
bool "Static settings data"

void settings_static_data_load(const struct settings_load_arg *arg)
{
STRUCT_SECTION_FOREACH(settings_static_data, ch) {
if (settings_static_data_skip(ch, arg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

declare variables at top not part way through functions

LOG_INF("%s Called for %s", __func__, key);
zassert_true(settings_name_steq(key, "cfg", NULL), "bad settings key");

struct mysub_cfg cfg;
Copy link
Collaborator

Choose a reason for hiding this comment

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

declare variables at top not part way through functions

.. _settings_static_data:

Settings Subsystem static data Test
#######################################
Copy link
Collaborator

Choose a reason for hiding this comment

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

mismatched length

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.

5 participants