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

tools: support cases where bootloader is in chunks #9317

Merged
merged 2 commits into from Feb 27, 2019

Conversation

Projects
None yet
10 participants
@naveenkaje
Copy link
Contributor

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

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
Copy link
Contributor

left a comment

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

tools/config/__init__.py Outdated
@@ -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):

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Jan 10, 2019

Contributor

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.

tools/config/__init__.py Outdated
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 comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Jan 10, 2019

Contributor

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.

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

@naveenkaje naveenkaje force-pushed the naveenkaje:tools_bootloader_script branch Jan 17, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Build log from various build configurations attached

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

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@theotherjimmy Can you please review the latest changes

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@LiyouZhou might be worth having a look?

@theotherjimmy
Copy link
Contributor

left a comment

Style and UI nits.

tools/config/__init__.py Outdated
else:
break
if end_address == None:
raise ConfigException("bootloader segments don't fit within rom region")

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 6, 2019

Contributor

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")
tools/config/__init__.py Outdated
else:
_, end_address = part.segments()
if (end_address > rom_end):
raise ConfigException("bootloader segments don't fit within rom region")

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 6, 2019

Contributor

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")
tools/config/__init__.py Outdated
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)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 6, 2019

Contributor

Could you re-indent this line?

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Feb 10, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 11, 2019

Contributor

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

tools/config/__init__.py Outdated
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 comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 6, 2019

Contributor

This indentation looks off. Could you re-indent?

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Feb 11, 2019

Author Contributor

updated indentation in the new patch-set.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

@LiyouZhou

This comment has been minimized.

Copy link

commented Feb 7, 2019

can the header be moved freely as well?

@LiyouZhou

This comment has been minimized.

Copy link

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

This comment has been minimized.

Copy link

commented Feb 7, 2019

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

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

@naveenkaje naveenkaje force-pushed the naveenkaje:tools_bootloader_script branch 2 times, most recently Feb 10, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

Build logs from

  1. mbed-bootlader and
  2. 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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@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?

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@naveenkaje Any update for the reviews?

@naveenkaje naveenkaje force-pushed the naveenkaje:tools_bootloader_script branch Feb 20, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

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
Copy link
Contributor

left a comment

Love that new log output! So slick! 😄

I have a few concerns about the _get_end_address function.

tools/config/__init__.py Outdated
end_address = rom_end

for s, e in region_list:
if start_address < s and start_address < e:

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 20, 2019

Contributor

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

tools/config/__init__.py Outdated
"""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

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 20, 2019

Contributor

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.

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Feb 21, 2019

Author Contributor

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

tools/config/__init__.py Outdated
for s, e in region_list:
if start_address < s and start_address < e:
end_address = s
return end_address

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 20, 2019

Contributor

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

@naveenkaje naveenkaje force-pushed the naveenkaje:tools_bootloader_script branch Feb 21, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

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

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

naveenkaje added some commits Feb 10, 2019

tools: support cases where bootloader is in chunks
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.
tools: build_api_test: Add tests to verify the processing of bootload…
…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 naveenkaje force-pushed the naveenkaje:tools_bootloader_script branch to a47cfd4 Feb 21, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

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

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

@cmonr cmonr requested review from bridadan and theotherjimmy Feb 25, 2019

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

CI started

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

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Started CI prematurely. Need IAR8 work to land first.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 27, 2019

Test run: SUCCESS

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

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

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

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9201 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.