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

Fix GCC 13 __FPU_PRESENT redefinition warning #1191

Merged

Conversation

calebchalmers
Copy link
Contributor

Resolves #1181.

Let me know what you think about removing the previous defines, as mentioned here.

@calebchalmers
Copy link
Contributor Author

Looks like one of the tests failed due to running out of space?

scons: building terminated because of errors.modm/src/modm/platform/core/vectors.c:239:9: fatal error: error writing to /tmp/cchdxs7h.s: No space left on device

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Yes I think you're and this is the correct way to define the required macros.

ext/arm/arm_math_types.h.in Outdated Show resolved Hide resolved
@salkinium salkinium added this to the 2024q3 milestone Jul 14, 2024
@calebchalmers calebchalmers force-pushed the fix/arm-math-types-fpu-warning branch from 90e417f to bfcd7ed Compare July 14, 2024 19:29
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Thanks!

@salkinium salkinium force-pushed the fix/arm-math-types-fpu-warning branch from bfcd7ed to d8e46d6 Compare July 14, 2024 21:25
@salkinium
Copy link
Member

Something in this PR breaks the CMSIS DSP examples on Nucleo-F429. I've split the F4 examples into three jobs now and it still overflows disk space…

@calebchalmers
Copy link
Contributor Author

Super weird. Seems like there's also an issue with stm32f407vet6_devebox/blinky/main.cpp ?

@calebchalmers
Copy link
Contributor Author

Ok, I've been digging into the Nucleo-F429 issue. The cause of all the disk usage is two-fold:

  1. By including device.hpp, we are including stm32f429xx.h which is over 1MB in size. (Although the part we actually need is only lines 47-51.)
  2. Many headers files in the DSP library are including arm_math_types.h themselves, which means loading stm32f429xx.h over and over again, causing each object file to increase in size by roughly 800KB.

We could fix the issue by moving the device.hpp include from arm_math_types.h into arm_math.h, so it only gets included once. Thoughts @salkinium ?

@salkinium
Copy link
Member

Yes, try it! I think I may have encountered this issue when I initially integrated CMSIS-DSP. Maybe we could also read the first few lines of the header file in Python and regex out the required defines.

@calebchalmers
Copy link
Contributor Author

calebchalmers commented Jul 21, 2024

Hmm I've been playing around this and I've noticed that in some of the examples, they include both arm_math.h and arm_const_structs.h. (The latter also includes arm_math_types.h.)

The regex thing could work, but it doesn't seem too nice for a long term solution. I wonder if we could design a better interface? Maybe write our own headers over top of the arm ones to ensure the include order is correct. Or, we could put the includes for everything into one dsp.hpp header which would simplify the issue.

I'm not 100% sure what the intended interface for the DSP extension is currently, so guidance here would be appreciated!

@calebchalmers calebchalmers marked this pull request as draft August 18, 2024 03:47
@calebchalmers calebchalmers marked this pull request as ready for review August 18, 2024 05:45
@calebchalmers
Copy link
Contributor Author

I took another look at this and figured it out. I got confused last time, but moving the configuration into arm_math.h fixes the original issue as well as the massive file sizes. I removed arm_math_types.h.in since it isn't needed anymore, and I also updated the docs to reflect that users should only use arm_math.h or arm_math_f16.h, rather than including specific library headers.

This should be good to go now 👍

@salkinium salkinium force-pushed the fix/arm-math-types-fpu-warning branch from 846b412 to a3ac5db Compare August 18, 2024 13:01
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Thanks!

(I rebased and squashed your commits)

@salkinium salkinium merged commit a3ac5db into modm-io:develop Aug 18, 2024
12 checks passed
@calebchalmers calebchalmers deleted the fix/arm-math-types-fpu-warning branch August 18, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

GCC 13 warning: Redefinition of __FPU_PRESENT in CMSIS module
2 participants