Bluetooth: Mesh: Persistent storage of Virtual Addresses #19799
Conversation
LingaoM
requested review from
jhedberg,
joerchan,
trond-snekvik and
Vudentz
as code owners
|
How does this relate to #19499? Why did you open a new PR instead of continuing with the existing one? |
|
All checks are passing now. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
|
This PR should be more concise than before. |
There was a problem hiding this comment.
All my comments from the previous PR have been resolved 👍
I think the get_label() function in cfg_srv.c is a code smell, and it'd be better to take the index as a parameter to that function. That way, we'd encapsulate the array and its boundary. We'd still be manipulating another module's data though, but this is kind of the nature of the settings module.
@jhedberg: Perhaps the settings module should be restructured to only be a multiplexer for the "bt/mesh" settings directory, and the handlers should go into their respective modules? In a separate task, of course.
@trond-snekvik yes, sounds like a good idea |
subsys/bluetooth/mesh/foundation.h
Outdated
| @@ -112,12 +112,25 @@ | |||
| #define STATUS_UNSPECIFIED 0x10 | |||
| #define STATUS_INVALID_BINDING 0x11 | |||
|
|
|||
| enum label_flag { | |||
There was a problem hiding this comment.
I don't think you necessarily need a name for this enum, at least I don't think I saw you use the name anywhere
subsys/bluetooth/mesh/settings.c
Outdated
|
|
||
| if (err) { | ||
| BT_ERR("Failed to %s %s value", | ||
| IS_VA_DEL(lab)?"delete":"store", |
There was a problem hiding this comment.
Minor coding style: there should be spaces before & after ? and :
The 16-bit format group addresses will be stored, but we don't store (or restore) the virtual label UUIDs, i.e. after a power cycle the 16-bit group addresses would be meaningless. Fixes #19342 Signed-off-by: Lingao Meng <mengabc1086@gmail.com>
|
@LingoMeng please stop pushing to this PR, I've fixed up some coding style issues and pushed them to it, and I've approved the PR, so let's just wait for CI to finish so we can merge it. |
The mesh settings.c module is a giant piece of code responsible for storing the mesh stack configuration. Such approach makes it difficult to control the data to be stored, breaks the stack modules' encapsulation by forcing them to reveal the internal kitchen, which leads to unpleasant issues such as zephyrproject-rtos#19799. This commit moves the responsibility of storing the configuration to corresponding modules while keeping control of the moment of storing the configuration and of starting the stack after the settingss loading is completed. This doesn't introduce any abstraction between the mesh settings.c and other modules as it will add more complexity and overhead than necessary for the actual task. Fixes zephyrproject-rtos#19850 Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
The mesh settings.c module is a giant piece of code responsible for storing the mesh stack configuration. Such approach makes it difficult to control the data to be stored, breaks the stack modules' encapsulation by forcing them to reveal the internal kitchen, which leads to unpleasant issues such as #19799. This commit moves the responsibility of storing the configuration to corresponding modules while keeping control of the moment of storing the configuration and of starting the stack after the settingss loading is completed. This doesn't introduce any abstraction between the mesh settings.c and other modules as it will add more complexity and overhead than necessary for the actual task. Fixes #19850 Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
The 16-bit format group addresses will be stored,
but we don't store (or restore) the virtual label UUIDs,
i.e. after a power cycle the 16-bit group addresses
would be meaningless.
Fixes #19342
Signed-off-by: Lingao Meng mengabc1086@gmail.com