Skip to content

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS #11883

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
cladmi:pr/newib/remove_use_nano_specs_variable
Jul 22, 2019
Merged

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS #11883
maribu merged 2 commits intoRIOT-OS:masterfrom
cladmi:pr/newib/remove_use_nano_specs_variable

Conversation

@cladmi
Copy link
Contributor

@cladmi cladmi commented Jul 22, 2019

Contribution description

Check for the usage of newlib_nano module instead of the
USE_NANO_SPECS variable.

This allows also benefiting from the printf_float and scanf_float
behaviour on arm7 and riscv cpus.

Testing procedure

Verify it is actually handled correctly on arm7 and riscv boards.

Run the tests/unittests tests-scanf_float on arm7 and riscv boards.

BOARD=msba2 make --no-print-directory -C tests/unittests/ tests-scanf_float flash-only test

Booting (hardware reset)...

Reset CPU (into user code)
Programming done.
ssh -t vaduz.imp.fu-berlin.de 'source /srv/ilab-builds/workspace/workspace.rc && BOARD=msba2 QUIET=0 make --no-print-directory -C /srv/ilab-builds/boards term'
/srv/ilab-builds/boards/RIOT/dist/tools/pyterm/pyterm -tg -p "/dev/riot/ttyMSBA2"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2019-07-22 16:37:43,181 - INFO # Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
2019-07-22 16:37:44,187 - INFO # main(): This is RIOT! (Version: 2019.10-devel-104-g294fb-HEAD)
2019-07-22 16:37:44,187 - INFO # ......
2019-07-22 16:37:44,188 - INFO # OK (6 tests)

Run other relevant tests:

  • msba2
    • tests/unittests tests-scanf_float @cladmi
    • tests/libfixmath_unittests
  • hifive1
    • tests/unittests tests-scanf_float

Issues/PRs references

General fix for #11882

USE_NANO_SPECS was added in #5094
Not sure why it was not set with newlib_nano at the end.

cladmi added 2 commits July 22, 2019 16:52
Check for the usage of `newlib_nano` module instead of the
`USE_NANO_SPECS` variable.

This allows also benefiting from the `printf_float` and `scanf_float`
behaviour on `arm7` and `riscv` cpus.
It was replaced by using directly the `newlib_nano` module.
@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

@maribu if you want to take a look. For the msba2 it would be the equivalent of setting USE_NANO_SPECS = 1 in the board/cpu.

@kenrabold can you try on the hifive1 board if the scanf_float printf_float was not working in master and working with this?

@maribu
Copy link
Member

maribu commented Jul 22, 2019

@cladmi: Thanks for adressing this. Should I update the other PR to do move the build system setup from boards to cpu/arm7_common (and remove the superseeded stdio with float support fix)? Or do you want to have a look at that yourself?

(As said, I only have basic understanding of the build system and would be happy someone with more insight would refactor this.)

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

It looks like some tests using printf_float still have issues for msba2.
It can also be a test issue and not the board.

BOARD=msba2 make --no-print-directory -C tests/unittests/ tests-printf_float  flash-only test

Type '/exit' to exit.
2019-07-22 17:10:02,014 - INFO # main(): This is RIOT! (Version: 2019.10-devel-106-g2d5ed-pr/newib/remove_use_nano_specs_variable)
2019-07-22 17:10:02,015 - INFO # .
2019-07-22 17:10:02,019 - INFO # pkt_tests.sfprintf_float (tests/unittests/tests-printf_float/tests-printf_float.c 40) exp "2016.034900" was "-0.000000"
2019-07-22 17:10:02,019 - INFO # 
2019-07-22 17:10:02,020 - INFO # run 1 failures 1

and also

BOARD=msba2 make --no-print-directory -C tests/thread_float/ flash term
...
2019-07-22 17:11:26,073 - INFO # Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
2019-07-22 17:11:27,080 - INFO # main(): This is RIOT! (Version: 2019.10-devel-106-g2d5ed-pr/newib/remove_use_nano_specs_variable)
2019-07-22 17:11:27,081 - INFO # THREADS CREATED
2019-07-22 17:11:27,081 - INFO #
2019-07-22 17:11:27,082 - INFO # THRTHREAD 4 start

However it makes tests/libfixmath_unittests now work correctly (was broken in master).

BOARD=msba2 make --no-print-directory -C tests/libfixmath_unittests/ flash test

...
2019-07-22 17:18:47,659 - INFO # ----Testing basic square roots----
2019-07-22 17:18:47,661 - INFO # OK: fix16_sqrt(fix16_from_int(16)) == fix16_from_int(4)
2019-07-22 17:18:47,663 - INFO # OK: fix16_sqrt(fix16_from_int(100)) == fix16_from_int(10)
2019-07-22 17:18:47,666 - INFO # OK: fix16_sqrt(fix16_from_int(1)) == fix16_from_int(1)
2019-07-22 17:18:47,666 - INFO #
2019-07-22 17:18:47,668 - INFO # ----Testing square root rounding corner cases----
2019-07-22 17:18:47,669 - INFO # OK: fix16_sqrt(214748302) == 3751499
2019-07-22 17:18:47,671 - INFO # OK: fix16_sqrt(214748303) == 3751499
2019-07-22 17:18:47,673 - INFO # OK: fix16_sqrt(214748359) == 3751499
2019-07-22 17:18:47,684 - INFO # OK: fix16_sqrt(214748360) == 3751500
2019-07-22 17:18:47,685 - INFO #
2019-07-22 17:18:47,686 - INFO # ----Running test cases for square root----
2019-07-22 17:18:47,687 - INFO # OK: failures == 0
2019-07-22 17:18:47,687 - INFO # SUCCESS

tests/ps_schedstatistics works, but it does not use float prints…
It should be removed from the dependencies.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

@cladmi: Thanks for adressing this. Should I update the other PR to do move the build system setup from boards to cpu/arm7_common (and remove the superseeded stdio with float support fix)? Or do you want to have a look at that yourself?

You can do it if you want, moving to cpu does not need extensive testing and can be done through checking variables values only.
BOARD=msba2 make info-debug-variable-CFLAGS, BOARD=msba2 make info-debug-variable-ANY_VARIABLE_YOU_WANT in an application directory.
I cannot ACK the pr but can take a look to test and spot if I see issues.

If you prefer I can take care of it inspired by your PR.

(As said, I only have basic understanding of the build system and would be happy someone with more insight would refactor this.)

No worry.

There are many things that are not made as documented, with side-effects, limitation, and different way in each architecture/cpu/board.
Things are said to be done in a way, but they are not, or not completely, or just sometime.
Or done in a way nobody else does.

How should anybody understand without spending a lot of time in it.

I am working to removing these things but it takes time, extensive testing procedure.
I wanted to document some parts but it was hard to try explaining all the workarounds and quirks. So I started trying to fix them first.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

I have a weird thing. I manage to make printf_float unittest work by moving the stack from .bss to .data. (not sure if the syntax is really initializing everything but it makes it work).

BOARD=msba2 make --no-print-directory -C tests/unittests/ clean tests-printf_float flash-only test
/srv/ilab-builds/boards/RIOT/dist/tools/pyterm/pyterm -tg -p "/dev/riot/ttyMSBA2"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2019-07-22 18:31:56,087 - INFO # Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
2019-07-22 18:31:57,094 - INFO # main(): This is RIOT! (Version: 2019.10-devel-106-g2d5ed-pr/newib/remove_use_nano_specs_variable)
2019-07-22 18:31:57,095 - INFO # .
2019-07-22 18:31:57,095 - INFO # OK (1 tests)

I also checked when using printf.

diff --git a/core/kernel_init.c b/core/kernel_init.c
index 1b153b38a..bb61c5dd9 100644
--- a/core/kernel_init.c
+++ b/core/kernel_init.c
@@ -74,7 +74,7 @@ static void *idle_thread(void *arg)
 const char *main_name = "main";
 const char *idle_name = "idle";

-static char main_stack[THREAD_STACKSIZE_MAIN];
+static char main_stack[THREAD_STACKSIZE_MAIN] = {0xff};
 static char idle_stack[THREAD_STACKSIZE_IDLE];

 void kernel_init(void)

Smells like ldscript issues.
EDIT: or stack overflow somewhere…

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

The BOARD=msba2 make --no-print-directory -C tests/thread_float/ flash term issue is also there when not printing float, so unrelated to here.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

The issue from tests/thread_float is that thread_yield() is called from interrupt context which switch out of the interrupt for msba2 like thread_yield_higher as shown in #11759 (must be the same for msp430).

Nothing in the documentation says if it is supposed to work in interrupt or not.
Interrupts have their own sched_context_switch_request = 1; to trigger thread_yield_higher().

How an interrupt is supposed to trigger a normal thread_yield()?

This should be moved to a separated issue though and does not block this PR.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 22, 2019
@kenrabold
Copy link
Contributor

I confirmed that on HiFive1 and HiFive1B boards with the PR and they all passed:

tests/unittests tests-scanf_float
tests/unittests tests-printf_float

with master, the above unit tests all failed.

@maribu
Copy link
Member

maribu commented Jul 22, 2019

OK, here is what I did for testing

  • Double checked no other users of USE_NANO_SPECS is present in the code using grep
  • Confirmed that the tests-scanf_float in tests/unittests is now indeed working on the MSB-A2

I can confirm that tests-printf_float still fails on the MSB-A2, but that has to be unrelated.

@maribu
Copy link
Member

maribu commented Jul 22, 2019

@kenrabold: Thanks for testing!

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 22, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK :-)

@maribu maribu merged commit 0729beb into RIOT-OS:master Jul 22, 2019
@maribu
Copy link
Member

maribu commented Jul 22, 2019

I opened an issue for the failing printf() on the MSB-A2. I'll open another issue a PR containing the fix tomorrow for the thread_yield() in ISR. As promised, I'll investigate and fix both to keep the MSB-A2 alive with RIOT.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2019

Thank you for the review.

@maribu
Copy link
Member

maribu commented Jul 23, 2019

You're very welcome :-) Thanks for the fix!

@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2019

@MrKevinWeiss If you want, I can backport this one as it fixed the msba2 and hifive1 floating point tests.

The impact is limited to only these two architectures and when printf_float or scanf_float is used. For arm the handling was already done. It just remove the need to define USE_NANO_SPECS.

@maribu
Copy link
Member

maribu commented Jul 23, 2019

For the hifive1 this makes sense. But for the msba2 I'm about the only user left, and I only use master :-D

Edit: Before causing confusion: I did't want to suggest to magically split of the hifive1 fix from the msba2 fix for the backport. I just wanted to point out that if the fix were only addressing the issue for the msba2, their would be no need to invest the time to backport.

@cladmi cladmi deleted the pr/newib/remove_use_nano_specs_variable branch July 23, 2019 11:20
@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2019

@maribu it is listed on the http://riot-os.org/ website under supported architectures. The number of supported architectures and boards is often advertised.

I am running the test suite on it regularly and for the release too https://ci-ilab.imp.fu-berlin.de/job/build-results/27/artifact/msba2/failuresummary.md/*view*/

It is the output of the ./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py if you also want to monitor the tests failure from time to time. It takes 1h30 for me, but I am building with docker so it slows it down a bit.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2019

Backport provided in #11890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants