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

[BUG] [DSP] arm_math.h does not consider Cortex M33 #427

Closed fgr1986 closed 5 years ago

fgr1986 commented 6 years ago

Though it considers ARM_MATH_ARMV8MML, the addition of ARM_MATH_CM33 could help the developers not to tweak this file. Idea:

...
#elif defined (ARM_MATH_CM33)
  #include "core_cm33.h"
  #if (defined (__DSP_PRESENT) && (__DSP_PRESENT == 1))
    #define ARM_MATH_DSP
  #endif
llefaucheur commented 6 years ago

Since you are Arm employee I will send you a direct SfB message :-)

JonatanAntoni commented 6 years ago

Hi @fgr1986,

Thanks for raising this issue. We received multiple complains about that in the meanwhile. On the short run I propose adding the following to arm_math.h:

#ifdef _RTE_
#include "RTE_Components.h"
#endif

#if defined(CMSIS_device_header)
  #include CMSIS_device_header
  #if ((__CORTEX_M == 0) || (__CORTEX_M == 1) || (__CORTEX_M == 23))
    #define ARM_MATH_CM0_FAMILY
  #elif (!defined(__DSP_PRESENT) && ((__CORTEX_M == 4) || (__CORTEX_M == 7)))
    #define ARM_MATH_DSP
  #elif (defined(__DSP_PRESENT) && (__DSP_PRESENT == 1))
    #define ARM_MATH_DSP
  #endif
#else
  #define __CMSIS_GENERIC         /* disable NVIC and Systick functions */
  #if defined(ARM_MATH_CM7)
    #include "core_cm7.h"
    #define ARM_MATH_DSP
  #elif defined (ARM_MATH_CM4)
    #include "core_cm4.h"
    #define ARM_MATH_DSP
  #elif defined (ARM_MATH_CM3)
    #include "core_cm3.h"
  #elif defined (ARM_MATH_CM0)
    #include "core_cm0.h"
    #define ARM_MATH_CM0_FAMILY
  #elif defined (ARM_MATH_CM0PLUS)
    #include "core_cm0plus.h"
    #define ARM_MATH_CM0_FAMILY
  #elif defined (ARM_MATH_ARMV8MBL)
    #include "core_armv8mbl.h"
    #define ARM_MATH_CM0_FAMILY
  #elif defined (ARM_MATH_ARMV8MML)
    #include "core_armv8mml.h"
    #if (defined (__DSP_PRESENT) && (__DSP_PRESENT == 1))
      #define ARM_MATH_DSP
    #endif
  #else
    #error "Define according the used Cortex core ARM_MATH_CM7, ARM_MATH_CM4, ARM_MATH_CM3, ARM_MATH_CM0PLUS, ARM_MATH_CM0, ARM_MATH_ARMV8MBL, ARM_MATH_ARMV8MML"
  #endif
  #undef  __CMSIS_GENERIC         /* enable NVIC and Systick functions */
#endif

This solution would automatically take the RTE configuration into account, if available. Users not using RTE can still define the device header to be used manually by giving CMSIS_device_header. As the fallback the current solution with individual defines can still be used, i.e. due to backward compatibility.

Unfortunately we have one code snippet implemented in assembly: arm_bitreversal2. This one still uses some specific device defines to distinguish between Armv6-M (v8-MBL) and Armv-7M (v8-MML). The assembly does not include arm_math.h of course. Hence we need another solution here. Perhaps the easiest solution would be to split this file into pieces, one per architecture.

Please let me all know about your opinion.

Cheers, Jonatan

fgr1986 commented 6 years ago

Jonatan, Laurent,

Thank you for the information. I think that in this case providing CMSIS_device_header will be the cleanest approach.

Kind regards

JonatanAntoni commented 6 years ago

@fgr1986,

this proposal has not yet been approved nor added to the official code. Thanks for your feedback. We need to finally decide what to do.

Cheers, Jonatan

zejiang0jason commented 6 years ago

Thanks Jonatan,

Thanks for feedback, we got the same problem, and we will follow your proposal.

Thanks.

JonatanAntoni commented 5 years ago

This has been addressed in commit 5913df81.

JonatanAntoni commented 5 years ago

@fgr1986,

May I ask you to cross check the provided fix and finally close the issue, please?

Thanks, Jonatan

fgr1986 commented 5 years ago

@JonatanAntoni My bad. Thank you very much for the support given