Add width modifier and prepending zeros for hexadecimal output and decimal precision for floating point #13548

EmbedEngineer · 2020-09-04T10:33:35Z

Summary of changes

Added width modifier and prepend zeros if the width modifier is given feature to printing integer, hexadecimal and floating point values. So e.g. printf("%04X", i); works now the same as the std version.

Added precision specifier for printing floating point values, as it already existed for strings. If no precision specifier is given MBED_CONF_PLATFORM_MINIMAL_PRINTF_SET_FLOATING_POINT_MAX_DECIMALS will be used.

This PR also fixes #13783.

Impact of changes

Prepending zeros and width modifier work with integer, hexadecimal and floating point values when using minimal_printf.
Also the precision modifier works for floating point variables. If no precision specifier is given MBED_CONF_PLATFORM_MINIMAL_PRINTF_SET_FLOATING_POINT_MAX_DECIMALS is used.

Migration actions required

Documentation

None

Pull request type

Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
Feature update (New feature / Functionality change / New API)
Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] 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

ciarmcom · 2020-09-04T11:00:32Z

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

0xc0170 · 2020-09-04T12:00:16Z

Please review astyle failures.

LDong-Arm · 2020-09-04T16:39:17Z

@EmbedEngineer Thanks for the contribution, the width modifier is something I've personally wanted for while.
@pan- FYI, if this gets in we should be able to switch our ble-cliapp back to minimal-printf (i.e. default) from std.

Though any feature added increases the size of minimal printf - shall we make this feature configurable? @evedon @hugueskamba

pan- · 2020-09-04T16:52:39Z

@EmbedEngineer That could be a great addition, do you have numbers in term of code size ? How much do we pay to print hexadecimal values with the width modifier ?

hugueskamba

Thank you for the changes.
How does the change affect code size?
Perhaps it should be a configurable option as we do not want to increase code size for existing users.

platform/source/minimal-printf/mbed_printf_implementation.c

evedon · 2020-09-07T09:01:30Z

Thanks for the PR. Like the others, I'd like to know the impact on code size. In addition could you add test for this? See https://github.com/ARMmbed/mbed-os/blob/master/platform/tests/TESTS/mbed_platform/minimal-printf/compliance/main.cpp

EmbedEngineer · 2020-09-07T09:17:04Z

@pan- @hugueskamba @evedon considering the size, I got 90 bytes more with 0s and arm gcc 9

LDong-Arm

Thanks @EmbedEngineer, the code looks good in general

platform/source/minimal-printf/mbed_printf_implementation.c

LDong-Arm · 2020-09-07T10:25:39Z

Thanks for the PR. Like the others, I'd like to know the impact on code size. In addition could you add test for this? See https://github.com/ARMmbed/mbed-os/blob/master/platform/tests/TESTS/mbed_platform/minimal-printf/compliance/main.cpp

@EmbedEngineer This is the test for minimal-printf. Could you please extend test_printf_x() & test_snprintf_x() (hexadecimals) and test_printf_f() & test_snprintf_f() (floating points) to test widths and decimal precisions? It should be enough to copy some existing code and replace the format strings, expected values, etc.

Then the test can be run with

mbed test -t GCC_ARM -m detect -n platform-tests-tests-mbed_platform-minimal-printf

if you have a board connected.

Thanks for the effort in so far!

EmbedEngineer · 2020-09-07T10:35:48Z

@LDong-Arm thanks.
I will work on the tests.

platform/source/minimal-printf/mbed_printf_implementation.c

EmbedEngineer · 2020-09-07T12:37:36Z

@LDong-Arm @evedon I finished the tests so far. Could you please review them?

hugueskamba

Thank you.
platform/source/minimal-printf/README.md also needs updating.

EmbedEngineer · 2020-09-08T06:25:52Z

Thank you.
platform/source/minimal-printf/README.md also needs updating.

Done. Unfortunately I can't adapt the code sizes since I neither have an ARMC6 nor an IAR workbench compiler.

LDong-Arm · 2020-09-08T10:05:14Z

@EmbedEngineer I tried it locally, and it uses an extra 1KB of flash with GCC and ARM compilers. I compared the latest master branch as of now vs this branch rebased onto latest master. All builds were clean builds.

If I got things right, then it shows we need an extra configuration to conditionally enable/disable width and precision supports.

The test looks good to me 👍

EmbedEngineer · 2020-09-08T10:40:01Z

@EmbedEngineer I tried it locally, and it uses an extra 1KB of flash with GCC and ARM compilers. I compared the latest master branch as of now vs this branch rebased onto latest master. All builds were clean builds.

If I got things right, then it shows we need an extra configuration to conditionally enable/disable width and precision supports.

The test looks good to me +1

@LDong-Arm That's very interesting because I get 88 bytes more when i do the same thing with mbed compile -t GCC_ARM -m K64F -c which results in ./BUILD/K64F/GCC_ARM/mbed-os-example-blinky.bin with 42982 Flash and 12072 RAM compared to 42894 Flash and 12072 RAM on the master without floating points enabled. If I activate floating points I got 140 bytes flash (45706 -> 45846) more.

Thank you for looking over the test!

EmbedEngineer · 2020-09-09T06:35:32Z

@LDong-Arm Could you please recheck your size calculations, because no matter what I do, I cannot find the 1kB of Flash usage. It always stays 88 bytes w/o floating points and 140 bytes with floating points, which makes kind of sense for me, since I only added about 20 lines of code.

LDong-Arm · 2020-09-09T08:38:18Z

@LDong-Arm Could you please recheck your size calculations, because no matter what I do, I cannot find the 1kB of Flash usage. It always stays 88 bytes w/o floating points and 140 bytes with floating points, which makes kind of sense for me, since I only added about 20 lines of code.

Sorry my bad, I checked again and the size increase is only 190 bytes with the Arm Compiler and floating point not enabled.

LDong-Arm

The implementation looks good to me. But anyone thinks we need a config to enable/disable this?

0xc0170 · 2020-09-10T14:32:50Z

platform/source/minimal-printf/mbed_printf_implementation.c

 */
-static void mbed_minimal_formatted_string_hexadecimal(char *buffer, size_t length, int *result, MBED_UNSIGNED_STORAGE value, FILE *stream, bool upper)
+static void mbed_minimal_formatted_string_hexadecimal(
+    char *buffer,


why was this changed to have arg on a new separate line? As the rest of the functions have it the way it was, should this be reverted?

it was a change request from @hugueskamba since the line got too long

Sorry about the contradictory comments but this change should be reverted for consistency with the rest of the file.

evedon · 2020-12-02T10:23:18Z

@EmbedEngineer That's would be great. Thanks.

EmbedEngineer · 2020-12-04T12:38:30Z

@evedon so far I think I'm finished

evedon

Unfortunately, by default the tests do not run with floating points enabled. You need the following command:

mbed test -t gcc_arm -m k64f -n *minimal-printf* -vv --app-config platform/tests/TESTS/mbed_platform/minimal-printf/compliance/test_config.json

You will find that the current tests don't pass for floating point values. Look at my suggestion for a fix.

platform/source/minimal-printf/mbed_printf_implementation.c

platform/tests/TESTS/mbed_platform/minimal-printf/compliance/main.cpp

Pull request has been modified.

evedon

Great! the implementation is correct now. Thanks for the work on this PR.
I'd suggest that you squash your commits and that @0xc0170 kicks off CI

evedon · 2020-12-08T08:50:22Z

I just noticed that the PR description needs updating, specifically the "Impact" section. Also this PR fixes #13783 so this should be added in the description

Pull request has been modified.

EmbedEngineer · 2020-12-08T15:10:47Z

@evedon I updated the pull request description and squashed the commits.

platform/source/minimal-printf/mbed_printf_implementation.c

…floating point output as well as decimal precision for floating point

EmbedEngineer · 2020-12-09T08:17:49Z

@evedon I think I'm finally done. Please let me know if there is anything else to do on this PR. Thank you for your input. I appreciated it very much.

evedon · 2020-12-09T08:33:37Z

Code change approved but the PR description still needs updating as width modifier does not need to be an even number. Also update the "Impact" section to say what has been fixed; it is useful when we make the release notes.
Thanks

EmbedEngineer · 2020-12-09T09:07:43Z

@evedon Done!

mbed-ci · 2020-12-09T21:57:25Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

caoddx · 2021-01-19T01:44:21Z

I did not find the test case of printf %f in the test log

It seems that the test_config.json file didn't work.
(platform/tests/TESTS/mbed_platform/minimal-printf/compliance/test_config.json)

LDong-Arm · 2021-01-20T10:16:40Z

I did not find the test case of printf %f in the test log

It seems that the test_config.json file didn't work.
(platform/tests/TESTS/mbed_platform/minimal-printf/compliance/test_config.json)

This test_config.json is not used in CI which tests the default configuration. You can run it locally with

mbed test -t <TOOLCHAIN> -m <TARGET> -n platform-tests-tests-mbed_platform-minimal-printf --app-config platform/tests/TESTS/mbed_platform/minimal-printf/compliance/test_config.json

Btw, thanks for raising the issue in minimal-printf!

mergify bot added the needs: work label Sep 4, 2020

ciarmcom added the release-type: feature label Sep 4, 2020

ciarmcom requested review from a team September 4, 2020 11:00

ciarmcom added the needs: review label Sep 4, 2020

hugueskamba changed the title ~~added width modifier and prepending zeros for hexadecimal output and …~~ added width modifier and prepending zeros for hexadecimal output and decimal precision for floating point Sep 4, 2020

hugueskamba reviewed Sep 4, 2020

View reviewed changes

mergify bot removed the needs: review label Sep 7, 2020

LDong-Arm reviewed Sep 7, 2020

View reviewed changes

platform/source/minimal-printf/mbed_printf_implementation.c Outdated Show resolved Hide resolved

platform/source/minimal-printf/mbed_printf_implementation.c Outdated Show resolved Hide resolved

LDong-Arm reviewed Sep 7, 2020

View reviewed changes

platform/source/minimal-printf/mbed_printf_implementation.c Outdated Show resolved Hide resolved

0xc0170 added needs: review and removed needs: work labels Sep 7, 2020

hugueskamba reviewed Sep 7, 2020

View reviewed changes

EmbedEngineer force-pushed the enhance-minimal-printf branch from 8f00a96 to ebe15a7 Compare September 8, 2020 10:55

LDong-Arm approved these changes Sep 9, 2020

View reviewed changes

0xc0170 reviewed Sep 10, 2020

View reviewed changes

evedon previously requested changes Dec 7, 2020

View reviewed changes

platform/source/minimal-printf/mbed_printf_implementation.c Show resolved Hide resolved

platform/tests/TESTS/mbed_platform/minimal-printf/compliance/main.cpp Show resolved Hide resolved

platform/tests/TESTS/mbed_platform/minimal-printf/compliance/main.cpp Show resolved Hide resolved

evedon previously approved these changes Dec 8, 2020

View reviewed changes

EmbedEngineer force-pushed the enhance-minimal-printf branch from ac250ae to c4f3450 Compare December 8, 2020 15:08

evedon reviewed Dec 8, 2020

View reviewed changes

platform/source/minimal-printf/mbed_printf_implementation.c Outdated Show resolved Hide resolved

Add width modifier and prepending zeros for integer, hexadecimal and …

03cf621

…floating point output as well as decimal precision for floating point

EmbedEngineer force-pushed the enhance-minimal-printf branch from c4f3450 to 03cf621 Compare December 8, 2020 20:10

evedon approved these changes Dec 9, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Dec 9, 2020

mergify bot added ready for merge and removed needs: CI labels Dec 9, 2020

0xc0170 merged commit c06fcaf into ARMmbed:master Dec 10, 2020

mergify bot removed the ready for merge label Dec 10, 2020

mbedmain added release-version: 6.6.0 Release-pending and removed release-type: feature Release-pending labels Dec 11, 2020

0xc0170 mentioned this pull request Dec 18, 2020

[mbed-os-6.6.0] minimal-printf incorrect output #14069

Closed

Add width modifier and prepending zeros for hexadecimal output and decimal precision for floating point #13548

Add width modifier and prepending zeros for hexadecimal output and decimal precision for floating point #13548

Uh oh!

Conversation

EmbedEngineer commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Sep 4, 2020

Uh oh!

0xc0170 commented Sep 4, 2020

Uh oh!

LDong-Arm commented Sep 4, 2020

Uh oh!

pan- commented Sep 4, 2020

Uh oh!

hugueskamba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evedon commented Sep 7, 2020

Uh oh!

EmbedEngineer commented Sep 7, 2020

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LDong-Arm commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmbedEngineer commented Sep 7, 2020

Uh oh!

Uh oh!

EmbedEngineer commented Sep 7, 2020

Uh oh!

hugueskamba left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmbedEngineer commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmbedEngineer commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmbedEngineer commented Sep 9, 2020

Uh oh!

LDong-Arm commented Sep 9, 2020

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

EmbedEngineer Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

evedon Nov 17, 2020

Choose a reason for hiding this comment

EmbedEngineer commented Sep 4, 2020 •

edited

Loading

LDong-Arm commented Sep 7, 2020 •

edited

Loading

hugueskamba left a comment •

edited

Loading

EmbedEngineer commented Sep 8, 2020 •

edited

Loading

LDong-Arm commented Sep 8, 2020 •

edited

Loading

EmbedEngineer commented Sep 8, 2020 •

edited

Loading

EmbedEngineer commented Dec 9, 2020 •

edited

Loading

LDong-Arm commented Jan 20, 2021 •

edited

Loading