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

CMSIS for ARMCLANG and arm_compat.h #1211

Closed
CanastraRF opened this issue Jun 11, 2021 · 9 comments
Closed

CMSIS for ARMCLANG and arm_compat.h #1211

CanastraRF opened this issue Jun 11, 2021 · 9 comments

Comments

@CanastraRF
Copy link

We upgraded CMSIS core to the current version V5.4.3 for cmsis_armclang.h.

Now arm_compat.h is no longer included but some functions are copied to cmsis_armclang.h.
ex.
__enable_irq()
__disable_irq()

So cmsis_armclang.h and arm_compat.h can't no longer used together in one project.

But we use more compatibility function from arm_compat.h.
ex:
__current_pc()
__current_sp()

So please add all compat function to CMSIS or guard in CMSIS the copied functions.

@JonatanAntoni
Copy link
Member

Hi @CanastraRF,

thanks for pointing this out. I'll check that for the upcoming release.

-Jonatan

@JonatanAntoni
Copy link
Member

Hi @CanastraRF,

It looks like the current CMSIS Compiler header do conflict with arm_compat.h. I don't see how we can prevent this, reliably.

I could guard the definition in the compiler header. This would only cover the case when arm_compat.h is included before. Without modifying arm_compat.h including it second would still cause redefinition-errors.

Would it be enought from your point of view to enhance CMSIS Compiler abstraction with equivalents to __current_pc and __current_sp (similar to available __get_MSP and __get_PSP)? We'd need to work out implementations for all compilers, i.e. GCC, AC5, AC6 and IAR. Do you have experience to contribute?

Thanks,
Jonatan

@CanastraRF
Copy link
Author

Hi Jonatan
If you add in cmsis_armclang.h

#ifndef __ARM_COMPAT_H
__STATIC_FORCEINLINE void __enable_irq(void)
{
  __ASM volatile ("cpsie i" : : : "memory");
}
#endif

and similar to __disable_irq()
We can #include <arm_compat.h> prior to cmsis_armclang.h and it should work fine.

At the moment I've no time to contribute.
BTW. I sent you some modification in DSP source long time ago and I never get any responce.

@JonatanAntoni
Copy link
Member

Hi @CanastraRF,

Yes, including arm_compat.h before would work. But mandating an include order looks bad practice to me.
Hence I am thinking about adding __get_PC and __get_SP to CMSIS so that you don't need arm_compat.h at all, anymore. The additional benefit is that you code is Compiler agnostic when you don't need to rely on Arm Compiler stuff.

The challenge is to have this properly implemented for all supported compilers.

Cheers,
Jonatan

@CanastraRF
Copy link
Author

Hi Jonatan
For new code __get_PC() and __get_SP() is helpfull.
But all ARM5 users migrate to ARMCLANG must update there code.
For backward compatibility is arm_compat.h.
There are lot of other functions in arm_compat.h and arm_acle.h.
So it would be nice when arm_compat.h can be used without any limitations.

But now __enable_irq() and __disable_irq() are defined in both files.
I think CMSIS should use an unique Name prefix like CM_getPC().
But this breaks the compatibility to exist CMSIS release.

Reto Felix

@JonatanAntoni
Copy link
Member

Hi Reto,

I concern we can change the names in CMSIS, easily.
There should be no issue with using arm_acle.h but only for arm_compat.h.

What else do you miss from arm_compat.h? To my knowledge this is rarther small.
I can add the #ifdef guards around the conflicting function definitions in addition. But as discussed earlier this would create a dependency on include order which I don't find really clean.

Jonatan

@CanastraRF
Copy link
Author

Hi Jonatan,
I check our legacy codes and found the following intrinsic from ARMCC5
__nop()
__isb()
__dsb()
__dmb()
__rev()
__rbit()
__ror()
__clz()
__qdbl()
__usat()
__ssat()
__usat16()
__ssat16()
__uadd16()
__uadd8()
__sadd16()
__sadd8()
__usub16()
__usub8()
__ssub16()
__ssub8()
__uqadd16()
__uqadd8()
__qadd16()
__qadd8()
__uqsub16()
__uqsub8()
__qsub16()
__qsub8()
__uhadd16()
__uhadd8()
__shadd16()
__shadd8()
__uhsub16()
__uhsub8()
__shsub16()
__shsub8()
__uasx()
__usax()
__sasx()
__ssax()
__uqasx()
__uqsax()
__qasx()
__qsax()
__uhasx()
__uhsax()
__shasx()
__shsax()
__wfi()
__wfe()
__sev()
__breakpoint()
...
for this functions we incude arm_compat.h.
Most of the definitions are in arm_acle.h. But arm_acle.h is included from arm_compat.h

So in any case we must modify our code in any way.

  • replace all ARMCC5 intrinic by CMSIS equivalent.
  • replace all #include <arm_compat.h> by #include <arm_acle.h>
  • modify cmsis_armclang.h

->the smalles modification is to modify cmsis_armclang.h

Reto Felix

JonatanAntoni added a commit that referenced this issue Jun 17, 2021
The arm_compat.h defines the ArmCC compatibility functions
__enable_irq and __disable_irq. These are (re)defined in
CMSIS which cause a clash if both are used at a time.
@JonatanAntoni
Copy link
Member

Hi Reto,

I've now added the preprocessor guards to work around your immediate issue.
Does this work for you, midterm? I want to rethink this for the long run.

Cheers,
Jonatan

@CanastraRF
Copy link
Author

Hi Jonatan
Thanks. This is OK for me.
If You find a better solution then implement it.
Reto Felix

0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jul 12, 2021
Reference: CMSIS 5.8.0 known issues and ARM-software/CMSIS_5#1211

This fixes the error about redefinition of enable/disable irq. we need compat header because of
semihosting (not yet provided in CMSIS).
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jul 19, 2021
Reference: CMSIS 5.8.0 known issues and ARM-software/CMSIS_5#1211

This fixes the error about redefinition of enable/disable irq. we need compat header because of
semihosting (not yet provided in CMSIS).
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jul 27, 2021
Reference: CMSIS 5.8.0 known issues and ARM-software/CMSIS_5#1211

This fixes the error about redefinition of enable/disable irq. we need compat header because of
semihosting (not yet provided in CMSIS).
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jul 29, 2021
Reference: CMSIS 5.8.0 known issues and ARM-software/CMSIS_5#1211

This fixes the error about redefinition of enable/disable irq. we need compat header because of
semihosting (not yet provided in CMSIS).
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jul 29, 2021
Reference: CMSIS 5.8.0 known issues and ARM-software/CMSIS_5#1211

This fixes the error about redefinition of enable/disable irq. we need compat header because of
semihosting (not yet provided in CMSIS).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants