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: Set default configurations to false #11450

Merged

Conversation

@hugueskamba
Copy link
Contributor

commented Sep 10, 2019

Description

Mbed OS should not require floating point in its base configuration.
This provides further code size savings out of the box.

Pull request type

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

Reviewers

@evedon @kjbracey-arm

Release Notes

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@evedon is this needed to be in 5.14? Based on the changes - adding new configuration/changing current one, this would go to 5.15.

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-minimal-printf-default-options-false branch 2 times, most recently from f6c410c to 3177671 Sep 10, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

This force-push only leaves the commit which set floating point support to false by default.

@evedon

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@evedon is this needed to be in 5.14? Based on the changes - adding new configuration/changing current one, this would go to 5.15.

We should target 5.14.0 RC2.

@@ -51,7 +51,7 @@ Minimal printf is configured by the following parameters defined in `platform/mb
}
```

By default, 64 bit integers, floating point and FILE stream printing are enabled.
By default, 64 bit integers and FILE stream printing are enabled.

This comment has been minimized.

Copy link
@evedon

evedon Sep 10, 2019

Contributor

We should remove all references to FILE stream printing since the option to disable/enable has been removed in a previous PR.
This also applies to the tables at the end of the file

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Sep 10, 2019

Author Contributor

I will have to re-build the example project and get new numbers for the first two rows in each table as the numbers do not include the enabled file stream support. But this should not be blocking the release as it can be update later.

{
"target_overrides": {
"*": {
"platform.minimal-printf-enable-floating-point": true

This comment has been minimized.

Copy link
@evedon

evedon Sep 10, 2019

Contributor

You will need to pass the option when running greentea tests
--app-config ./TESTS/minimal-printf/compliance/test_app.json

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Sep 10, 2019

Author Contributor

@jamesbeyond suggested we re-define the macro in the test main.cpp.
Though I am not sure if that helps because compilation of the minimal-printf source seems to happen before the macro re-definition in main.cpp.
In any case, the test passe regardless of the macro redefinition therefore this IMO shouldn't also block the release and can be looked at later.

@ciarmcom ciarmcom requested review from evedon, kjbracey-arm and ARMmbed/mbed-os-maintainers Sep 10, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

It seems reasonable for the default of the "minimal" to be more minimal, but this is an API change. If it doesn't land in 5.14, then I think 5.15 is too late to change it, as it will have been published with the default the other way.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Set to 5.14.0-rc2. Lets make this ready by tomorrow noon

cc @adbridge

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-minimal-printf-default-options-false branch from 3177671 to e51e56e Sep 10, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

This force-push ensures floating point support is enabled for the Greentea test. It also modifies the minimal-printf README to remove mentions of an optional file stream support.

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-minimal-printf-default-options-false branch from e51e56e to 2195c4b Sep 11, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

This force-push removes the macro re-definition as it does not help compiling minimal-printf with floating point support since the re-definition only happened later on.
The test does not fail, however, the side effect is that the minimal-printf floating point tests are not run.
The bigger question here (not just related to minimal-printf) is "how do we run tests for configuration options that are disabled by default?". There needs to be a generic way to test features that are off by default.
@jamesbeyond

Mbed OS should not require floating point in its base configuration.
This provides further code size savings out of the box.
@hugueskamba hugueskamba force-pushed the hugueskamba:hk-minimal-printf-default-options-false branch from 2195c4b to 03c484a Sep 11, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

This force-push updates the code size values in the minimal-printf README tables.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Started CI to get test results while we review

@0xc0170 0xc0170 removed the needs: work label Sep 11, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 11, 2019

Test run: SUCCESS

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

@evedon

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I agree that the default should be to have float support disabled.

I will create a ticket for the testing issue:

The bigger question here (not just related to minimal-printf) is "how do we run tests for configuration options that are disabled by default?". There needs to be a generic way to test features that are off by default.
@jamesbeyond

Copy link
Contributor

left a comment

I'd like one more change to the greentea test before this is merged though.
@hugueskamba please put back test_app.json and add a README file with the command line to use to test for floats

@evedon
evedon approved these changes Sep 11, 2019
Copy link
Contributor

left a comment

We will make the change to test_app.json in a different PR; it can come in 5.14.1 as well.

@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

We will make the change to test_app.json in a different PR; it can come in 5.14.1 as well.

Here is the PR #11455

@0xc0170 0xc0170 merged commit 91515fe into ARMmbed:master Sep 11, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
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 RTOS ROM(+0 bytes) RAM(+0 bytes)
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 8660 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 8464B.
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
Projects
None yet
6 participants
You can’t perform that action at this time.