cortexm_common: add FPU support for cortex-m4f and cortex-m7 by vincent-d · Pull Request #6630 · RIOT-OS/RIOT

vincent-d · 2017-02-20T17:29:41Z

Not sure if every cases are covered, but it works with nucleo-f401 and nucleo-f413.
I added a test app based on thread_msg which uses float.

No hard fault so far and float results seem OK with different threads.

haukepetersen · 2017-02-21T07:49:32Z

Nice! Looks great. I think it might make sense to add a fpu feature, so we can do nicer filtering when building the test?!

kaspar030 · 2017-02-21T08:06:01Z

The test runs those three threads in parallel, right?

Maybe it would be useful to crazily switch between them, e.g., using a periodic timer?

vincent-d · 2017-02-22T14:48:55Z

@haukepetersen I don't really know what it implies, so maybe we could. But I don't really like the Makefile stuff to disable the feature, so if you have a better idea, don't hesitate to give it.

@kaspar030 you're right, we should try to switch in the middle of the float calculations, I'll try to improve the test app

vincent-d · 2017-02-22T16:59:33Z

I added a patch to cmsis-dsp to include cpu_conf.h in order to fix the build issues

vincent-d · 2017-02-23T10:46:16Z

Test app updated to switch threads more agressively

vincent-d · 2017-02-28T13:21:11Z

Fixed CI issues

kYc0o · 2017-03-03T15:05:38Z

@vincent-d Can you squash to enable the CI?

vincent-d · 2017-03-03T15:55:44Z

squashed

kYc0o · 2017-03-03T16:48:24Z

cpu/Makefile.include.cortexm_common

+    endif
+    export MCPU := cortex-m4
+else
 CFLAGS_FPU ?= -mfloat-abi=soft


The default behaviour is then soft float?

No, this else is for everything not cortex-m4f

OK, I misread the ifneq (llvm,$(TOOLCHAIN)). Thanks!

kYc0o · 2017-03-03T16:48:52Z

tests/thread_float/Makefile

+                             nucleo-f334 opencm9-04 pca10000 pca10005 spark-core \
+                             stm32f0discovery weio yunjia-nrf51822
+
+DISABLE_MODULE += auto_init


Why auto_init should be disabled?

This is actually a good question :D

I copied/paste another test, so I guess we could remove that line and enable auto_init.

jnohlgard · 2017-03-03T19:29:55Z

cpu/Makefile.include.cortexm_common

+        export CFLAGS_FPU ?= -mfloat-abi=soft
+    else
+        # clang assumes there is an FPU
+        ifneq (llvm,$(TOOLCHAIN))


It's there any reason to not give these flags to clang, regardless of the default setting? Does clang give an error if you specify hard float? Toolchains are usually quite configurable and the defaults may not always be the same.

Wihtout this ifneq, the following error is generated:

src/RIOT/cpu/cortexm_common/include/cmsis_gcc.h:344:54: error: unknown register name 'vfpcc' in asm __ASM volatile ("VMSR fpscr, %0" : : "r" (fpscr) : "vfpcc"); ^ 1 error generated.

clang version : 3.8.1-12ubuntu1

vincent-d · 2017-03-06T09:46:58Z

Auto_init no longer disabled.

vincent-d · 2017-03-29T13:57:22Z

rebased
improved Makefile.onclude.cortexm_common to avoid having a warning when disabling cortexm_fpu module

MrKevinWeiss · 2018-05-30T06:53:08Z

tests/thread_float/main.c

+    f = init;
+
+    while (1) {
+        for (int i = 0; i < 100000; i++) {


Seems murdock doesn't like this too much, maybe specify uint32_t or limit it to 32767. If you do that I will test then hopefully merge now that I am a maintainer ;)

should be fixed, haven't retested though

I think you hit the wrong for loop, do the one on line 83.

Sorry, I had fixed the other one, this one should be fixed now as well

On the AVR (among others) an int is only 16 bits, so this comparison is always true for those architectures.

@bergzand I switch to unsigned long

tests/thread_float/Makefile

MrKevinWeiss · 2018-05-31T13:23:27Z

Well it's probably a silly mistake for me but I am trying to run the test on the nucleo-f446ze and it appears that it cannot switch threads. The MCU is a cortex M4 with FPU. It stays in thread pid == 3 and never switches.

2018-05-31 15:16:39,540 - INFO # THREADS CREATED
2018-05-31 15:16:39,541 - INFO # 
2018-05-31 15:16:39,542 - INFO # THREAD 3 start
2018-05-31 15:16:39,592 - INFO # T(3): 141.466812
2018-05-31 15:16:39,641 - INFO # T(3): 141.490616
2018-05-31 15:16:39,694 - INFO # T(3): 141.521576
2018-05-31 15:16:39,742 - INFO # T(3): 141.559769
2018-05-31 15:16:39,791 - INFO # T(3): 141.605148
2018-05-31 15:16:39,844 - INFO # T(3): 141.657516
2018-05-31 15:16:39,892 - INFO # T(3): 141.717255
2018-05-31 15:16:39,941 - INFO # T(3): 141.783844
2018-05-31 15:16:39,995 - INFO # T(3): 141.857513
2018-05-31 15:16:40,042 - INFO # T(3): 141.938278
2018-05-31 15:16:40,096 - INFO # T(3): 142.026108
2018-05-31 15:16:40,144 - INFO # T(3): 142.120789

If I set the while loops to while(0) is appears that the threads are there.

2018-05-31 15:18:04,680 - INFO # THREADS CREATED
2018-05-31 15:18:04,680 - INFO # 
2018-05-31 15:18:04,680 - INFO # THREAD 3 start
2018-05-31 15:18:04,684 - INFO # THREAD 4 start
2018-05-31 15:18:04,685 - INFO # THREAD 5 start

Is that the intended behavior for the test or is there a problem with the sched_switch() not yielding?

(I can also confirm it is getting into the timer_cb)

kaspar030 · 2018-06-01T13:33:21Z

tests/thread_float/main.c

+    (void) arg;
+
+    xtimer_set(&timer, OFFSET);
+    sched_switch(THREAD_PRIORITY_MAIN - 2);


shouldn't "thread_yield()``` do the trick here?

I tested with thread_yield() and it seems to work ok!

2018-06-01 15:58:12,754 - INFO # THREADS CREATED 2018-06-01 15:58:12,755 - INFO # 2018-06-01 15:58:12,756 - INFO # THTTRHHERREEAAADDD 55 5 s ststatraatrr 2018-06-01 15:58:12,757 - INFO # tt 2018-06-01 15:58:12,758 - INFO # 2018-06-01 15:58:12,898 - INFO # T(5): 141.521576 2018-06-01 15:58:12,899 - INFO # T(3): 141.466812 2018-06-01 15:58:13,042 - INFO # T(5): 141.559769 2018-06-01 15:58:13,049 - INFO # T(3): 141.490616 2018-06-01 15:58:13,192 - INFO # T(5): 141.605148 2018-06-01 15:58:13,198 - INFO # T(3): 141.521576 2018-06-01 15:58:13,343 - INFO # T(5): 141.657516 2018-06-01 15:58:13,344 - INFO # T(3): 141.559769 2018-06-01 15:58:13,492 - INFO # T(5): 141.717255 2018-06-01 15:58:13,494 - INFO # T(3): 141.605148

MrKevinWeiss · 2018-11-29T12:09:38Z

@kaspar030 @vincent-d Should we try to push this one forward?

kaspar030 · 2018-11-29T12:30:44Z

Yes! @vincent-d could you rebase once more?

aabadie · 2019-01-03T13:22:02Z

@vincent-d, just a rebase is missing. I guess you can also squash.

vincent-d · 2019-01-03T14:28:49Z

Rebased and squashed

MrKevinWeiss · 2019-01-03T14:50:50Z

@vincent-d looks like arduino-uno and arduino-duemilanove needs insufficient memory to make murdock happy...

I would also like to retest quickly.

MrKevinWeiss · 2019-01-03T14:59:49Z

Test is the same. @kaspar030 are the changes sufficient for you?

vincent-d · 2019-01-03T15:08:20Z

Fixed and amended directly

MrKevinWeiss · 2019-01-03T15:23:33Z

@vincent-d Thanks for the fast response. 👍

We wait for @kaspar030 to dismiss at this point.

kaspar030 · 2019-01-04T10:57:02Z

I quickly checked code size, memory consumption and context switch performance. code & memory is not worth talking about, context switch performance is down 2%, which is a lot less than I expected and completely acceptable.

ACK & go.

Thanks @ALL and especially to @vincent-d for the work and patience!

benpicco · 2021-09-27T15:54:33Z

It would be great if the test had a tests/01-run.py to verify the correct output of the test.
I'm a bit at a loss trying to figure out what is the correct behavior to expect from the test.

vincent-d assigned haukepetersen and kaspar030 Feb 22, 2017

vincent-d added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Feb 22, 2017

vincent-d force-pushed the pr/cortex-m4f-fpu branch from b2b5037 to fc51f8c Compare March 3, 2017 15:55

kYc0o added this to the Release 2017.04 milestone Mar 3, 2017

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 3, 2017

kYc0o reviewed Mar 3, 2017

View reviewed changes

jnohlgard reviewed Mar 3, 2017

View reviewed changes

OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 6, 2017

vincent-d force-pushed the pr/cortex-m4f-fpu branch from edecef6 to 687fc5e Compare March 29, 2017 13:56

kYc0o added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 3, 2017

kaspar030 modified the milestones: Release 2017.04, Release 2017.07 Apr 21, 2017

vincent-d force-pushed the pr/cortex-m4f-fpu branch from 687fc5e to 9106721 Compare May 24, 2017 16:17

MrKevinWeiss requested changes May 30, 2018

View reviewed changes

vincent-d force-pushed the pr/cortex-m4f-fpu branch from f65f277 to 17e2c26 Compare May 30, 2018 07:25

MrKevinWeiss requested changes May 30, 2018

View reviewed changes

tests/thread_float/Makefile Outdated Show resolved Hide resolved

vincent-d force-pushed the pr/cortex-m4f-fpu branch from 5e5fafb to 22e337e Compare May 30, 2018 13:54

MrKevinWeiss approved these changes May 31, 2018

View reviewed changes

kaspar030 requested changes Jun 1, 2018

View reviewed changes

vincent-d force-pushed the pr/cortex-m4f-fpu branch from 22e337e to cdb890d Compare July 17, 2018 14:33

cortexm_common: enable FPU on cortex-m4f

06f0c14

vincent-d force-pushed the pr/cortex-m4f-fpu branch from cdb890d to d30ebc2 Compare January 3, 2019 14:24

Vincent Dupont added 2 commits January 3, 2019 16:06

tests: add thread_float test app

402b60e

pkg/cmsis-dsp: include cpu_conf.h for __FPU_PRESENT define

e06993e

vincent-d force-pushed the pr/cortex-m4f-fpu branch from d30ebc2 to e06993e Compare January 3, 2019 15:06

kaspar030 approved these changes Jan 4, 2019

View reviewed changes

MrKevinWeiss merged commit 9554f75 into RIOT-OS:master Jan 4, 2019

aabadie added this to the Release 2019.01 milestone Jan 4, 2019

dylad mentioned this pull request Jan 15, 2019

cpu/cortexm_common: Update ARM CMSIS to V5.4.0 #10558

Merged

cladmi mentioned this pull request Jul 23, 2019

cpu/arm7_common: Fix thread_yield_higher in ISR #11887

Merged

aabadie mentioned this pull request Jan 30, 2020

cpu/cortexm-fpu: use it via a feature instead of DEFAULT_MODULE #13236

Closed

benpicco mentioned this pull request Oct 30, 2023

cortexm_common: drop TODO about Cortex-M4F FPU #20029

Merged

Conversation

vincent-d commented Feb 20, 2017

Uh oh!

haukepetersen commented Feb 21, 2017

Uh oh!

kaspar030 commented Feb 21, 2017

Uh oh!

vincent-d commented Feb 22, 2017

Uh oh!

vincent-d commented Feb 22, 2017

Uh oh!

vincent-d commented Feb 23, 2017

Uh oh!

vincent-d commented Feb 28, 2017

Uh oh!

kYc0o commented Mar 3, 2017

Uh oh!

vincent-d commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 6, 2017

Uh oh!

vincent-d commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MrKevinWeiss commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Nov 29, 2018

Uh oh!

kaspar030 commented Nov 29, 2018

Uh oh!

aabadie commented Jan 3, 2019

Uh oh!

vincent-d commented Jan 3, 2019

Uh oh!

MrKevinWeiss commented Jan 3, 2019

Uh oh!

MrKevinWeiss commented Jan 3, 2019

Uh oh!

vincent-d commented Jan 3, 2019

Uh oh!

MrKevinWeiss commented Jan 3, 2019

Uh oh!

kaspar030 commented Jan 4, 2019

Uh oh!

benpicco commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects