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

Add BOOTLOADER feature #7714

Merged
merged 3 commits into from
Aug 25, 2018
Merged

Conversation

brianesquilona
Copy link
Contributor

BOOTLOADER feature

Description

Added BOOTLOADER feature and copy bootloader binaries into mbed OS repository for supported targets

Pull request type

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

@brianesquilona
Copy link
Contributor Author

Might need to update the binaries

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

@brianesquilona Some questions:

  • Why do you think the bootloader images need to be updated?
  • Do we have a method of testing this new feature?
  • Will there be/are there docs coming in to addresshow to use the feature?

@brianesquilona
Copy link
Contributor Author

Quoting @theotherjimmy
"Add the feature BOOTLOADER into your application configuration in one of the following ways:

  1. add into mbed_app.json:
    "target_overrides": {
    "*": {
    "target.features_add": ["BOOTLOADER"]
    2.add on the command line as -C'target.features_add=["BOOTLOADER"]' after this PR (this example is in the PR description )"

I have tested the first one to work.

0xc0170
0xc0170 previously requested changes Aug 7, 2018
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binaries needs LICENSE file, please add it

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

Might need to update the binaries

I'm not worried about this, since the updates can come during a patch release.

@0xc0170 0xc0170 changed the title Added BOOTLOADER feature and copy bootloader binaries into mbed OS repository for supported targets Add BOOTLOADER feature Aug 7, 2018
@SenRamakri
Copy link
Contributor

@theotherjimmy - Should the binaries for each platform be placed under corresponding TARGET_ folder?

@SenRamakri
Copy link
Contributor

@brianesquilona - As we discussed with @theotherjimmy, current locations of those binaries are fine. So no need to change them.

@cmonr - Please see my responses to some of your questions:
Why do you think the bootloader images need to be updated? - This is part of the requirements for update feature.
Do we have a method of testing this new feature? - The binaries are already tested and copied into mbed-bootloader-binaries repo from mbed-bootloader. So I think we don't have to test them again. Let me know if you don't agree with this.

Will there be/are there docs coming in to addresshow to use the feature? - Yes, there will be update to the docs coming into Handbook repo capturing these changes. We will add you to that once its up.

@SenRamakri
Copy link
Contributor

SenRamakri commented Aug 7, 2018

@0xc0170 - bootloader binaries are copied from mbed-bootloader-binaries repo which has same license(Apache License 2.0) as mbed-os. So no need to copy the license again. Let me know if you think we still need the license.

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

@SenRamakri It's good that the licenses are somewhere, but within the context of Mbed OS, if someone was casually browsing this particular folder and used these binaries, a user would have to search through the file history to eventually find this PR to find out that the licenses exist elsewhere. I think the idea of having the licenses locally within the repo, and next to the binaries is to make it explicitly known what the user is getting.

A random example: https://github.com/ARMmbed/mbed-os/tree/646cc89a620f978ed6174cb7bbb921e0f0d910f2/targets/TARGET_ARM_SSG/TARGET_BEETLE/TOOLCHAIN_ARM_STD. Just a single search was needed to find that binary, and now I also know what kind of license it's paired with.

Do we have a method of testing this new feature?
The binaries are already tested and copied into mbed-bootloader-binaries repo from mbed-bootloader. So I think we don't have to test them again. Let me know if you don't agree with this.

I was more asking as to whether we had some sort of CI testing wrapped around this feature and/or binaries. Manual testing things and saying that they're ok gives me the jeebees, as that's just asking for issues down the road. However, if some sort of automated testing is in the pipeline or not ready yet, then I think that's not a dealbreaker fow now.

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

:+1 for referencing the docs PR.

@bulislaw
Copy link
Member

bulislaw commented Aug 8, 2018

I'm assuming that application can override the binaries with their own in different location?

@brianesquilona
Copy link
Contributor Author

Added LICENSE file

@cmonr cmonr dismissed 0xc0170’s stale review August 14, 2018 01:55

Licenses added.

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

Unless anyone else has input for this PR (@bulislaw @theotherjimmy @donatieng @SenRamakri ), I'm going to get it going in CI.

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 14, 2018

Build : SUCCESS

Build number : 2801
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7714/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theotherjimmy

@bulislaw Hex/bin files in tree are not automatically assumed to be bootloaders, and the configuration option is what selects the bootloader for a target. The mbed_lib.json is what picks the bootloader in this PR.

I don't like how we move devices specific files out of target directory. It's even more confusing that we move out the config to another file. If we want to make our system feel consistent we should be trying to minimise surprises and things to know. I don't care whether it's DEVICE_BOOTLOADER or FEATURE_BOOTLOADER, but I'd like to have it under targets/. The two things that need to be considered:

  • Bootloader has to be easily (in/ex)cluded from a build
  • There must be a way for users to provide their own binary and settings

Copy link
Contributor

@teetak01 teetak01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified to work with upcoming version of mbed-cloud-client-example.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 23, 2018

I don't like how we move devices specific files out of target directory. It's even more confusing that we move out the config to another file. If we want to make our system feel consistent we should be trying to minimise surprises and things to know. I don't care whether it's DEVICE_BOOTLOADER or FEATURE_BOOTLOADER, but I'd like to have it under targets/. The two things that need to be considered:

  • Bootloader has to be easily (in/ex)cluded from a build
  • There must be a way for users to provide their own binary and settings

Any response for this? Is this intended (the structure) for a reason as @theotherjimmy indicated earlier about bootloader being specific use case thus this tree (not in targets/ folder).

cc @donatieng

@donatieng
Copy link
Contributor

I agree with @bulislaw - even if the tools don't require it we should keep things consistent by having binaries in folders specific to targets, such as FEATURE_BOOTLOADER/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/mbed-bootloader-block_device-sotp-v3_4_0.bin.

As we move towards adding support for more targets, this will ensure future-proofness.

@donatieng
Copy link
Contributor

@brianesquilona please switch the license header to PBL.

FYI @ChiefBureaucraticOfficer

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @brianesquilona !
Let's wait for the green light regarding license but looks good. @theotherjimmy does this work for you?

@donatieng
Copy link
Contributor

donatieng commented Aug 24, 2018

Just got confirmation that this PBL is correct & appropriate so good to go from a legal standpoint 👮

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 24, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

Build : SUCCESS

Build number : 2891
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7714/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

Test : SUCCESS

Build number : 2639
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7714/2639

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 24, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

@cmonr cmonr merged commit 02c0320 into ARMmbed:master Aug 25, 2018
@cmonr cmonr removed the risk: A label Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants