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

Expose target_offset and header_offset parameters in targets.json #12081

Merged
merged 1 commit into from Jan 8, 2020

Conversation

micgur01
Copy link
Contributor

Summary of changes

The app_offset and header_offset parameters have always been a part of the target configuration parameters. However, they were intercepted by the tools and never exposed to the application. The new FOTA (update client next generation) functionality requires these parameters in the code as well now. So this PR adds them to targets.json file. The only result of this PR is the addition of these two macros into the code.
Note that the tools already expose the macros APPLICATION_ADDR and HEADER_ADDR having the same values. However, these macros are only available when the application is built with the bootloader. If it isn't, these macros aren't available, hence this PR.
This change is required for new fota client.

Impact of changes

None

Migration actions required

None

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@alzix
@donatieng

@ciarmcom ciarmcom requested review from alzix, donatieng and a team December 11, 2019 10:00
@ciarmcom
Copy link
Member

@micgur01, thank you for your changes.
@alzix @donatieng @ARMmbed/mbed-os-maintainers please review.

@bulislaw
Copy link
Member

Where are the APPLICATION_ADDR and HEADER_ADDR coming from? I can't see their definition in target.json so I assume they are defined elsewhere. Should we add the offset to the same place to keep it consistent?

@alzix
Copy link
Contributor

alzix commented Dec 11, 2019

These values (app_offset and header_offset) are typically defined in application layer in either mbed_lib.json or mbed_app.json.
Please refer to the client examples and mbed-bootloader documentation.
These values are already part of build system and consumed by config/__init__.py for the BL build, but they do not propagate to the config as they are defined at a target level.
Requesting customers to redefine these values in a config portion of our configuration tree creates duplication and is error prone.
Thus the most elegant/minimal impact solution we found is just adding them to a config so they will be exposed to application layer at compile time. We do need these in FOTA client as we need to read the metadata header.

@alzix
Copy link
Contributor

alzix commented Dec 11, 2019

@bulislaw APPLICATION_ADDR and HEADER_ADDR are not defined anywhere they are injected by the tools somewhere around in config/__init__.py::_generate_bootloader_build. Do not recall exact location. I did the case study a while ago - now we are just upstreaming our changes from the side project we were working on for past couple of months.

As mentioned in PR description the auto defined macros (APPLICATION_ADDR and HEADER_ADDR) are calculated based on app_offset and header_offset and probably mbed_rom_start. But they are injected in to build if and only if prebuilt bootloader image is found during the resource scan.
Relying on sometimes auto-generated macros produces weird build errors which a hard to understand.

Instead we have chosen to expose the the offset macros which are always present at build time via autogenerated mbed_config.h

The changes purposed in this PR have a workaround nature as they do not address original problem. But these changes allow us to move forward without introducing duplications in application level config files.

I think the whole portion of logic around bootloader builds should be carefully reviewed and may be simplified a bit.

@alzix
Copy link
Contributor

alzix commented Dec 15, 2019

@ARMmbed/mbed-os-maintainers ping

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Dec 16, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 16, 2019

Note that the tools already expose the macros APPLICATION_ADDR and HEADER_ADDR having the same values. However, these macros are only available when the application is built with the bootloader. If it isn't, these macros aren't available, hence this PR.

A lot of info above, but still dont understand how this fixes it. As this set them to null, they are not defined ? why not false ?

from config docs:

Leaving the value field undefined or setting the value field to null allows the parameter to be stored as a configuration option and appear with the mbed compile --config command; however, the key is not be defined in mbed_config.h and does not affect the application or OS unless it is overridden.

The override is a key here? So an app can overwrite these, correct?

Might be good to illustrate in app, how this PR fixes it (code snippet before after the fix).

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 3, 2020

@micgur01 Please review the above. This is the simple update but it touches all targets.

Let's have this resolved and fixed.

@alzix
Copy link
Contributor

alzix commented Jan 5, 2020

0xc0170, sorry for late response.

Yes, the whole purpose of this exercise is being able to override these values.
Please refer to https://github.com/ARMmbed/mbed-bootloader/blob/9fe306e683e28443fbdcf65d47b11ce1e9e20844/README.md#step-3-configure-the-pelion-cloud-client-application

On a custom project with custom board and perhaps different flash layout - customer is required to specify these values twice: in bootloader configurations and in the application configuration.

The idea to bring these configurations as close as possible by reusing the same configuration keys.

This PR allows to reuse defines set for the BL configuration in the application code. Without this PR we need to provide additional configurations for setting these values yet another time.
See the https://github.com/ARMmbed/mbed-cloud-client-internal/blob/master/update-client-hub/mbed_lib.json#L4 as example. In this case application_details == MBED_ROM_DTART + header_offset.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 7, 2020

@donatieng Please review

@mbed-ci
Copy link

mbed-ci commented Jan 7, 2020

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 1a6934a into ARMmbed:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants