zephyr: Missing header and definitions in boot_serial extensions

[de-nordic]

Moved group definitions to extension source code.

Signed-off-by: Dominik Ermel dominik.ermel@nordicsemi.no

[nordicjm]

This should be including <zephyr/mgmt/mcumgr/grp/zephyr/zephyr_basic.h> not redefining these

[de-nordic]

Unfortuately this requires addition of mgmt.h which then requires addition of smp.h which then ... generally half of MCUmgr and Zephyr needs to be brought here. The MCUboot provides own definitions for groups it serves, so I do not find this as a problem.

[nordicjm]

This is duplicating data and making it redundant/prone to errors if one is changed. @gmarull thoughts on this?

For the command, zephyr_basic.h can at least be included as it has no include requirements

[de-nordic]

MCUboot is independent from Zephyr and may redefine identifiers as it wishes, and actually does so.

[nordicjm]

boot/zephyr/boot_serial_extensions.c this is a zephyr file. Neither MCUmgr groups nor commands should not re-registered, or like any duplicate code you have the possibility of a mismatch in the future - this is not a MCUmgr-specific issue

[kmeinhar]

I am curious what is the status on this PR? Is this the intended fix or is there another way to use boot_serial extensions?

[de-nordic]

I am curious what is the status on this PR?

Disagreement limbo.

Is this the intended fix

Yes.
This specific source file defines Zephyr specific extension to MCUboot serial recovery. It is Zephyr specific in a way that it has originally been defined for Zephyr to provide command of storage erase; it has been given group 1 below first user group in Zephyr.
MCUboot is free to redefine any identifiers for the SMP protocol group as it wishes, which the PR tries to do.
This is a little bit gray area because the code implements Zephyr specific codes so the best way to do that would be to bring identifier definitions from some Zephyr header, but this causes major problem: Zephyr headers, often, do not separate identifiers/enums and API definitions which means that when you try to include header for some identifier you end up including half of the system headers, and this gets problematic when some of the headers are not visible, for some reason, to MCUboot.
So it makes the Zephyr extensions for boot_serial not possible to compile.

That is why the redefinition here.

or is there another way to use boot_serial extensions?

I do not understand the question.

[tejlmand]

Zephyr headers, often, do not separate identifiers/enums and API definitions which means that when you try to include header for some identifier you end up including half of the system headers, and this gets problematic when some of the headers are not visible, for some reason, to MCUboot.

Have you tried opening a PR in Zephyr that improves this for this use-case ?

That would allow you to just include what you need and avoid duplication of define, as well as it avoids including half of Zephyr system headers.

[de-nordic]

Zephyr headers, often, do not separate identifiers/enums and API definitions which means that when you try to include header for some identifier you end up including half of the system headers, and this gets problematic when some of the headers are not visible, for some reason, to MCUboot.

Have you tried opening a PR in Zephyr that improves this for this use-case ?

No. I was considering that, but I will (or @nordicjm ) doing the work for MCUmgr anyway and it takes time to review stuff in Zephyr. And I needed quick workaround for customer visible issue, that is why I have decided to duplicate the definition rather than wait for the solution to get accepted in Zephyr. Afterwards I can remove workaround without customer noticing.

The question is whether this should be considered Zephyr-wide issue to fix or are we fixing the MCUmgr headers for the use-case.

That would allow you to just include what you need and avoid duplication of define, as well as it avoids including half of Zephyr system headers.

I don't mind duplicate defines in separate project, as the project is allowed to define stuff as it wants as long as it works with protocol specification, but as I have mentioned before: in this case it would be better if Zephyr headers could be used.

[github-actions]

This pull request 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 pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.