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

mgmt/mcumgr/lib: Parasitic use of CONFIG_HEAP_MEM_POOL_SIZE in image management #44214

Closed
de-nordic opened this issue Mar 25, 2022 · 1 comment · Fixed by #46013
Closed

mgmt/mcumgr/lib: Parasitic use of CONFIG_HEAP_MEM_POOL_SIZE in image management #44214

de-nordic opened this issue Mar 25, 2022 · 1 comment · Fixed by #46013
Assignees
Labels
area: mcumgr bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@de-nordic
Copy link
Collaborator

de-nordic commented Mar 25, 2022

Describe the bug
The mcumgr image management needs to allocate context flash_img_context that holds several pieces of information, including quite significant in size flash image buffer, controller by other Kconfig option CONFIG_IMG_BLOCK_BUF_SIZE.
This context can either be allocated as static variable at compile time or from heap.
The problem is that a user does not have control over where the buffer is actually allocated, and when CONFIG_HEAP_MEM_POOL_SIZE is set to non-0 the mcumgr will automatically switch to using the HEAP.
Because this is not mentioned anywhere in Kconfig and the relation is non obvious it may be surprising to the user that perfectly working application stops working when mcumgr is enabled within it or opposite, the working smp_svr no longer wants to work when added to an application.

To Reproduce
Steps to reproduce the behavior:

  1. Build smp_svr with CONFIG_HEAP_MEM_POOL_SIZE smaller or equal to CONFIG_IMG_BLOCK_BUF_SIZE for the smp_svr image management to no longer work.

Attempt to upload image will finish with error 1 (UNKNOWN) being returned to the mcumgr client and uploads will no longer work.

Expected behavior
The connection between enabling CONFIG_HEAP_MEM_POOL_SIZE and mcumgr image management switching to use the HEAP should be both clearly indicated in Kconfig and documentations, and decision upon switching to HEAP should not be based on CONFIG_HEAP_MEM_POOL_SIZE >0 but, due to the significant size of the context, should be clearly left to the user with addition of a Kconfig option to control such switch.

Impact
Annoyance: increased pressure on heap at runtime leads to failed image upload attempts, changes to the Zephyr that impact heap allocation make applications that use to work completely suddenly not being able to perform image uploads; merging of an application that does use heap with smp_svr that does not seem to be ding so, by the project configuration, suddenly makes either stop working.

Logs and console output

Environment (please complete the following information):
Zephyr: 104a668

Additional context
Yey, it triggers #44219

@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label May 29, 2022
de-nordic added a commit to de-nordic/zephyr that referenced this issue May 30, 2022
The commit fixes issue where image management would switch to using
heap, whether developer wanted or not, when CONFIG_HEAP_MEM_POOL
gets value greater than zero.
Now when heap is enabled the user can select whether image management
will keep on using static variable, taking static RAM, or will use
heap to allocate the flash image context only when needed.
For this purpose CONFIG_IMG_MGMT_USE_HEAP_FOR_FLASH_IMG_CONTEXT
has been added, which is available when CONFIG_HEAP_MEM_POOL is enabled.

Fixes zephyrproject-rtos#44214

Signed-off-by: Dominik Ermel <[email protected]>
@de-nordic de-nordic removed the Stale label May 30, 2022
carlescufi pushed a commit that referenced this issue Jun 6, 2022
The commit fixes issue where image management would switch to using
heap, whether developer wanted or not, when CONFIG_HEAP_MEM_POOL
gets value greater than zero.
Now when heap is enabled the user can select whether image management
will keep on using static variable, taking static RAM, or will use
heap to allocate the flash image context only when needed.
For this purpose CONFIG_IMG_MGMT_USE_HEAP_FOR_FLASH_IMG_CONTEXT
has been added, which is available when CONFIG_HEAP_MEM_POOL is enabled.

Fixes #44214

Signed-off-by: Dominik Ermel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: mcumgr bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
2 participants