Add padding up to restrict_size #11043

statropy · 2019-07-13T18:25:06Z

Description

https://os.mbed.com/docs/mbed-os/v5.13/reference/bootloader-configuration.html section target.restrict_size states that:
"The postbuild merge process pads the resulting bootloader binary to its end address."
This padding is added for region merges that specify restrict_size. Additional pad_to_restrict_size boolean argument added to allow both full and 'update' outputs.

Fixes: #9949

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

https://os.mbed.com/docs/mbed-os/v5.13/reference/bootloader-configuration.html section target.restrict_size states that: "The postbuild merge process pads the resulting bootloader binary to its end address." This padding is added for region merges that specify restrict_size. Additional pad_to_restrict_size boolean argument added to allow both full and 'update' outputs. Fixes: ARMmbed#9949

ciarmcom · 2019-07-13T19:00:23Z

@statropy, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

bulislaw · 2019-07-16T12:45:40Z

@mark-edgeworth @madchutney please review

mbed-ci · 2019-07-17T17:55:20Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_exporter

mbed-ci · 2019-07-19T13:32:55Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

madchutney · 2019-07-24T09:43:27Z

Hi @statropy, I am trying understand the problem you are trying to solve with this change. Also I didn't understand the comment of 'full' vs 'update'. Could you please explain?

statropy · 2019-07-24T13:54:54Z

As captured in issue #9949, somewhere between mbed-os 5.11.0 and 5.11.1, creating a bootloader section using restrict_size was broken. Using restrict_size, as stated by the documentation referenced above, will pad out the binary output of the build to the restrict_size value. This is so that the main application can bundle the bootloader section with its binary, and to determine the proper start address for that application (directly after the bootloader section). This change adds the padding to the binary output of a build that uses restrict_size.

I probably used the wrong terminology for 'update' vs 'full'. The mbed-os build produces two binary outputs: [project].bin and [project]_application.bin. The [project].bin includes the restrict_size padding and is used as the "target.bootloader_img" value in the main application. The _application.bin does NOT include the padding. This binary is intended to be used to update just the bootloader section on a device where a previous version of the bootloader is already present, therefore the padding isn't needed. The value of this is for a smaller size binary for use with updates, such as an over-the-air update.

This change makes the functionality match the referenced documentation.

Edit: [project] was not visible

SeppoTakalo · 2019-07-25T09:35:57Z

@madchutney Are you happy with the explanation? This seems to pass tests, and is otherwise good to go in. I would however mark this as 5.14-rc1 as it might change the way how binaries are produced, so that would give us some time to run it in the CI and manual testing, before pushing it to public release.

madchutney · 2019-07-25T10:56:29Z

@statropy Thanks for the explanation and the contribution, I have been looking at the original cause of the issue which seems to be #8097 and I'm trying to determine if this fix will cause a problem with that requirement.

madchutney · 2019-07-25T11:01:02Z

@SeppoTakalo The explanation was useful, I am checking that this won't cause a regression. I believe we will need a unit test for this code to ensure the functionality is not unexpectedly removed again.

I am also concerned whether there is a need for the pad_to_restrict_size variable as it seems to me at the moment the only correct behaviour is when it is set to True. I'd rather not introduce an unnecessary variable, although I completely understand why @statropy has done so in this PR.

statropy · 2019-07-25T12:38:03Z

@madchutney Thanks for looking into it further. I am concerned that I wasn't able to find the original point of regression, and that this change would cause other regressions.

As for the Nordic targets in particular, the default output is .hex not bin, so my fix would only work when the output format is changed to .bin. This may be required for a bootloader segment though, because I think "target.bootloader_img" will only accept a binary.

A further complication with the Nordic targets is the bundling of a soft-device as 'bootloader' segment. I'm not using the soft-device in my Nordic build, but I don't want to break that functionality either.

madchutney

Thanks a lot for the changes, I don't think this will break the previous fix and this should certainly resolve the defect. To ensure we don't get a similar regression again could you please add a unit test to mbed-os/tools/test/build_api/build_api_test.py?

There is one failing test in the suite (test_always_complete_build) so I don't think they are run in CI, which I will look into.

Many thanks,
Graham

madchutney · 2019-07-25T15:25:15Z

tools/regions.py

@@ -168,6 +170,9 @@ def merge_region_list(
            pad_size = start - begin
            merged.puts(begin, padding * pad_size)
            begin = stop + 1
+        if restrict_size is not None and pad_to_restrict_size:
+            pad_size = int(restrict_size, 0) - begin


This would restore the output to something similar to pre 11.1 behaviour

Suggested change

pad_size = int(restrict_size, 0) - begin

pad_size = int(restrict_size, 0) - begin

notify.info(" Padding region with 0x%x bytes" % pad_size)

madchutney · 2019-07-25T15:26:30Z

tools/regions.py

@@ -116,6 +117,7 @@ def merge_region_list(
    destination - file name to write all regions to
    padding - bytes to fill gaps with
    restrict_size - check to ensure a region fits within the given size
+    pad_to_restrict_size - fill with padding up to restrict_size


I do not think we need pad_to_restrict_size, I cannot see a situation where this would be needed and without it I think it fixes the regression that appeared in 11.1

statropy · 2019-07-25T16:17:52Z

OK you've convinced me on not needing pad_to_restrict_size. I can get the same effect when calling merge_region_list() and leaving restrict_size as None. Much better. That bool felt hacky.

I'll look at adding a unit test.

This is my first project using mbed and love it so far. Happy to help contribute.

madchutney · 2019-07-26T14:59:07Z

Thanks that will be great

0xc0170 · 2019-08-20T11:08:08Z

@statropy any update for this PR? If any questions, ask.

0xc0170 · 2019-08-26T12:27:58Z

@statropy It would be good to have this fixed. I would close this PR as it has not been updated for a month.

pawelm87 · 2019-10-02T06:56:10Z

Ok I try build bootloader:

`PS C:\Users\p\Desktop\workspace> mbed import mbed-os-example-bootloader
[mbed] Working path "C:\Users\p\Desktop\workspace" (directory)
[mbed] Importing program "mbed-os-example-bootloader" from "https://github.com/ARMmbed/mbed-os-example-bootloader" at latest revision in the current branch
[mbed] Adding library "mbed-os" from "https://github.com/ARMmbed/mbed-os" at rev #0f959dbe4749
PS C:\Users\p\Desktop\workspace> cd mbed-os-example-bootloader
PS C:\Users\p\Desktop\workspace\mbed-os-example-bootloader> mbed toolchain GCC_ARM
[mbed] Working path "C:\Users\p\Desktop\workspace\mbed-os-example-bootloader" (program)
[mbed] GCC_ARM now set as default toolchain in program "mbed-os-example-bootloader"
PS C:\Users\p\Desktop\workspace\mbed-os-example-bootloader> mbed target NUCLEO_L476RG
[mbed] Working path "C:\Users\p\Desktop\workspace\mbed-os-example-bootloader" (program)
[mbed] NUCLEO_L476RG now set as default target in program "mbed-os-example-bootloader"
PS C:\Users\p\Desktop\workspace\mbed-os-example-bootloader> mbed compile
[mbed] Working path "C:\Users\p\Desktop\workspace\mbed-os-example-bootloader" (program)
Building project mbed-os-example-bootloader (NUCLEO_L476RG, GCC_ARM)
Scan: mbed-os-example-bootloader
Compile [  0.1%]: except.S
Compile [  0.2%]: mbed_tz_context.c
Compile [  0.4%]: mbed_fault_handler.c
Compile [  0.5%]: at24mac.cpp
Compile [  0.6%]: MCR20Drv.c
Compile [  0.7%]: NanostackRfPhyAtmel.cpp
Compile [  0.8%]: main.cpp
[Fatal Error] main.cpp@2,27: SDBlockDevice.h: No such file or directory
[ERROR] .\main.cpp:2:27: fatal error: SDBlockDevice.h: No such file or directory
 #include "SDBlockDevice.h"
                           ^
compilation terminated.

[mbed]** ERROR: "c:\python27\python.exe" returned error.
       Code: 1
       Path: "C:\Users\p\Desktop\workspace\mbed-os-example-bootloader"
       Command: "c:\python27\python.exe -u C:\Users\p\Desktop\workspace\mbed-os-example-bootloader\mbed-os\tools\make.py -t GCC_ARM -m NUCLEO_L476RG --source . --build .\BUILD\NUCLEO_L476RG\GCC_ARM"
       Tip: You could retry the last command with "-v" flag for verbose output
---
PS C:\Users\p\Desktop\workspace\mbed-os-example-bootloader>`

When I build program in Mbed Studio I got this same error

Building project mbed-os-example-bootloader (NUCLEO_L476RG, ARMC6)
Scan: mbed-os-example-bootloader
Compile [  5.8%]: CellularLog.cpp
Compile [  5.9%]: AT_ControlPlane_netif.cpp
Compile [  6.0%]: AT_CellularStack.cpp
Compile [  6.1%]: CellularUtil.cpp
Compile [  6.2%]: CellularContext.cpp
Compile [  6.3%]: CellularDevice.cpp
Compile [  6.5%]: CellularStateMachine.cpp
Compile [  6.6%]: GEMALTO_CINTERION_CellularInformation.cpp
Compile [  6.7%]: GEMALTO_CINTERION.cpp
Compile [  6.8%]: GEMALTO_CINTERION_CellularContext.cpp
Compile [  6.9%]: main.cpp
[ERROR] .\main.cpp:2:10: fatal error: 'SDBlockDevice.h' file not found
#include "SDBlockDevice.h"
^~~~~~~~~~~~~~~~~
1 error generated.

This is simple example program from github, I tryed on NUCLEO_476RG (my change in mbed_app.json) and NUCLEO_F429ZI.

mark-edgeworth · 2019-10-02T08:42:48Z

I've been able to repeat this - looks like an issue with this specific target as it works ok with K64F

SeppoTakalo · 2019-10-02T13:17:08Z

https://github.com/ARMmbed/mbed-os-example-bootloader#required-hardware
There is list of boards that this example is meant to work.

0xc0170 · 2019-10-18T10:11:37Z

@mark-edgeworth mark-edgeworth reopened this 16 days ago

Reopening this without any update does not help here much. How can we progress with this fix, as it seems it is our turn to help here?

madchutney · 2019-10-21T08:09:40Z

@0xc0170 The fix seems valid to me but as per my review I believe there is some work outstanding. Since the originator hasn't updated as promised we should take this on ourselves and complete the fix. I do not think closing the issue was the correct action in this circumstance.

0xc0170 · 2019-10-21T08:16:35Z

I do not think closing the issue was the correct action in this circumstance.

The issue is still opened #9949 so should not be forgotten. Only PR was closed as it's stalled. It can be reopened any time or discussion ongoing.
I would like to close this if there is no update in the next 2 weeks planned (this is documented in our workflow, we don't keep opened PRs without a progress for weeks. Closure should not be taken as "won't fix".).

0xc0170 · 2019-10-28T12:27:48Z

The referenced issue is opened, still valid bug that needs fixing and this can be reopened once updated

amq · 2019-11-10T16:42:43Z

Without this, the bootloader functionality is broken for me.

ciarmcom requested a review from a team July 13, 2019 19:00

ciarmcom added the needs: review label Jul 13, 2019

SeppoTakalo added the needs: CI label Jul 17, 2019

SeppoTakalo approved these changes Jul 22, 2019

View reviewed changes

SeppoTakalo removed the needs: CI label Jul 22, 2019

SeppoTakalo added the release-version: 5.14.0-rc1 label Jul 25, 2019

madchutney requested changes Jul 25, 2019

View reviewed changes

0xc0170 removed the release-version: 5.14.0-rc1 label Aug 26, 2019

0xc0170 closed this Sep 9, 2019

mark-edgeworth reopened this Oct 2, 2019

adbridge removed the needs: review label Oct 7, 2019

adbridge added the needs: work label Oct 7, 2019

0xc0170 closed this Oct 28, 2019

icsys-omh mentioned this pull request Aug 11, 2021

Fix expansion of application to target.restrict_size #14994

Closed

	pad_size = int(restrict_size, 0) - begin
	pad_size = int(restrict_size, 0) - begin
	notify.info(" Padding region with 0x%x bytes" % pad_size)

Add padding up to restrict_size #11043

Add padding up to restrict_size #11043

Uh oh!

Conversation

statropy commented Jul 13, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Jul 13, 2019

Uh oh!

bulislaw commented Jul 16, 2019

Uh oh!

mbed-ci commented Jul 17, 2019

Test run: FAILED

Uh oh!

mbed-ci commented Jul 19, 2019

Test run: SUCCESS

Uh oh!

madchutney commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

statropy commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeppoTakalo commented Jul 25, 2019

Uh oh!

madchutney commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madchutney commented Jul 25, 2019

Uh oh!

statropy commented Jul 25, 2019

Uh oh!

madchutney left a comment

Choose a reason for hiding this comment

Uh oh!

madchutney Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

madchutney Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

statropy commented Jul 25, 2019

Uh oh!

madchutney commented Jul 26, 2019

Uh oh!

0xc0170 commented Aug 20, 2019

Uh oh!

0xc0170 commented Aug 26, 2019

Uh oh!

pawelm87 commented Oct 2, 2019

Uh oh!

mark-edgeworth commented Oct 2, 2019

Uh oh!

SeppoTakalo commented Oct 2, 2019

Uh oh!

0xc0170 commented Oct 18, 2019

Uh oh!

madchutney commented Oct 21, 2019

Uh oh!

0xc0170 commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Oct 28, 2019

Uh oh!

amq commented Nov 10, 2019

Uh oh!

Uh oh!

madchutney commented Jul 24, 2019 •

edited

Loading

statropy commented Jul 24, 2019 •

edited

Loading

madchutney commented Jul 25, 2019 •

edited

Loading

0xc0170 commented Oct 21, 2019 •

edited

Loading