tools: support cases where bootloader is in chunks #9317

naveenkaje · 2019-01-09T22:07:39Z

Bootloader can be placed beyond the application. Support this requirement.
With FEATURE_BOOTLOADER support, the bootloader can be placed at a
high address. Add support to the tools so that Application can be placed in
the available space before the beginning of the Bootloader.

Pull request type

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

naveenkaje · 2019-01-09T22:41:55Z

Tested with cloud client.

Log from build where BOOTLOADER for NRF52 is placed at 0x64000

0 2408
409600 478028
New offset for placing application 4096 last_address 409600 size 405504
rom_end 524288 rom_end - start 520192
Using ROM regions bootloader, header, application in this build.
  Region bootloader: size 0x75000, offset 0x0
  Region header: size 0x70, offset 0x75000
  Region application: size 0x63000, offset 0x1000

output_bl_64000.txt

Log with configuration where the bootloader, header and application are linearly placed

New offset for placing application 73856 last_address 524288 size 450432
rom_end 524288 rom_end - start 450432
Using ROM regions bootloader, header, application in this build.
  Region bootloader: size 0x12000, offset 0x0
  Region header: size 0x70, offset 0x12000
  Region application: size 0x6df80, offset 0x12080

LinearPlacing.txt

theotherjimmy

I think this approach, allowing regions within a build to overlap, will prevent us from checking conditions that should fail. Instead, we could create a way to have multiple regions refer to the same file, and keep these checks

theotherjimmy · 2019-01-10T17:06:52Z

tools/config/__init__.py

@@ -769,9 +769,9 @@ def _make_header_region(self, start, header_format, offset=None):
        return (start, region)

    @staticmethod
-    def _assign_new_offset(rom_start, start, new_offset, region_name):
+    def _assign_new_offset(rom_start, start, new_offset, region_name, override=False):


So:

Adding override=True skips the check in this function

This function does rom_start + new_offset and an overflow check

I think adding override to this function misses the point: this function exists for combining the overflow check with the new offset calculation.

theotherjimmy · 2019-01-10T17:20:16Z

tools/config/__init__.py

                start = self._assign_new_offset(
-                    rom_start, start, self.target.app_offset, "application")
-            yield Region("application", start, rom_end - start,
+                    rom_start, start, self.target.app_offset, "application", True)


This change will not protect users from the following scenario, which it was originally designed for:
Say you have the following:

A 16 K BL, end address is 0x4000

Application configures it's offset to 0x3000

This situation should result in an error. With this change, it might raise an exception from IntelHex which will not help the user. I would like to keep this deterministic error message.

naveenkaje · 2019-01-17T22:52:51Z

Build log from various build configurations attached

app at 0x16000, mbed-bootloader (at 0x64000)
bootloader and app are contiguous
Trying to place application within bootloader space. Should fail
BuildLogs.txt

adbridge · 2019-02-06T11:54:13Z

@theotherjimmy Can you please review the latest changes

adbridge · 2019-02-06T11:55:41Z

@bridadan @JanneKiiskila would you guys also like to review ?

JanneKiiskila · 2019-02-06T11:57:24Z

@LiyouZhou might be worth having a look?

theotherjimmy

Style and UI nits.

theotherjimmy · 2019-02-06T14:33:28Z

tools/config/__init__.py

+                    else:
+                        break
+                    if end_address == None:
+                        raise ConfigException("bootloader segments don't fit within rom region")


Please remove "region" from this exception, as ROM is not a region:

Suggested change

raise ConfigException("bootloader segments don't fit within rom region")

raise ConfigException("bootloader segments don't fit within rom")

theotherjimmy · 2019-02-06T14:33:54Z

tools/config/__init__.py

+            else:
+                _, end_address = part.segments()
+                if (end_address > rom_end):
+                    raise ConfigException("bootloader segments don't fit within rom region")


Please remove "region" from this exception, as ROM is not a region:

Suggested change

raise ConfigException("bootloader segments don't fit within rom region")

raise ConfigException("bootloader segments don't fit within rom")

theotherjimmy · 2019-02-06T14:34:07Z

tools/config/__init__.py

+                if (end_address > rom_end):
+                    raise ConfigException("bootloader segments don't fit within rom region")
+                part_size = Config._align_ceiling(end_address, self.sectors) - rom_start
+                yield Region("bootloader", rom_start, part_size, False,
                         filename)


Could you re-indent this line?

bootloader regions are generated (bootloader0, bootloader1, so on) within the loop. I hope that's clear. Added a comment.

The indentation is off. I'm just asking you to line up filename with the fist " in the previous line.

theotherjimmy · 2019-02-06T14:34:34Z

tools/config/__init__.py

+                        raise ConfigException("bootloader segments don't fit within rom region")
+                    part_size = Config._align_ceiling(end_address, self.sectors) - rom_start
+                    yield Region("bootloader"+`part_count`, start, part_size, False,
+                             filename)


This indentation looks off. Could you re-indent?

updated indentation in the new patch-set.

theotherjimmy · 2019-02-06T14:54:58Z

@naveenkaje Do you think we could have a few tests of the behavior we're expecting here?

LiyouZhou · 2019-02-07T11:48:15Z

can the header be moved freely as well?

LiyouZhou · 2019-02-07T11:50:06Z

Tested with cloud client.

Log from build where BOOTLOADER for NRF52 is placed at 0x64000

0 2408
409600 478028
New offset for placing application 4096 last_address 409600 size 405504
rom_end 524288 rom_end - start 520192
Using ROM regions bootloader, header, application in this build.
  Region bootloader: size 0x75000, offset 0x0
  Region header: size 0x70, offset 0x75000
  Region application: size 0x63000, offset 0x1000

why does it say here region bootloader is at offset 0x0 and if what is printed here is correct, it looks like the application overlapped with bootloader and the bootloader is not at the offset you say it is.

theotherjimmy · 2019-02-07T14:58:54Z

why does it say here region bootloader is at offset 0x0

That's pretty straight forward: because you have part of your bootloader at 0. You may call it the MBR, but the tools don't know (or care) about that naming convention.

and if what is printed here is correct, it looks like the application overlapped with bootloader

@naveenkaje I was under the impression that this would generate more than one bootloader region. If this is true then @LiyouZhou are you using the code from the most recent version of this branch?

and the bootloader is not at the offset you say it is

This is addressed above.

LiyouZhou · 2019-02-07T15:19:29Z

Oh I was just reading this output from the top of this PR.

theotherjimmy · 2019-02-07T15:20:47Z

@naveenkaje Could you show us what the new version looks like?

naveenkaje · 2019-02-10T21:00:07Z

Build logs from

mbed-bootlader and
client-lite built with the bootloader binary built in step 1

Bootloader at 0x60000 with MBR
Scan: mbed-bootloader
Scan: env
Using ROM regions bootloader, application in this build.
  Region bootloader: size 0x1000, offset 0x0
  Region application: size 0x20000, offset 0x60000
....

mbed-client placed at 0x30000. Overlap is expected.

Building project client-lite (NRF52_DK, GCC_ARM)
Scan: client-lite
Using ROM regions bootloader1, bootloader2, header, application in this build.
  Region bootloader1: size 0x1000, offset 0x0
  Region bootloader2: size 0x68000, offset 0x60000
  Region header: size 0x70, offset 0x68000
  Region application: size 0x50000, offset 0x30000
Compile [  0.1%]: m2mtimer.cpp
...
Link: client-lite_application
Elf2Bin: client-lite_application
Merging Regions
  Filling region bootloader1 with /home/navkaj01/code/client-lite/configs/../tools/mbed-bootloader-nrf52_dk.hex
  Filling region header with ./BUILD/NRF52_DK/GCC_ARM/client-lite_header.hex
  Filling region application with ./BUILD/NRF52_DK/GCC_ARM/client-lite_application.hex
[ERROR] Data overlapped at address 0x60000
[mbed] ERROR: "/usr/bin/python" returned error.
       Code: 1
       Path: "/home/navkaj01/code/client-lite"
       Command: "/usr/bin/python -u /home/navkaj01/code/client-lite/mbed-os/tools/make.py -t GCC_ARM -m NRF52_DK --source . --build ./BUILD/NRF52_DK/GCC_ARM -c --app-config configs/wifi_esp8266_v4.json"
       Tip: You could retry the last command with "-v" flag for verbose output

mbed-client placed now at 0x10000, no overlap.

Building project client-lite (NRF52_DK, GCC_ARM)
Scan: client-lite
Using ROM regions bootloader1, bootloader2, header, application in this build.
  Region bootloader1: size 0x1000, offset 0x0
  Region bootloader2: size 0x68000, offset 0x60000
  Region header: size 0x70, offset 0x68000
  Region application: size 0x70000, offset 0x10000
Compile [  0.1%]: m2mtimer.cpp
Compile [  0.2%]: protoman_error_parser.c
<snip>
Link: client-lite_application
Elf2Bin: client-lite_application
Merging Regions
  Filling region bootloader1 with /home/navkaj01/code/client-lite/configs/../tools/mbed-bootloader-nrf52_dk.hex
  Filling region header with ./BUILD/NRF52_DK/GCC_ARM/client-lite_header.hex
  Filling region application with ./BUILD/NRF52_DK/GCC_ARM/client-lite_application.hex
Space used after regions merged: 0x68070
Merging Regions
  Filling region application with ./BUILD/NRF52_DK/GCC_ARM/client-lite_application.hex
Space used after regions merged: 0x3a590
| Module                              |           .text |       .data |          .bss |
|-------------------------------------|-----------------|-------------|---------------|
| [fill]                              |       438(+438) |     24(+24) |     127(+127) |
| [lib]/c.a                           |   50523(+50523) | 2474(+2474) |       89(+89) |
| [lib]/gcc.a                         |     4148(+4148) |       0(+0) |         0(+0) |
| [lib]/m.a                           |           8(+8) |       0(+0) |         0(+0) |
| [lib]/misc                          |       208(+208) |     12(+12) |       28(+28) |
| [lib]/nosys.a                       |         32(+32) |       0(+0) |         0(+0) |
| [lib]/stdc++.a                      |           1(+1) |       0(+0) |         0(+0) |
| main.o                              |     3089(+3089) |       4(+4) |       36(+36) |
| mbed-cloud-client/mbed-client       |   43558(+43558) |     29(+29) |       54(+54) |
| mbed-cloud-client/source            |     5154(+5154) |       9(+9) |       20(+20) |
| mbed-cloud-client/update-client-hub |   18005(+18005) |   232(+232) |   5150(+5150) |
| mbed-os/cmsis                       |     1033(+1033) |       0(+0) |       84(+84) |
| mbed-os/components                  |   12429(+12429) |       0(+0) |   2928(+2928) |
| mbed-os/drivers                     |     4489(+4489) |       0(+0) |       48(+48) |
| mbed-os/events                      |     1897(+1897) |       0(+0) |   2180(+2180) |
| mbed-os/features                    |   54861(+54861) |   157(+157) |     937(+937) |
| mbed-os/hal                         |     1859(+1859) |       9(+9) |     128(+128) |
| mbed-os/platform                    |     5331(+5331) |   276(+276) |   2705(+2705) |
| mbed-os/rtos                        |   12460(+12460) |   168(+168) |   6041(+6041) |
| mbed-os/targets                     |   14160(+14160) |     49(+49) |     726(+726) |
| mbed_cloud_dev_credentials.o        |       309(+309) |       0(+0) |         0(+0) |
| source/application_init.o           |       104(+104) |       0(+0) |         0(+0) |
| source/blinky.o                     |     1107(+1107) |       1(+1) |         4(+4) |
| source/oma_lwm2m_object_defs.o      |       338(+338) |       0(+0) |         0(+0) |
| source/platform                     |     1363(+1363) |       4(+4) |     103(+103) |
| source/resource.o                   |       300(+300) |       0(+0) |         0(+0) |
| update_default_resources.o          |         41(+41) |       0(+0) |         0(+0) |
| update_ui_example.o                 |       282(+282) |       0(+0) |         4(+4) |
| Subtotals                           | 237527(+237527) | 3448(+3448) | 21392(+21392) |
Total Static RAM memory (data + bss): 24840(+24840) bytes
Total Flash memory (text + data): 240975(+240975) bytes

naveenkaje · 2019-02-10T21:01:20Z

@naveenkaje Could you show us what the new version looks like?

@theotherjimmy @LiyouZhou
I posted build logs from various scenarios. Let me know if you want me to try any other combinations.

theotherjimmy · 2019-02-11T14:57:29Z

@naveenkaje It looks like those are now out of order. Could we sort them by start address before printing? Also, it looks like we don't print "bootloader2" ever when merging at the end. Could we print skipped as it's already present or something?

bridadan

The code is looking good so far! And thanks very much for the test cases.

Your last log output is confusing since the offset+sizes seem to overlap. Perhaps the sizes aren't being computed correctly anymore?

0xc0170 · 2019-02-20T14:23:33Z

@naveenkaje Any update for the reviews?

naveenkaje · 2019-02-20T22:03:40Z

The code is looking good so far! And thanks very much for the test cases.

Your last log output is confusing since the offset+sizes seem to overlap. Perhaps the sizes aren't being computed correctly anymore?

Added a patch set to compute the size of the application region. With this change, sample output from before looks like this

Scan: client-lite                                                                            
Using ROM regions bootloader1, application, bootloader2, header in this build.               
  Region bootloader1: size 0x1000, offset 0x0                                                
  Region application: size 0x50000, offset 0x10000                                           
  Region bootloader2: size 0x68000, offset 0x60000                                           
  Region header: size 0x70, offset 0x68000                                                   
Compile [  0.1%]: m2mtimer.cpp                                                               
Compile [  0.2%]: m2mtimerpimpl.cpp

bridadan

Love that new log output! So slick! 😄

I have a few concerns about the _get_end_address function.

bridadan · 2019-02-20T22:25:21Z

tools/config/__init__.py

+        end_address = rom_end
+
+        for s, e in region_list:
+            if start_address < s and start_address < e:


Wouldn't the start_address < e be unnecessary? The "end" address (e) should always be greater than the "start" address (s) right?

bridadan · 2019-02-20T22:27:17Z

tools/config/__init__.py

+        """Given a start address and set of regions, compute
+        the end address. The end address is either
+        rom_end or beginning of next section, whichever
+        is smaller


We should document that we expect the region_list to be sorted based on start address. OR you can always sort the regions in this function to ensure that.

Good point. Uploaded a patch set with suggested changes. Please re-review

bridadan · 2019-02-20T22:28:22Z

tools/config/__init__.py

+        for s, e in region_list:
+            if start_address < s and start_address < e:
+                end_address = s
+        return end_address


This will continue looping even once the valid region is found, I think you want something closer to this:

for s, e in region_list if start_address < s: return s return end_address

cmonr · 2019-02-21T16:29:08Z

@theotherjimmy @bridadan Mind re-reviewing? Looks like all comments were addressed.

Support the requirement where bootloader can be in chunks and enable placing the application at a particular offset specified by config. With FEATURE_BOOTLOADER support, the bootloader can be placed at a high address. Add support to the tools so that application can be placed in the available space before the beginning of the bootloader.

…er images Add tests to 1. Verify that a ConfigException is generated if application is placed within the bootloader region 2. Verify that a ConfigException is generated if bootloader segments don't fit witin rom.

naveenkaje · 2019-02-25T00:57:51Z

added _get_end_address helper method to compute the size of the regions and print the addresses in a sorted manner in the log as per offline discussion I had with Brian

bulislaw · 2019-02-26T15:31:09Z

This PR is at risk of missing 5.12 release as it's marked as "needs: review". Code freeze is coming! On Friday 1st. Please review the changes ASAP.

@ARMmbed/mbed-os-tools

All commennts addressed from brian's review.

cmonr · 2019-02-26T17:30:18Z

CI started

mbed-ci · 2019-02-26T17:33:41Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

cmonr · 2019-02-26T17:34:22Z

Started CI prematurely. Need IAR8 work to land first.

cmonr · 2019-02-26T21:53:35Z

CI started

mbed-ci · 2019-02-27T01:10:56Z

Test run: SUCCESS

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

cmonr requested review from theotherjimmy, bridadan and JanneKiiskila January 10, 2019 06:33

cmonr added needs: review release-version: 5.12.0-rc1 labels Jan 10, 2019

theotherjimmy suggested changes Jan 10, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 15, 2019

naveenkaje force-pushed the tools_bootloader_script branch from 375a0ef to 42b38b6 Compare January 17, 2019 22:48

cmonr added needs: review and removed needs: work labels Jan 18, 2019

theotherjimmy suggested changes Feb 6, 2019

View reviewed changes

naveenkaje force-pushed the tools_bootloader_script branch 2 times, most recently from f6d6c30 to 501fb5f Compare February 10, 2019 18:38

0xc0170 added needs: work and removed needs: review labels Feb 11, 2019

bridadan suggested changes Feb 14, 2019

View reviewed changes

naveenkaje force-pushed the tools_bootloader_script branch from d85b9e9 to 9d3dfc2 Compare February 20, 2019 22:01

bridadan previously requested changes Feb 20, 2019

View reviewed changes

naveenkaje force-pushed the tools_bootloader_script branch from 9d3dfc2 to 4e880e5 Compare February 21, 2019 15:25

cmonr added needs: review and removed needs: work labels Feb 21, 2019

Naveen Kaje added 2 commits February 21, 2019 15:11

naveenkaje force-pushed the tools_bootloader_script branch from 4e880e5 to a47cfd4 Compare February 21, 2019 21:12

0xc0170 added the risk: A label Feb 25, 2019

cmonr requested review from bridadan and theotherjimmy February 25, 2019 17:33

theotherjimmy approved these changes Feb 26, 2019

View reviewed changes

cmonr added needs: CI and removed needs: review labels Feb 26, 2019

cmonr added ready for merge and removed needs: CI labels Feb 27, 2019

cmonr merged commit e77f03c into ARMmbed:master Feb 27, 2019

cmonr removed the ready for merge label Feb 27, 2019

	raise ConfigException("bootloader segments don't fit within rom region")
	raise ConfigException("bootloader segments don't fit within rom")

tools: support cases where bootloader is in chunks #9317

tools: support cases where bootloader is in chunks #9317

Uh oh!

Conversation

naveenkaje commented Jan 9, 2019

Pull request type

Uh oh!

naveenkaje commented Jan 9, 2019

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenkaje commented Jan 17, 2019

Uh oh!

adbridge commented Feb 6, 2019

Uh oh!

adbridge commented Feb 6, 2019

Uh oh!

JanneKiiskila commented Feb 6, 2019

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Feb 6, 2019

Uh oh!

LiyouZhou commented Feb 7, 2019

Uh oh!

LiyouZhou commented Feb 7, 2019

Uh oh!

theotherjimmy commented Feb 7, 2019

Uh oh!

LiyouZhou commented Feb 7, 2019

Uh oh!

theotherjimmy commented Feb 7, 2019

Uh oh!

naveenkaje commented Feb 10, 2019

Uh oh!

naveenkaje commented Feb 10, 2019

Uh oh!

theotherjimmy commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 20, 2019

Uh oh!

naveenkaje commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

theotherjimmy commented Feb 11, 2019 •

edited

Loading

naveenkaje commented Feb 20, 2019 •

edited

Loading

cmonr commented Feb 21, 2019 •

edited

Loading