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

Enable MTS_DRAGONFLY_F411RE to register with Pelion #10287

Merged
merged 4 commits into from May 10, 2019

Conversation

Projects
None yet
10 participants
@linlingao
Copy link
Contributor

commented Apr 1, 2019

Description

Enable MTS_DRAGONFLY_F411RE to register with Pelion

Pull request type

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

Reviewers

Release Notes

@linlingao linlingao changed the title Correct NVIC flash vector address for application Correct NVIC flash vector address for application - DO NOT MERGE YET Apr 1, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@linlingao, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-storage please review.

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 1, 2019

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 2, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Correct NVIC flash vector address for application - DO NOT MERGE YET #10287

Let us know once ready. This looks like some fixes can be sent separately (fixing target code)

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@0xc0170 The target code is ready to be merged. Since I needed the SPI changes from @bentcooke, those commits were rolled into this PR. I'm planning to wait for Ben's changes to go in first then rebase.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

What is the PR number (depending PR here) ?

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@linlingao Would you happen to know what'll happen with this PR since #10177 was closed?

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@cmonr I think @bentcooke will either reopen it or refactor the code in another PR.

@linlingao linlingao changed the title Correct NVIC flash vector address for application - DO NOT MERGE YET Correct NVIC flash vector address for application Apr 11, 2019

@linlingao linlingao added needs: review and removed needs: work labels Apr 11, 2019

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@0xc0170 @cmonr This PR is ready to be reviewed/merged.
We have received a new module with a SPIF supporting SFDP. So changes pertinent to no SFDP that were pulled in from PR10177 are no longer needed. I have therefore removed those changes.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@linlingao Mind rebasing?

@0xc0170
Copy link
Member

left a comment

Looks fine to me ,one clarification for functionality removal.

Show resolved Hide resolved targets/targets.json

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 12, 2019

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Rebased.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Can you rebase instead of merge ? The commit 849b05a is not needed here

@cmonr cmonr added the do not merge label Apr 12, 2019

@linlingao linlingao force-pushed the linlingao:pr10177 branch from 849b05a to de260bb Apr 15, 2019

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@0xc0170 @cmonr I rebased. @bentcooke would like to double check with Mutitech to make sure it's okay to remove their bootloader. Please don't merge yet.

@cmonr cmonr added needs: review and removed needs: work labels Apr 16, 2019

@bridadan
Copy link
Contributor

left a comment

Quite a nice solution! I have a few questions/suggestions below.

Show resolved Hide resolved features/lwipstack/mbed_lib.json Outdated
if hasattr(self.target, 'post_binary_hook'):
if self.target.post_binary_hook is None:
define_string = self.make_ld_define(
"DISABLE_POST_BINARY_HOOK", 1)

This comment has been minimized.

Copy link
@bridadan

bridadan May 1, 2019

Contributor

Really neat idea. I want to run a slight tweak by you and see what you think.

Naming the macro DISABLE_POST_BINARY_HOOK implies that a post binary hook is usually enabled. In general, this isn't the case, only a few targets use post binary hooks. How do you feel about changing this to macro to POST_BINARY_HOOK_ENABLED? So that would mean if post_binary_hook is not null, POST_BINARY_HOOK_ENABLED would be set to 1, otherwise it'd be set to 0?

I think this tweak makes the solution a bit more generic and potentially useful for other targets.

This comment has been minimized.

Copy link
@linlingao

linlingao May 1, 2019

Author Contributor

The reason I named it "DISABLE_POST_BINARY_HOOK" is because for those targets that have post_binary_hook, they're actually enabled in target.json. When you override it with null (uncommon case), it's becoming a "disable". I think you might come from a different perspective where most targets do not have post_binary_hook, yes?

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@felser I didn't change your bootloader. Mbed bootloader is not checked in so there's no way to include it. I'm not aware of any other bootloaders. Is it possible the MTS bootloader in the repo is out of date?

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@bridadan WRT the stack size, I knew I could override it in mbed_app.json. I did it on purpose since the stack size was okay prior to 5.12.2(?) release, but in the latest code, it's no longer enough. I would expect this issue to show up in all applications.

@felser

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Sorry, not sure what I did. Must have been "operator error"... our bootloader is working.

@felser

felser approved these changes May 1, 2019

Copy link
Contributor

left a comment

Approving the PR. Bootloader is working as expected with default settings.

@0xc0170
Copy link
Member

left a comment

Target override for lwip in targets.json file for specific target to be set

@linlingao linlingao force-pushed the linlingao:pr10177 branch from ffb3d8a to 6ac9417 May 2, 2019

@linlingao linlingao force-pushed the linlingao:pr10177 branch from 4976e67 to 84b8517 May 2, 2019

@linlingao linlingao requested a review from 0xc0170 May 2, 2019

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

I have reluctantly increased the tcpip stack just for this board. I think it would be a more proactive approach to increase the stack size for all boards so that we don't have to fix this same issue again and again. Our users tend to panic when they see a fault. Not everyone feels comfortable to go in and override the settings. Oh well, enough said.

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 3, 2019

I have reluctantly increased the tcpip stack just for this board. I think it would be a more proactive approach to increase the stack size for all boards so that we don't have to fix this same issue again and again. Our users tend to panic when they see a fault. Not everyone feels comfortable to go in and override the settings. Oh well, enough said.

@SeppoTakalo Please review the feedback

@0xc0170
Copy link
Member

left a comment

One last question about DISABLE binary hook macro:

@bridadan proposed to have POST_BINARY_HOOK_ENABLED - wouldnt it be better ? set to 0 if not enabled, 1 if enabled. The linker scripts here would use this value.

@bridadan
Copy link
Contributor

left a comment

After going over it with @linlingao, she made a good argument that this is a lower touch change. My suggestion would add a new macro to every linker script. Generally I don't like the negative macro names (DISABLE_X) with positive values (1). But in this case I think its probably fine.

@linlingao

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@0xc0170 Is there work needed from my side? Can we merge this PR?

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 10, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 10, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Restarted exporters

@adbridge adbridge added ready for merge and removed needs: CI labels May 10, 2019

@adbridge adbridge merged commit 97e1c9c into ARMmbed:master May 10, 2019

26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
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-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8601 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
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.