dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library #30370

nvlsianpu · 2020-12-01T17:03:25Z

As some code are defined here and there it becomes problem when module is compiled within MCUboot.
It is possible to mitigate problem using #fdefry - but this is temporary hack.

aim to supersede #30060

fixes #29963

takes needs of dfu: mcuboot: add shell module #29669 into account

step needed for merge this PR

get the Zephyr's release-team member who can assist with merges sequence smoothly

1. PR approved,
2. merge zephyr: extracted bootutil_public library mcu-tools/mcuboot#892 (is approved, ready to be merged)
3. create upmereg PR to https://github.com/zephyrproject-rtos/mcuboot
4. merge the upmerge PR
5. update mcuboot SHA and merge this PR.

modules/Kconfig.mcuboot_bootutil

github-actions · 2020-12-01T20:42:14Z

The following projects have a revision update in this Pull Request:

Name	Old Revision	New Revision
mcuboot	zephyrproject-rtos/mcuboot@`c986a90`	zephyrproject-rtos/mcuboot@`6f48e0a` (master)

Note: This message is automatically posted and updated by the Manifest GitHub Action.

modules/Kconfig.mcuboot_bootutil

include/dfu/mcuboot.h

tests/subsys/dfu/mcuboot/src/main.c

utzig · 2020-12-18T13:16:17Z

include/dfu/mcuboot.h

+ * @return 0 on success, negative errno code on fail.
+ */
+int mcuboot_read_swap_state_by_id(int flash_area_id,
+				  struct mcuboot_swap_state *state);


Is there a rationale for adding this function and struct mcuboot_swap_state instead of just using boot_read_swap_state_by_id and struct boot_swap_state?

boot_read_swap_state_by_id() can't be made zephyr API. It can only be used by module which made a module shim (as it links with MCUBOOT_BOOTUTIL) (by convention). For the MCUBoot shell i can link it with the library as well, and left behind this API.
Do you think that this API will be rather not useful for other?

Ok, I think I understand, but shouldn't you also add the image_num field to struct mcuboot_swap_state since mcuboot_read_swap_state_by_id still calls boot_read_swap_state_by_id (including a proper pointer cast) and boot_read_swap_state tries to populate boot_num which you have not included in your definition of struct mcuboot_swap_state. Is it correct?

Adding image_num would be a short term fix. We should not blindly cast one structure to the other, because that is error prone. Somebody will change order of fields or add new information to struct boot_swap_state and we will be screwed once again.

So we should either:

add helper struct boot_swap_state variable and copy each structure field separately,

link MCUBOOT_BOOTUTIL to mcuboot shell or other modules that use mcuboot public API.

link MCUBOOT_BOOTUTIL to mcuboot shell or other modules that use mcuboot public API.

Ok, I will go with that

modules/Kconfig.mcuboot_bootutil

nvlsianpu · 2021-01-08T16:28:41Z

Currently this PR breaks git bisectability: all commits (except the last one that touches tests) are needed in order to build successfully.

I will squash patches once PR changes will be accepted.

de-nordic

Looks OK.

de-nordic · 2021-01-11T08:35:56Z

modules/Kconfig.mcuboot_bootutil

+#hiden option for disabling module-own log configuration
+#while building MCUboot bootloader


Missing space between # and first word.

de-nordic · 2021-01-11T08:48:16Z

subsys/dfu/boot/mcuboot.c

-bool boot_is_img_confirmed(void)
-{
-	return boot_image_ok_read(FLASH_AREA_IMAGE_PRIMARY) == BOOT_FLAG_SET;
+	return flag_val == BOOT_FLAG_SET;


I have noticed that it is not distinguished between failure to access the flag and the flag not being set.
This is not issue to this PR but maybe we need return some other type than bool to distinguish between failures and actual flag check.

Reworking this would be an API change - so, yes not in this PR.

nvlsianpu · 2021-01-11T12:42:14Z

Currently this PR breaks git bisectability: all commits (except the last one that touches tests) are needed in order to build successfully.

@mniestroj Squashed

include/dfu/mcuboot.h

nvlsianpu · 2021-01-13T11:21:49Z

@mniestroj Regard next step for eliminate copied definitions (so linking test, mcumgr, img_utill to MCUBOOT_BOOTUTIL library)- I will do that in the next PR which will open immediately after this will be merge. I think this PR is heavy enough - I prefer smaller chunks of patches.

nvlsianpu · 2021-01-13T17:41:28Z

mcuboot counterpart PR zephyrproject-rtos/mcuboot#42

Some parts of subsys/dfu/boot code are re-implementations of what is implemented in the MCUBoot repository. Mcuboot's repository already provide implementation of function required for application for interact with the MCUboot. This patch introduces new MCUBOOT_BOOTUTIL module which covers common code which is used in the bootloader and the chainnloaded application. dfu/boot: use MCUBoot's source code Module was reworked so it start using MCUBoot's bootutil_public API instead of copied code. Reworked boot_is_img_confirmed() used MCUBoot's API for determine image_ok flag. mcuboot_shell switchd to use MCUboot's boot_read_swap_state_by_id() This is MCUBoot function, use it for avoid linking conflict. test/subsys/mcuboot: fix `test_write_confirm` dfu/boot library was reworked so it uses MCUboot's bootutil_public library whenever it can. The library required that image was marked as copy-done before it can be pending. This patch adds such mark which fixes the test. Signed-off-by: Andrzej Puzdrowski <[email protected]>

MCUboot support flash write-bock-size up to BOOT_MAX_ALIGN. This patch takes this into account. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Mcumgr-cli application doesn't accept log on the communication channel. Since zephyrproject-rtos#30370 was merged log messages are generated from MCUBOOT_UTIL by default. That made `mcumgr image upload` failing. Patch disables logging via shell which cure the issue. It is not necessary if another application on the host intercepts log messages (like picocom can). Signed-off-by: Andrzej Puzdrowski <[email protected]>

Mcumgr-cli application doesn't accept log on the communication channel. Since #30370 was merged log messages are generated from MCUBOOT_UTIL by default. That made `mcumgr image upload` failing. Patch disables logging via shell which cure the issue. It is not necessary if another application on the host intercepts log messages (like picocom can). Signed-off-by: Andrzej Puzdrowski <[email protected]>

Mcumgr-cli application doesn't accept log on the communication channel. Since zephyrproject-rtos#30370 was merged log messages are generated from MCUBOOT_UTIL by default. That made `mcumgr image upload` failing. Patch disables logging via shell which cure the issue. It is not necessary if another application on the host intercepts log messages (like picocom can). Signed-off-by: Andrzej Puzdrowski <[email protected]> (cherry picked from commit cb6e2a6) Signed-off-by: Johann Fischer <[email protected]> (cherry picked from commit 895f9ed)

nvlsianpu requested a review from nashif as a code owner December 1, 2020 17:03

nvlsianpu commented Dec 1, 2020

View reviewed changes

modules/Kconfig.mcuboot_bootutil Outdated Show resolved Hide resolved

github-actions bot added area: Modules manifest west manifest-mcuboot DNM This PR should not be merged (Do Not Merge) labels Dec 1, 2020

de-nordic reviewed Dec 2, 2020

View reviewed changes

modules/Kconfig.mcuboot_bootutil Outdated Show resolved Hide resolved

nvlsianpu requested review from carlescufi, galak and MaureenHelm as code owners December 7, 2020 12:07

github-actions bot added the area: API Changes to public APIs label Dec 7, 2020

nvlsianpu mentioned this pull request Dec 12, 2020

dfu: mcuboot: add shell module #29669

Merged

nvlsianpu force-pushed the zephyr/mcuboot_util branch from 7d5b848 to 29f2b5d Compare December 16, 2020 07:26

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Dec 16, 2020

nvlsianpu force-pushed the zephyr/mcuboot_util branch 4 times, most recently from 7a5e7b3 to 4bccc6f Compare December 16, 2020 14:25

nvlsianpu changed the title ~~[WIP] dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library~~ dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library Dec 16, 2020

nvlsianpu mentioned this pull request Dec 18, 2020

zephyr: extracted bootutil_public library mcu-tools/mcuboot#892

Merged

nvlsianpu requested a review from utzig December 18, 2020 11:20

nvlsianpu force-pushed the zephyr/mcuboot_util branch from 4bccc6f to 69d49c8 Compare December 18, 2020 11:38

utzig reviewed Dec 18, 2020

View reviewed changes

nvlsianpu force-pushed the zephyr/mcuboot_util branch from 69d49c8 to ad0e275 Compare December 18, 2020 17:05

nvlsianpu mentioned this pull request Dec 18, 2020

dfu: mcuboot: fail to build with CONFIG_BOOTLOADER_MCUBOOT=n and CONFIG_IMG_MANAGER=y #30075

Closed

mniestroj self-requested a review December 18, 2020 19:33

tejlmand reviewed Dec 22, 2020

View reviewed changes

modules/Kconfig.mcuboot_bootutil Outdated Show resolved Hide resolved

nvlsianpu force-pushed the zephyr/mcuboot_util branch from ad0e275 to b9bd15e Compare December 22, 2020 18:07

nvlsianpu force-pushed the zephyr/mcuboot_util branch from 89b7130 to fdd19bc Compare January 8, 2021 16:26

zephyrbot added the area: DFU Device Firmware Upgrade label Jan 8, 2021

zephyrbot assigned nvlsianpu Jan 8, 2021

utzig approved these changes Jan 11, 2021

View reviewed changes

de-nordic approved these changes Jan 11, 2021

View reviewed changes

nvlsianpu force-pushed the zephyr/mcuboot_util branch 2 times, most recently from ade92ac to 357993d Compare January 11, 2021 12:41

mniestroj reviewed Jan 11, 2021

View reviewed changes

include/dfu/mcuboot.h Show resolved Hide resolved

include/dfu/mcuboot.h Show resolved Hide resolved

include/dfu/mcuboot.h Show resolved Hide resolved

nvlsianpu force-pushed the zephyr/mcuboot_util branch 2 times, most recently from ffa9038 to 29e876f Compare January 13, 2021 11:17

mniestroj approved these changes Jan 13, 2021

View reviewed changes

carlescufi approved these changes Jan 13, 2021

View reviewed changes

nvlsianpu force-pushed the zephyr/mcuboot_util branch from 29e876f to c30de78 Compare January 13, 2021 17:39

nvlsianpu added 2 commits January 14, 2021 17:04

tests/subsys/dfu/mcuboot: support mcuboot max alignment

ae6404d

MCUboot support flash write-bock-size up to BOOT_MAX_ALIGN. This patch takes this into account. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu force-pushed the zephyr/mcuboot_util branch from c30de78 to ae6404d Compare January 14, 2021 16:04

github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Jan 14, 2021

carlescufi merged commit d9c308c into zephyrproject-rtos:master Jan 14, 2021

nvlsianpu mentioned this pull request Jan 14, 2021

dfu: mcuboot: allow only to erase secondary slot #30060

Closed

mniestroj mentioned this pull request Jan 14, 2021

dfu: mcuboot: define boot_img_magic when FLASH_AREA_IMAGE_SECONDARY #30076

Closed

This was referenced Jan 26, 2021

samples/subsys/mcumgr/smp_svr: disable log over shell #31626

Merged

mcumgr-cli image upload is failing on shell channel after MCUBOOT_BOOTUTIL library was introduced #31629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library #30370

dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library #30370

nvlsianpu commented Dec 1, 2020 •

edited

Loading

github-actions bot commented Dec 1, 2020 •

edited

Loading

utzig Dec 18, 2020

nvlsianpu Dec 18, 2020

nvlsianpu Dec 23, 2020

utzig Jan 5, 2021

mniestroj Jan 5, 2021

nvlsianpu Jan 8, 2021

nvlsianpu Jan 8, 2021

nvlsianpu commented Jan 8, 2021

de-nordic left a comment

de-nordic Jan 11, 2021

de-nordic Jan 11, 2021

nvlsianpu Jan 11, 2021

nvlsianpu commented Jan 11, 2021

nvlsianpu commented Jan 13, 2021

nvlsianpu commented Jan 13, 2021

		#hiden option for disabling module-own log configuration
		#while building MCUboot bootloader

dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library #30370

dfu/boot: rework using newly introduced MCUBOOT_BOOTUTIL library #30370

Conversation

nvlsianpu commented Dec 1, 2020 • edited Loading

github-actions bot commented Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu commented Jan 8, 2021

de-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu commented Jan 11, 2021

nvlsianpu commented Jan 13, 2021

nvlsianpu commented Jan 13, 2021

nvlsianpu commented Dec 1, 2020 •

edited

Loading

github-actions bot commented Dec 1, 2020 •

edited

Loading