ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

Warning Using DSP with CM23 #453

Closed kjassmann-renesas closed 4 years ago

kjassmann-renesas commented 6 years ago

I see this warning trying to use DSP library functions with CM23: core_armv8mbl.h:71:9: warning: '__CORTEX_M' macro redefined

To reproduce:

include "core_cm23.h"

define ARM_MATH_ARMV8MBL

include "arm_math.h" // includes "core_armv8mbl.h", which redefines __CORTEX_M

JonatanAntoni commented 6 years ago

Hi @kjassmann-renesas,

This is a shortcoming in the current DSP library implementation. You should never need to include the core header for your device directly but indirect through the device headers. In your case you get two different core headers included which cannot work.

For the time being you need to work around this issue until we can fix this properly. Hence you need to patch the arm_math.h and remove the duplicate core header include.

Please let me know if this works for you.

Cheers, Jonatan

kjassmann-renesas commented 6 years ago

Sure, we can work around it for now. We just wanted to report it so you can look into it.

And yes, the example was a simplification of our actual situation. We are including S1JA.h (our device header file), which includes core_cm23.h.

JonatanAntoni commented 6 years ago

Hi @kjassmann-renesas,

just to make you aware that we discussed two possibilities.

One solution is simply to add further device defines like ARM_MATH_CMxx and keep including the according core header file. The drawback is that this is not future-proof when adding new core headers to CMSIS-Core. Furthermore CMSIS-DSP does not really need the core header at all but the compiler header.

Hence another solution could be to remove the device defines and core includes completely. Instead we include the cmsis_compiler.h directly. Unfortunately this change is way more intrusive and we need to assure that nothing breaks. We need to infer the defines ARM_MATH_DSP and eventually ARM_MATH_CM0_FAMILY from compiler pre-defines. Concluding this solution looks cleaner and better maintainable.

Cheers, Jonatan

kjassmann-renesas commented 6 years ago

Thanks for the follow up. The second proposal would be a big improvement for us. We support 3 cores and 2 compilers, so if you can get that done, we can just include arm_math.h instead having all this code before it:

`#if defined(IAR_SYSTEMS_ICC)

if (CORE == ARM7EM)

define ARM_MATH_CM4

#elif (__CORE__ == __ARM6M__)

define ARM_MATH_CM0PLUS

#elif (__CORE__ == __ARM8M_BASELINE__)

define ARM_MATH_ARMV8MBL

#endif

elif defined(GNUC)

#if   __ARM_ARCH_7EM__

define ARM_MATH_CM4

#elif __ARM_ARCH_6M__

define ARM_MATH_CM0PLUS

#endif

endif

include "arm_math.h"

`

christophe0606 commented 5 years ago

@JonatanAntoni It looks like latest versiona of arm_math.h are including cmsis_compiler.h directly and no more a core header. In addition to that, I have removed the tests on architecture which were used to define ARM_MATH_CM0_FAMILY because it is no more useful. Only 3 functions of CMSIS-DSP are using it and I don't see a real difference between both implementations.

So, I guess it is addressing this issue ?

JonatanAntoni commented 5 years ago

Hi @kjassmann-renesas,

can you confirm that the latest version of CMSIS-DSP now works for you, please?

Thanks, Jonatan