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

minimal-printf: Enable using a target configuration parameter #11891

Merged

Conversation

hugueskamba
Copy link
Collaborator

Description (required)

Summary of change (What the change is for and why)

Minimal-printf is currently enabled using a build profile extension; this is not the commonly used way to configure Mbed OS. Configuration of Mbed OS is usually done by overriding configuration parameters with a JSON file.

This PR adds a configuration parameter to enable Minimal-printf.

Documentation (Details of any document updates required)

Minimal printf README


Pull request type (required)

[] 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 (required)

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

Reviewers (optional)

@bulislaw @madchutney @evedon @rajkan01


Release Notes (required for feature/major PRs)

Summary of changes

Add ability to enable Minimal printf using configuration parammeter.
A new configuration parameter target.printf_lib has been added to enable it.

Impact of changes

Convenient and familiar way to configure Mbed OS.

Migration actions required

N/A

@ciarmcom
Copy link
Member

@hugueskamba, thank you for your changes.
@madchutney @rajkan01 @evedon @bulislaw @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 6826848 to c72e2ae Compare November 19, 2019 11:37
@kjbracey
Copy link
Contributor

Why a target option? It's not a natural fit there. Structurally it belongs in platform.

Does it make it easier to handle this in the tools?

Doing this means the build tools have to extract a config option either way, and by the time all config has been considered, we must have the library flags, I think.

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Nov 19, 2019

Why a target option? It's not a natural fit there. Structurally it belongs in platform.

Does it make it easier to handle this in the tools?

Doing this means the build tools have to extract a config option either way, and by the time all config has been considered, we must have the library flags, I think.

The other option to select library (default_lib) is also a target option so it seemed fit to add it there. Also it appears that toolchain optional arguments can only be changed if the Mbed option is a target option.

@kjbracey
Copy link
Contributor

kjbracey commented Nov 19, 2019

The other option to select library (default_lib) is also a target option so it seemed fit to add it there.

Ah, good point. I guess I'm thinking it in terms of "we're providing printf in platform", but you can equally view it as "the C library is not providing printf", so it's more of a C library option.

But on the other hand, microlib is selected via a -t, not JSON (default_lib is not a target config option - an app cannot modify target.default_lib, I believe), so this ends up as neither one thing nor the other.

There is a layering distinction here - there are build system options in target.json, and there are code config options. default_lib is a build option. Having an mbed config option that controls the build is a bit wonky. Do we have any precedent for it?

Need more input from tools team here - they'd know better how they want this to be structured, and what's important to them w.r.t. any future plans.

@40Grit
Copy link

40Grit commented Nov 19, 2019

Maybe we all have had it wrong this whole time and the c library should be brought in as just another external dependency...

I kid but it almost feels right.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2019

As said above, this is not target attribute but rather an application - minimal-printf should work for any and does not need any target code changes.

default_lib was added specifically (as I recall) for a online compiler as some targets did not back then support ARM (smaller targets only supported uARM or sometimes both but uARM was better choice for some). If -t is not provided for building, default_toolchain is used - I think this is how online compiler does it as well.

Let's wait for tools team review

@bulislaw
Copy link
Member

It doesn't fit as a target description as it's not a characteristic of HW platform. And I wouldn't like it to be different, by default, for different HW based on target.json entry. It's an user defined configuration option and should be set in mbed_app.json as mbed config. In the future we may connect it with bare metal, but it still a app option (except cases when a board only supports bare metal).

tools/toolchains/arm.py Outdated Show resolved Hide resolved
tools/toolchains/gcc.py Show resolved Hide resolved
tools/test/toolchains/test_toolchains.py Outdated Show resolved Hide resolved
tools/test/toolchains/test_toolchains.py Outdated Show resolved Hide resolved
@evedon
Copy link
Contributor

evedon commented Nov 19, 2019

The other option to select library (default_lib) is also a target option so it seemed fit to add it there.

But on the other hand, microlib is selected via a -t, not JSON (default_lib is not a target config option - an app cannot modify target.default_lib, I believe), so this ends up as neither one thing nor the other.

This PR is coming ahead of another PR we are working on which enables microlib using default_lib for ARM toolchain. default_lib is currently used to select newlib-nano when using GCC_ARM toolchain and we are extending the same mechanism to ARM.

This PR provides a similar mechanism for the selection of minimal-printf i.e. via a a target option.

I agree that this should be a mbed config option rather than a target option. But is it possible?
As @hugueskamba noted:

Also it appears that toolchain optional arguments can only be changed if the Mbed option is a target option.

@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch 3 times, most recently from c3442d3 to 074bab3 Compare November 19, 2019 15:25
@madchutney
Copy link
Contributor

This isn't the best place to review the architecture of Mbed 6 and I think we should speak to the architects guiding these changes.

I believe this change is following the change in approach to the microlib, it would be wise to follow suit with minimal printf. The plans there, as I understand them are as follows:

  • We control whether or not a target board works with an RTOS using:
    • Current: targets.json release_versions list e.g Mbed OS 5.x (use RTOS) or Mbed 2 (no RTOS)
    • Proposed: mbed_app.json adds required: ["baremetal"]
    • Issues: In the new system there is no way to indicate whether or not a board can support an RTOS.
  • We control whether or not a target board can support a standard library using:
    • Current: Mbed OS supported_toolchains "ARM" or "uARM"
    • Proposed System: targets.json default_lib setting, overridable in mbed_app.json
    • Issues: There will be a discrepancy between the boards which support building with microlib depending on whether the uArm toolchain method is used or the new default_lib system is used.
  • We control whether or not a target board is built with a standard library using:
    • Current: Mbed CLI command line toolchain "ARM" or "uARM"
    • Proposed: mbed_app.json default_lib setting.
    • Issues: In the new system there is no way to indicate whether or not a target board can only be built with microlib.

@evedon
Copy link
Contributor

evedon commented Nov 20, 2019

This isn't the best place to review the architecture of Mbed 6 and I think we should speak to the architects guiding these changes.

I will provide some comments here but this PR is not the correct place for this discussion.

I believe this change is following the change in approach to the microlib, it would be wise to follow suit with minimal printf.

This is correct. We will be introducing a change in the coming days to build microlib with the ARM toolchain, not the uARM toolchain which will be deprecated as per requirements.
The approach chosen and agreed with @bulislaw is to align the different toolchains by extending the current solution used to build with newlib-nano to microlib. To build with newlib-nano, the application overrides target.default_lib in mbed_app.json by specifiying small.

In this PR, we want to provide a similar mechanism for minimal-printf which is currently built using an extended profile mbed compile -t <toolchain> -m <target> --profile release --profile mbed-os/tools/profiles/extensions/minimal-printf.json

The plans there, as I understand them are as follows:

  • We control whether or not a target board works with an RTOS using:

    • Current: targets.json release_versions list e.g Mbed OS 5.x (use RTOS) or Mbed 2 (no RTOS)
    • Proposed: mbed_app.json adds required: ["baremetal"]
    • Issues: In the new system there is no way to indicate whether or not a board can support an RTOS.

To clarify, what you list under Proposed is actually Current; this is the existing mechanism to build bare metal profile in Mbed OS 5 and we are not modifying that.

You are correct about the issue though, there is no way to indicate whether or not a board can support a RTOS in terms or memory requirements or otherwise. There is work planned for next PI to port some Mbed 2 targets (particularly those which could not be ported to Mbed OS 5 due to memory limitations) to the bare metal profile. Part of this work should be to identify the HW requirements (and introduce a new target config parameter).

  • We control whether or not a target board can support a standard library using:

    • Current: Mbed OS supported_toolchains "ARM" or "uARM"
    • Proposed System: targets.json default_lib setting, overridable in mbed_app.json
    • Issues: There will be a discrepancy between the boards which support building with microlib depending on whether the uArm toolchain method is used or the new default_lib system is used.

The uARM toolchain is a hack ; we need to deprecate it. The current solution only applies to ARM toolchain. It does not cover GCC_ARM or IAR.

The upcoming PR will provide a solution to control whether "default_lib" : "small" is enabled for a target board (this is dependent on the target scatter file being modified for the ARM toolchain). The solution can be generic and be extended to all toolchains.

As you noted, there will be a discrepancy between the boards which support building with microlib depending on whether the uArm toolchain method is used or the new default_lib system is used because we do not have the capacity to update all targets at this time. I also think that we should limit our work to the golden targets and ask partners/community support for the other targets. In any case the discrepancy will be resolved when the uARM toolchain is removed.

  • We control whether or not a target board is built with a standard library using:

    • Current: Mbed CLI command line toolchain "ARM" or "uARM"
    • Proposed: mbed_app.json default_lib setting.
    • Issues: In the new system there is no way to indicate whether or not a target board can only be built with microlib.

This currently does not apply to GCC_ARM or IAR. For GCC_ARM, newlib or newlib-nano is chosen according to the default_lib setting.
There are ways to fix the issue you noted though. For example, we could introduce a new target config parameter and list all supported c libraries "supported_c_libs" : ["ARM std", "ARM microlib", "newlib", "newlib-nano", "IAR std"]. I am not proposing to do the change now as it needs to be prioritised and a proper solution needs to be agreed with the architects and your team.

@bulislaw
Copy link
Member

You are correct about the issue though, there is no way to indicate whether or not a board can support a RTOS in terms or memory requirements or otherwise. There is work planned for next PI to port some Mbed 2 targets (particularly those which could not be ported to Mbed OS 5 due to memory limitations) to the bare metal profile. Part of this work should be to identify the HW requirements (and introduce a new target config parameter).

There's no special HW features that boards need to have to support RTOS, it's all about available memory. In theory even very small board 4-8K could work with RTOS if it would be configured for this specific usecase. What we are talking about is the default, we need to provide some sort of configuration that works OOB and for some boards that would need to be RTOS-less.

@hugueskamba
Copy link
Collaborator Author

@madchutney anymore to do regarding this PR?

Copy link
Contributor

@madchutney madchutney left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, one more suggestion.

tools/toolchains/mbed_toolchain.py Outdated Show resolved Hide resolved
@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 074bab3 to 5dd0401 Compare November 26, 2019 07:40
@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 5dd0401 to 82e1ddb Compare November 28, 2019 12:02
@hugueskamba
Copy link
Collaborator Author

This force-push fixes the indentation.

@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 82e1ddb to 5df55ce Compare November 29, 2019 12:55
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 2, 2019

Also it appears that toolchain optional arguments can only be changed if the Mbed option is a target option.

Because of this we are sticking to target.printf_lib ? 🙄 Would it take that long to fix the tools?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 2, 2019

Started CI to get early results

@hugueskamba
Copy link
Collaborator Author

Also it appears that toolchain optional arguments can only be changed if the Mbed option is a target option.

Because of this we are sticking to target.printf_lib ? Would it take that long to fix the tools?

Yes, it was deemed to be a lot of work for a tool we are replacing "soon".

@mbed-ci
Copy link

mbed-ci commented Dec 2, 2019

Test run: SUCCESS

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

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

0xc0170 commented Dec 3, 2019

@evedon Happy with the current proposal ? Otherwise this is good to go in.

@evedon
Copy link
Contributor

evedon commented Dec 3, 2019

I am fine with the code change but we need to update minimal-printf README to give instructions on the alternative way of using the library.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2019

I am fine with the code change but we need to update minimal-printf README to give instructions on the alternative way of using the library.

@hugueskamba Part of this PR or is this in the docs?

@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 5df55ce to 893d2bd Compare December 3, 2019 12:29
@hugueskamba
Copy link
Collaborator Author

I am fine with the code change but we need to update minimal-printf README to give instructions on the alternative way of using the library.

@hugueskamba Part of this PR or is this in the docs?

@0xc0170 Done.

@hugueskamba hugueskamba force-pushed the hk-enable-minimal-printf-in-tools branch from 893d2bd to d5aef28 Compare December 3, 2019 12:32
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Dec 3, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Dec 4, 2019
@0xc0170 0xc0170 merged commit 2cf56b8 into ARMmbed:master Dec 4, 2019
@hugueskamba hugueskamba deleted the hk-enable-minimal-printf-in-tools branch December 4, 2019 09:25
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.

Is that documented anywhere in the handbook?

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.

9 participants