zephyr: Allow user-defined boot serial extensions #1788
Conversation
| zcbor_uint32_put(cs, MGMT_ERR_OK); | ||
| zcbor_map_end_encode(cs, 10); | ||
|
|
||
| return rc; |
There was a problem hiding this comment.
I am not sure whether rc here is stable: in case when it fails on the first slot in loop, then we have rc == 0 here finally and it seems that we return success even though we have failed in the first iteration, but if we fail in the last iteration then we return error here.
Does it make sense to return here something else than 0 if we assume that in case of error we just return empty string? Or should this be reflected by the "rc" returned by line 152?
There was a problem hiding this comment.
You know you are right in that it makes no sense, though it does appear that this code is actually what it does now: https://github.com/mcu-tools/mcuboot/blob/main/boot/zephyr/boot_serial_extensions.c
Not really sure what this should return, I don't see this command documented in the zephyr docs?
There was a problem hiding this comment.
In fact I would consider just removing this command entirely, what is the point in it? This is the hook it calls:
int boot_img_install_stat_hook(int image_index, int slot, int *img_install_stat)
{
return BOOT_HOOK_REGULAR;
}
So why return this at all then img mgmt list can return actual information including status about the image
There was a problem hiding this comment.
@nvlsianpu since you added this, what is the point in this command? Does anything use it? Can we just get rid of it?
There was a problem hiding this comment.
This command was added in order to make possible (among other) fetch NET core image installation status on nRF53, which was never used anywhere.
There was a problem hiding this comment.
Regard the return value - I would make it more consistent with failures this code could encounter.
There was a problem hiding this comment.
I'm of the mind it should just be deleted, if something is needed downstream for this it can be added downstream, and this is really not at all in the style of MCUmgr commands by generating a string with multiple pieces of information - in CSV format, and just appending it. I will update the PR
| STRUCT_SECTION_FOREACH(mcuboot_bs_custom_handlers, function) { | ||
| if (function->handler) { | ||
| mgmt_rc = function->handler(hdr, buffer, len, cs); | ||
|
|
||
| if (mgmt_rc != MGMT_ERR_ENOTSUP) { | ||
| break; |
There was a problem hiding this comment.
Wouldn't this mean that there might be more than one handler called for the same buffer and mutiple responses being sent for one?
There was a problem hiding this comment.
No, it would be a 1:1 mapping, if there are 3 handlers registered, the first one will be called, if it is not valid for that then it must return MGMT_ERR_ENOTSUP, it will then check the next handler, if the next handler is valid, it will return something else (i.e. usually EOK but could return EINVAL or anything) and then the loop here will be broken out, the final handler would not be called
This allows for out-of-tree modules to define their own boot serial functions by using iterable sections. Note that this also removes the custom img list command, which was not used in-tree. Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Adds a note on the reworked boot serial extensions features which now allows modules to add handlers Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
+106 byte flash increase with this change and both in-tree handlers enabled