Add Mcumgr handlers for group enumeration #70704
Conversation
Mcumgr currently has no method of enumerating all registered groups. This commit adds that capability, as well as adding a separate Mcumgr group for retrieving this information. Signed-off-by: Hunter Searle <[email protected]>
hsearle
force-pushed
the
mcumgr_add_enumeration_group
branch
from
006f433 to
976432a
Compare
subsys/mgmt/mcumgr/mgmt/src/mgmt.c
Outdated
| int mgmt_get_group_ids(int *ids) | ||
| { | ||
| int count = 0; | ||
| sys_snode_t *snp, *sns; | ||
|
|
||
| SYS_SLIST_FOR_EACH_NODE_SAFE(&mgmt_group_list, snp, sns) { | ||
| struct mgmt_group *loop_group = | ||
| CONTAINER_OF(snp, struct mgmt_group, node); | ||
| ids[count] = loop_group->mg_group_id; | ||
| count++; | ||
| } | ||
| return count; | ||
| } |
There was a problem hiding this comment.
This should call a callback instead so that the memory usage is bounded not unbounded which is the zephyr way of doing this
There was a problem hiding this comment.
I'm not fully understanding your comment here. I get that using a callback instead of a direct function call could be more of the zephyr way, but how does it change whether the memory usage is bounded or unbounded? Either way, to get all group IDs, we need to iterate through the whole mgmt_group_list array.
There was a problem hiding this comment.
If you have 3 groups, this returns 3 and your code creates a stack variable of size 3*4 = 12 bytes. If you have 200 groups, this returns 200 and your code creates a stack variable of size 200*4 = 800 bytes, thus it is unbounded.
There was a problem hiding this comment.
I understand that, but I'm not clear how moving to a callback implementation would cause the memory to be bounded. If we want a list of all group ids, no matter how we retrieve it, that will inherently be unbounded by the number of groups in the system.
There was a problem hiding this comment.
Not true, each callback occupies the same amount of stack space, if you are appending data to the response buffer then that is a pre-defined buffer of a certain size (or multiple if fragmented) so nothing is unbounded. See https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/grp/os_mgmt/src/os_mgmt.c#L338 for an example
There was a problem hiding this comment.
So, if I'm understanding correctly, I need to introduce a variable that caps the number of groups that can be returned, and if there are more groups registered than the variable allows, this command will just return an error?
There was a problem hiding this comment.
No? The callback is called for every entry. If in the callback in your .c which adds the groups to the response cannot add any more then that needs to return an SMP error
There was a problem hiding this comment.
There are still errors to work out, but do you think that my updated approach is acceptable? Or, am I at least getting closer?
|
Also needs tests, see https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/subsys/mgmt/mcumgr/settings_mgmt for example |
| count = sys_slist_len(groups); | ||
|
|
||
| ok = zcbor_tstr_put_lit(zse, "groups") && | ||
| zcbor_list_start_encode(zse, count); |
There was a problem hiding this comment.
count needs to be more e.g. if canonical mode is enabled
| * | ||
| * @return Pointer to list of registered groups | ||
| */ | ||
| sys_slist_t *mgmt_get_group_list(); |
There was a problem hiding this comment.
This should be a function that calls a callback multiple times, once per group, and returns the group object, it should not be just returning a list directly or using sys_slist_t when the type should be the mgmt_group type (should also be const in the callback)
| #define ENUMERATION_MGMT_LIST 0 | ||
| #define ENUMERATION_MGMT_GP_ID 10 | ||
|
|
| @@ -29,7 +30,7 @@ static sys_slist_t mgmt_callback_list = | |||
| void | |||
| mgmt_unregister_group(struct mgmt_group *group) | |||
| { | |||
| (void)sys_slist_find_and_remove(&mgmt_group_list, &group->node); | |||
| sys_slist_find_and_remove(&mgmt_group_list, &group->node); | |||
|
My work focus is shifting away from Zephyr, so I'm going to close out this PR. If someone else sees value in this change and wants to move the work forward, please do so. |
Mcumgr currently has no method of enumerating all registered groups. This commit adds that capability, as well as adding a separate Mcumgr group for retrieving this information.