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

Mbed: Remove unused header size config #985

Merged
merged 1 commit into from
May 8, 2021

Conversation

boraozgen
Copy link
Contributor

Removing the header size configuration which apparently does not do anything.

What was the reason behind this parameter? @AGlass0fMilk

@d3zd3z
Copy link
Member

d3zd3z commented Apr 29, 2021

Do you know how the header size is getting set on the images? In Zephyr at least, this needs to be known by Zephyr, as it needs to be able to be run with the header, and imgtool needs it, in order to properly pad the image (and write the header).

@boraozgen
Copy link
Contributor Author

In Mbed OS, we set the application start address, which must be (header address + header size). The OS does not know about the bootloader or the header, and cares only about the start address of the application. The responsibility to match the addresses are left to the developer.

@utzig
Copy link
Member

utzig commented Apr 30, 2021

@boraozgen You have to sign off your commit to pass the CI.

@boraozgen
Copy link
Contributor Author

@boraozgen You have to sign off your commit to pass the CI.

Will do.

@boraozgen boraozgen closed this Apr 30, 2021
@boraozgen boraozgen reopened this Apr 30, 2021
@boraozgen boraozgen force-pushed the remove-header-size-config branch from 8aee277 to 0a01cf8 Compare May 3, 2021 07:35
@AGlass0fMilk
Copy link
Collaborator

I don't think this should be merged. The header size is used in the bootloader.

Early on in the Mbed-OS port of Mcuboot, I had minimal configuration options to try to prevent invalid configuration headaches. When @LDong-Arm joined in the effort, he added more configuration options so that developers could explicitly specify their memory configuration.

This is more flexible in that advanced users can accommodate other parts of internal flash that may be used for things like non-volatile settings storage, etc.

I think the header size declaration is necessary to keep, especially for the bootloader build.

@boraozgen
Copy link
Contributor Author

The header size is used in the bootloader.

Where is it used exactly?

@LDong-Arm
Copy link
Contributor

I think it's safe to remove this config:

  • When signing an image, the header size is specified with --header-size so MCUboot knows it. Presumably the header size is tied to the image instead of the bootloader?
  • The application inside an image has target.mbed_app_start set so the code starts after the image header.

@boraozgen
Copy link
Contributor Author

@LDong-Arm exactly. My comment here is also relevant to this topic.

@utzig
Copy link
Member

utzig commented May 4, 2021

  • When signing an image, the header size is specified with --header-size so MCUboot knows it. Presumably the header size is tied to the image instead of the bootloader?

Yes, the header size is part of the header of the image itself; the bootloader does not have an image size setting per se.

@AGlass0fMilk
Copy link
Collaborator

The header size is used in the bootloader.

Where is it used exactly?

A quick search of this repo for "MCUBOOT_HEADER_SIZE" shows only the mbed_lib.json file so I would say this is safe to remove.

I can't recall exactly why this option was added when I first did the Mbed-OS port. I vaguely recall mcuboot at the time requiring it?

Otherwise, the Mbed-OS port used to try and take care of all the configuration math for you. Instead of allowing you to configure things like the application start and size and all that, it would calculate it from these configurations.

Regardless, I agree this can be removed. Thanks @boraozgen!

@utzig utzig merged commit 66f02e3 into mcu-tools:master May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants