-
Notifications
You must be signed in to change notification settings - Fork 3k
Bare metal mbed_lib updates #9628
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
Conversation
@orenc17, thank you for your changes. |
"storage_type": "TDB_INTERNAL" | ||
}, | ||
"NUCLEO_F411RE": { | ||
"storage_type": "TDB_INTERNAL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's was in the original mbed_lib.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeromecoutant Could you please move this comment to #9561?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Should I close the prior PR?
@theotherjimmy merge your PR first and then i'll rebase |
features/nanostack/mbed_lib.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ | |||
"name": "nanostack-upper" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll propose "nanostack-adaptation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe that this change is in @theotherjimmy original PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SeppoTakalo Actually, that's a hack that I have not yet removed. Could you comment on the appropriate folders to label such that we can completely exclude nanostack code from the build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that all folders within /features/nanostack/
should have their own mbed_lib.json files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps. It depends on what you would consider a complete "library", that could be turned on/off as one. If the code within the subdirectories in nanostack can be enabled independently, then it would make sense to have mbed_lib.json
in the root so that a user could use requires
on that library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this image help?
There are clear dependencies that can be expressed, so maybe it makes sense to all components being separate.
nanostack-eventloop
is actually used also with Mbed Client.
Couple of helper libraries also missing from the picture:
- features/frameworks/mbed-client-randlib (used by nanostack)
- features/frameworks/mbed-coap (used by coap-service and Pelion Client)
- features/frameworks/mbed-trace (used by Nanostack and Pelion Client, and various other libraries)
- features/frameworks/nanostack-libservice (used by pretty much all Nanostack libraries and even Pelion Client).
So.. after a longer thought.. it seems that "features/nanostack" should be empty, no .lib files there, and all that is inside it, should have their equivalent .lib files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SeppoTakalo @theotherjimmy Could you move this comment to #9561?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (reviewing storage related changes).
@SeppoTakalo @theotherjimmy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can approve following .lib files:
- mbed-client-cli
- mbed-client-randlib
- mbed-coap
- nanostack-libservice
- coap-service
- nanostack-interface
Technically I cannot review changes to the tooling as I have not enough knowledge about the design.
CI started |
Test run: FAILEDSummary: 3 of 8 test jobs failed Failed test jobs:
|
One of the errors:
|
CI started (for now). Still expected reviewer feedback |
@orenc17 Fyi, this will also need a |
@mbed-os-mesh @ARMmbed/mbed-os-pan @yossi2le Any thoughts? |
Test run: SUCCESSSummary: 12 of 12 test jobs passed |
I have taken a look at the changes and they look ok for me. |
Description
Create missing mbed_lib.json for the following libs:
Relays on #9561
Pull request type
Reviewers
@theotherjimmy @davidsaada @yossi2le
Release notes
Make the following modules comply with bare-metal: