Add support for nrf52_dk bootloader #8097

JammuKekkonen · 2018-09-12T11:45:36Z

Bootloader for nrf52_dk is a intel hex file, which contains write instructions to the start of rom and also into registry which is outside rom region. This causes the current logic of forming the build regions to fail as it only supports bootloader that is in one segments which is at the start of rom.

This pr fixes bootloader build region generation in case where application size is restricted (space reserved for something else after application) and also adds support for nrf52_dk bootloader, which contains write instructions to registry which is outside rom region.

Also fixed printing of error case.

Pull request type

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

…s with multiple segments, as required by NRF52 bootloader.

JammuKekkonen · 2018-09-12T11:46:04Z

@LiyouZhou
@JuhaYlipekkala
@yogpan01

yogpan01 · 2018-09-12T11:48:07Z

Approved,
@ARMmbed/mbed-os-maintainers please review.

cmonr · 2018-09-12T14:38:16Z

/morph build

theotherjimmy · 2018-09-12T14:39:49Z

tools/build_api.py

+            if len(part.segments()) == 1:
+                part_size = (part.maxaddr() - part.minaddr()) + 1
+            else:
+                # make same assumption as in region builder; first segments must fit.


I don't understand what this comment is about. Could you explain it?

theotherjimmy · 2018-09-12T14:42:13Z

tools/config/__init__.py

        return newstart

    def _generate_bootloader_build(self, rom_start, rom_size):
        start = rom_start
        rom_end = rom_start + rom_size
+        max_app_addr = -1


Please don't use -1 as a marker. Pythonic optional types are generally None or an instance of the type. Could you replace this -1 with None?

theotherjimmy · 2018-09-12T14:43:09Z

tools/config/__init__.py

+                # assume first segment is at start
+                # second at the end or outside ROM
+                # rest outside regular ROM
+                if part.segments()[0][0] != 0x0:


This is not always true; ROM does not always start at 0x0. Please compare with rom_start.

theotherjimmy · 2018-09-12T14:47:37Z

tools/config/__init__.py

+                part_size = part.segments()[0][1] - part.segments()[0][0]
+                part_size = Config._align_ceiling(rom_start + part_size, self.sectors) - rom_start
+                if part.segments()[1][0] < rom_end and self.target.restrict_size is None:
+                    if self.target.app_offset:


target.app_offset is not the only way that an application can be pushed further back: target.header_format and target.header_offset can also push an application further back. Either this check should check for all of them, or max_app_addr should be used unconditionally.

theotherjimmy · 2018-09-12T14:52:56Z

tools/config/__init__.py

@@ -722,9 +738,17 @@ def _generate_bootloader_build(self, rom_start, rom_size):
                start, region = self._make_header_region(
                    start, self.target.header_format)
                yield region._replace(filename=self.target.header_format)
+
+            if max_app_addr != -1 and self.target.restrict_size is None and not self.target.app_offset:


Why does target.app_offset inhibit the use of target.restrict_size here?

cmonr · 2018-09-12T15:10:58Z

Stopped build since changes have been requested.

cmonr · 2018-09-13T16:08:09Z

@theotherjimmy Mind doing a re-review?

theotherjimmy · 2018-09-13T18:02:51Z

tools/build_api.py

+                part_size = (part.maxaddr() - part.minaddr()) + 1
+            else:
+                # make same assumption as in region builder; first segments must fit.
+                part_size = part.segments()[0][1] - part.segments()[0][0]


Do you need the +1 here?

How is this branch different from the other branch when part.segments() == 1? Could we just use this implementation unconditionally?

theotherjimmy · 2018-09-13T18:05:11Z

tools/config/__init__.py

+                # rest outside regular ROM
+                if part.segments()[0][0] != rom_start:
+                    raise ConfigException("bootloader segments not in order")
+                part_size = part.segments()[0][1] - part.segments()[0][0]


Size is off by 1 here as well.

JammuKekkonen · 2018-09-17T13:03:25Z

PR updated according to feedback.

Tested with blinky.
K64F using existing bootloader
NRF52_DK with two different bootloaders; one that is located right after softdevice and one that is located at the end of flash (softdevice in beginning).

In both of the NRF52_DK tests, app header is located before app. @LiyouZhou.

bulislaw · 2018-11-26T14:54:23Z

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

0xc0170 · 2018-11-26T15:30:21Z

Moved to 5.11.1.

Hold a second still @0xc0170 , @simosillankorva will still check the configs.

This still needs work as I understood.

cmonr · 2018-11-26T16:02:19Z

@0xc0170 That sounds correct. Conversation has been taken offline.

cmonr · 2018-11-29T16:28:06Z

My bad. Email conversation is still ongoing.

JanneKiiskila · 2018-12-04T09:19:42Z

Internal ref: MBOTRIAGE-1871

0xc0170 · 2018-12-04T12:17:22Z

Based on the internal conversation, it was decide that this patch will go in as it is and another patch for tools will come separately

@theotherjimmy @naveenkaje Please confirm

naveenkaje · 2018-12-04T15:39:07Z

@0xc0170 Yes, that is correct. We plan to do a follow on patch to enable placing an application at an offset based on configuration parameters.

cmonr · 2018-12-04T18:37:21Z

CI started

mbed-ci · 2018-12-04T21:55:28Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 12
Build artifacts
Build logs

Fix bootloader build region generation and add support for bootloader…

4952e31

…s with multiple segments, as required by NRF52 bootloader.

cmonr requested a review from theotherjimmy September 12, 2018 13:25

cmonr added needs: review release-version: 5.10.0-rc3 labels Sep 12, 2018

cmonr approved these changes Sep 12, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Sep 12, 2018

theotherjimmy reviewed Sep 12, 2018

View reviewed changes

theotherjimmy suggested changes Sep 12, 2018

View reviewed changes

theotherjimmy added needs: work and removed needs: CI labels Sep 12, 2018

Review fixes for bootloader region generation

ae4b67e

cmonr added needs: review and removed needs: work labels Sep 13, 2018

theotherjimmy previously requested changes Sep 14, 2018

View reviewed changes

cmonr added needs: work risk: R release-version: 5.10.1 and removed needs: review risk: R release-version: 5.10.0-rc3 labels Sep 14, 2018

Review fixes 2 for bootloader region generation

c069cef

cmonr added needs: review risk: A and removed needs: CI rollup PR risk: G labels Nov 21, 2018

0xc0170 added release-version: 5.11.1 and removed release-version: 5.11.0-rc1 labels Nov 26, 2018

cmonr removed the risk: A label Nov 27, 2018

0xc0170 added needs: work and removed needs: review labels Nov 28, 2018

0xc0170 added needs: review and removed needs: work labels Dec 4, 2018

cmonr added needs: CI and removed needs: review labels Dec 4, 2018

cmonr added ready for merge and removed needs: CI labels Dec 4, 2018

cmonr merged commit a162589 into ARMmbed:master Dec 4, 2018

cmonr removed the ready for merge label Dec 4, 2018

mattbrown015 mentioned this pull request Dec 7, 2018

Build No Longer Fails if 'target.restrict_size' Exceeded #9012

Closed

This was referenced Jul 25, 2019

Bootloader Merging Regions Error #9949

Closed

Add padding up to restrict_size #11043

Closed

Add support for nrf52_dk bootloader #8097

Add support for nrf52_dk bootloader #8097

Uh oh!

Conversation

JammuKekkonen commented Sep 12, 2018 • edited by cmonr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request type

Uh oh!

JammuKekkonen commented Sep 12, 2018

Uh oh!

yogpan01 commented Sep 12, 2018

Uh oh!

cmonr commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 12, 2018

Uh oh!

cmonr commented Sep 13, 2018

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!

JammuKekkonen commented Sep 17, 2018

Uh oh!

bulislaw commented Nov 26, 2018

Uh oh!

0xc0170 commented Nov 26, 2018

Uh oh!

cmonr commented Nov 26, 2018

Uh oh!

cmonr commented Nov 29, 2018

Uh oh!

JanneKiiskila commented Dec 4, 2018

Uh oh!

0xc0170 commented Dec 4, 2018

Uh oh!

naveenkaje commented Dec 4, 2018

Uh oh!

cmonr commented Dec 4, 2018

Uh oh!

mbed-ci commented Dec 4, 2018

Uh oh!

Uh oh!

JammuKekkonen commented Sep 12, 2018 •

edited by cmonr

Loading

theotherjimmy Sep 12, 2018 •

edited

Loading