-
Notifications
You must be signed in to change notification settings - Fork 697
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
Update Mbed OS CMake build file for Mbed CE #2152
base: main
Are you sure you want to change the base?
Update Mbed OS CMake build file for Mbed CE #2152
Conversation
boot/bootutil/src/swap_scratch.c
Outdated
@@ -174,6 +174,13 @@ boot_slots_compatible(struct boot_loader_state *state) | |||
j = sz1 = secondary_slot_sz = 0; | |||
smaller = 0; | |||
while (i < num_sectors_primary || j < num_sectors_secondary) { | |||
// Make sure that the boot_img_sector_size() calls below don't access undefined memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into a rather nasty issue when testing where I didn't set BOOT_MAX_IMG_SECTORS large enough, so this for loop exhibited undefined behavior. Throwing in a check against this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't even make sense, this is already checked on line 156 above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I checked in the debugger, and if I set BOOT_MAX_IMG_SECTORS to 1000, on a device that uses 1536, I see:
.
Looking into it a bit more, it seems this may be an issue due to the Mbed version of flash_area_get_sectors() never returning a count
value greater than MCUBOOT_MAX_IMG_SECTORS
. That causes the existing warning (which I totally didn't notice X_X) to never trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok pushed a fix!
boot/bootutil/src/swap_scratch.c
Outdated
@@ -174,6 +174,13 @@ boot_slots_compatible(struct boot_loader_state *state) | |||
j = sz1 = secondary_slot_sz = 0; | |||
smaller = 0; | |||
while (i < num_sectors_primary || j < num_sectors_secondary) { | |||
// Make sure that the boot_img_sector_size() calls below don't access undefined memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't even make sense, this is already checked on line 156 above
boot/bootutil/src/swap_scratch.c
Outdated
// Make sure that the boot_img_sector_size() calls below don't access undefined memory. | ||
if(i >= BOOT_MAX_IMG_SECTORS || j >= BOOT_MAX_IMG_SECTORS) | ||
{ | ||
BOOT_LOG_WRN("Cannot upgrade: BOOT_MAX_IMG_SECTORS too small"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes need to be squashed
boot/mbed/mcuboot_imgtool.cmake
Outdated
# Find or install imgtool ------------------------------------------------------------------------ | ||
|
||
if(NOT MCUBOOT_IMGTOOL_FOUND) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boot/mbed/mcuboot_imgtool.cmake
Outdated
|
||
get_filename_component(IMGTOOL_SCRIPTS_DIR ${CMAKE_CURRENT_LIST_DIR}/../../scripts REALPATH) | ||
|
||
# Find or install imgtool ------------------------------------------------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dashes not needed on comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
boot/mbed/mcuboot_imgtool.cmake
Outdated
|
||
if(NOT MCUBOOT_IMGTOOL_FOUND) | ||
|
||
# If we are using the Mbed venv, we can install asn1tools automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake indent is 2 spaces not 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted to 2
boot/mbed/mcuboot_imgtool.cmake
Outdated
# this function. Otherwise, it will default to ${CMAKE_CURRENT_BINARY_DIR}/<target name>-initial-image.hex | ||
# | ||
function(mcuboot_generate_initial_image TARGET) # optional 2nd arg: file name | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boot/mbed/mcuboot_imgtool.cmake
Outdated
gen_upload_target(${TARGET}-${IMAGE_TYPE}-image ${IMAGE_BASE_PATH}.bin) | ||
add_dependencies(flash-${TARGET}-${IMAGE_TYPE}-image ${TARGET}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boot/mbed/src/flash_map_backend.cpp
Outdated
bd_size_t erase_size = bd->get_erase_size(offset); | ||
sectors[*count].fs_size = erase_size; | ||
|
||
if(*count < MCUBOOT_MAX_IMG_SECTORS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces needed in c code i.e. if (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
boot/mbed/src/flash_map_backend.cpp
Outdated
sectors[*count].fs_size = erase_size; | ||
|
||
if(*count < MCUBOOT_MAX_IMG_SECTORS) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also as code above, bracket goes on same line as if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
0fa393a
to
57c1b31
Compare
@multiplemonomials I tested encrypted image with 0fa393a as bootloader, and mcuboot v1.8.0 included in primary application on DISCO_F746NG. This feature seems working now. I guess there was a bug in v1.8.0 to upgrade from encrypted image. I will wait until you are done with this batch of revision to update documentation. A minor issue: is
The build bootloader hex file has content up till 0x1D034. Other than this I don't see other issues as far as bootloader is concerned. |
@zhiyong-ft If you use the new methodology with flash bank settings, then:
Size without Application header (0x1000) and Trailer TLV's (0x1000), total slot size 0xC0000 in my case. |
Could you point me the documentation on this? I tried to build the bootloader for NUCLEO_F767ZI, starting address in hex file is: |
Just to point out that v1.8.0 is 3 years old at this point, it should not be being used |
v1.8.0 from what? |
MCUboot: https://github.com/mcu-tools/mcuboot/releases/tag/v1.8.0 |
I am using the fork:
|
The unfortunate fact is that before @multiplemonomials started to work on it, v1.8.0 was the last working version that can build in Mbed Studio and upgrade unencrypted image from secondary slot, yet crashes when trying to upgrade from encrypted image. But this is behind us now. |
Using encrypted images will be my next step. And returning from using mbed-ce is no option anymore. My firmware is almost finished and I spent two years on it. |
I outlined steps needed to get encrypted image to work in another thread. Give it a try. It is rather straightforward once we know it is working. It took me a while to map out the kinks because it was not mentioned at all in Mr. Beckstein's original port. He too seems have had some trouble. |
Can you paste a link to that? |
e8017b7
to
ce340d5
Compare
@zhiyong-ft @nordicjm I have updated this PR in response to comments and to add a way to generate encrypted images. Should be ready for another look! |
@multiplemonomials I can confirm that mcuboot as of ce340d5 can boot primary application and upgrade encrypted images from secondary slot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit. looks ok
6b84b5a
to
a4da87d
Compare
@nordicjm updated to add signed-off-by, could you trigger CI again? |
Any idea why the Zephyr check is failing? I don't believe it's related to anything I did... |
You need to rebase your branch, that issue has been fixed |
Signed-off-by: Jamie Smith <[email protected]>
a4da87d
to
de8c4b0
Compare
Oh, gotcha. Done! |
Hi, I'm Jamie, maintainer of Mbed CE, the community fork of Mbed. As Mbed OS 6 has been end-of-lifed by ARM, we hope to keep support for the OS alive in our fork for a long time coming!
Several members of the project have expressed interest in getting mcuboot running with Mbed CE, so I took some time and put together a test project based off of George Beckstein's original work. I was able to get it working more or less out of the box, but I did have to make some build system changes to get things running.
In particular, I wasn't happy with how the original design required running manual commands after compiling the code. That was an unfortunate necessity with the original Mbed 5/6 build system, but with Mbed CE, we use standard CMake, so we can improve this situation! I added a CMake script which automates calling imgtool to generate initial images, update images, and the public key file needed to build the bootloader.
With these changes, I am able to get a rudimentary demo application booting and updating. I and the others on the mbed-ce project (including @lefebvresam and @zhiyong-ft) hope to test out (and document) more features in time!
On another note, we heard from this issue that there is discussion about removing the Mbed port of mcuboot. We at mbed-ce are happy to help maintain and test the port going forward, and really want it to stay in the main branch of mcuboot!
Compatibility note: This PR will maintain compatibility with Mbed OS 6 using the Mbed CLI build system and/or Mbed Studio. However, it will break compatibility with the Mbed CLI 2 CMake-based build system. This build system never really reached maturity before ARM Mbed shut down, so I doubt that too many people will be using it who do not use Mbed CE.