On core_cm4.h why is there casting like ((uint32_t)(int32_t)IRQn)?

Question

In the following code from core_cm4.h why is there a double cast ((uint32_t)(int32_t)IRQn)?

For example in the following function:

__STATIC_INLINE void NVIC_EnableIRQ(IRQn_Type IRQn)
{
  NVIC->ISER[(((uint32_t)(int32_t)IRQn) >> 5UL)] = (uint32_t)(1UL << (((uint32_t)(int32_t)IRQn) & 0x1FUL));
}

What is the purpose of this?

the first cast to int32_t sign extends the enum to 32bits and the second cast makes it unsigned so that the >> 5UL will be a logical shift. — cleblanc, Mar 29 '17 at 13:57
There are several options, where double casting is used: * They are trying to store the data in two halves. one unsigned + one signed. * Not knowing the type of value, you can't say, but in general, casting through an unsigned type guarantees a positive value in the long (unless the positive value won't fit). The idea is to get the value unsigned, to avoid sign extension for hex and octal representation. — danglingpointer, Mar 29 '17 at 14:13

unalignedmemoryaccess · Accepted Answer · 2017-03-29T17:21:29.883

4

Since CM4 software implements negative interrupt sources for core, you have to cast value first to 32bit signed integer and later to unsigned 32bit to make proper right shift with padding zeros on the left side of number.

CM4 uses -15 to -1 as CM4-Core sources and from 0 to next as vendor specific sources.

edited Mar 29 '17 at 17:21

answered Mar 29 '17 at 14:12

unalignedmemoryaccess

7,246
2
25
40

Yes, it obvious what it's doing now I am thinking about it properly. – Realtime Rik Mar 29 '17 at 14:32
1

Now you are thinking about it properly or now you have been told the answer? – Ant Mar 30 '17 at 15:36

Lundin · Answer 2 · 2017-09-28T13:22:05.507

3

By looking at that snippet, it is clear that whoever wrote it was confused over how implicit type promotion rules work. In addition, the >> 5UL looks very fishy and when I see that I immediately suspect that this code base suffers from a poor understanding of MISRA-C; possibly they are using a bad static analyser which spits out false positives.

A visit at Github proves my suspicion to be correct, there's comments stating that the intention is to follow MISRA-C:2004.

MISRA-C demands that no dangerous, implicit type promotions may occur. The casts are some failed attempts to silence a static analyser, without an understanding why the tool gave those warnings.

Correct, MISRA-C (2004 and 2012) compliant code would be this:

NVIC->ISER[((uint32_t)IRQn>>5UL)] = (1UL << ((uint32_t)IRQn & 0x1FUL));

(MISRA-C has a requirement that complex sub-expressions must use parenthesis despite what operator precedence there is.)

This code is still messy, so it should preferably be rewritten for readability, into something that will produce exactly the same machine code:

uint32_t ISER_index = ((uint32_t)IRQn >> 5UL);
uint32_t shift_n    = ((uint32_t)IRQn & 0x1FUL);
NVIC->ISER[ISER_index] = (1UL << shift_n);

Side note:
MISRA-C:2004 demanded that the result of the shift expression should be immediately cast to the "underlying type" (MISRA term). Thus one could also have written (IRQn_Type)(IRQn >> 5UL) and it would still be MISRA-C:2004 compliant.

However, there is nothing in MISRA preventing you to adding your own cast to a different type, such as uint32_t, before the shift. This is a better thing to do, since it eliminates the implicit promotion entirely.

For those who are confused about how the implicit type promotions work in C, see Implicit type promotion rules.

edited Sep 28 '17 at 13:22

answered Sep 28 '17 at 13:02

Lundin

195,001
40
254
396

Additional cast is more redundant than hiding warning. – unalignedmemoryaccess Sep 28 '17 at 13:09
@tilz0R The cast to uint32_t is definitely needed, as `IRQn` could be negative. Right-shifting a negative value invokes implementation-defined behavior - they might end up with arithmetic shifts which would introduce a severe bug on some systems. This is ultimately what the MISRA-C rule is trying to protect from. – Lundin Sep 28 '17 at 13:15
The only problem is int32 cast. And very fishy? – unalignedmemoryaccess Sep 28 '17 at 13:21
@tilz0R Yeah the `int32_t` cast but also general readbility. The vast amount of parenthesis and the "everything on one line" coding style makes the code very hard to read & dangerous to maintain. They would benefit from splitting the big expression up in several - the compiler will produce the same machine code in the end anyway. – Lundin Sep 28 '17 at 13:24
Are you trying to say that your oneline modification without spaces is more readable? – unalignedmemoryaccess Sep 28 '17 at 13:26
@tilz0R No, it is quite horrible too. Hence the 3 line snippet in the end. – Lundin Sep 28 '17 at 13:28
Ok. I think that you are a little overreacting here. Removing int32 cast from original would be best and thats all :) – unalignedmemoryaccess Sep 28 '17 at 13:31
@tilz0R I'm not overreacting, have you seen the [full source](https://github.com/ARM-software/CMSIS/blob/master/CMSIS/Include/core_cm4.h)? It's unreadable garbage. – Lundin Sep 28 '17 at 13:33
Im using it daily :) – unalignedmemoryaccess Sep 28 '17 at 13:34
@tilz0R Probably I am too, or some flavour of CMSIS at least. Just because some code works doesn't mean it is well-written. – Lundin Sep 28 '17 at 13:35
I do agree here. – unalignedmemoryaccess Sep 28 '17 at 13:37

score 2 · Answer 3 · answered Sep 28 '17 at 12:23

2

Assuming IRQn is an integer (any signed integer type) in the range you say, then (uint32_t)(int32_t)IRQn is exactly the same as (uint32_t)IRQn.

The author of the code possibly didn't understand C type conversion rules; which are based on values, not representations. Converting -3, for example, to uint32_t always gives UINT32_MAX - 2 regardless of which data type the -3 was. It's the value of "negative 3" that matters.

(The other answer explains the difference between using a cast and no cast at all).

answered Sep 28 '17 at 12:23

M.M

138,810
21
208
365

I believe this is the correct answer. Bluntly: whoever wrote the code didn't quite know what they were doing. – Lundin Sep 28 '17 at 12:35
@Lundin in fact to both of you. Even if there is `(int32_t)` cast, doesn't mean it is wrong! – unalignedmemoryaccess Sep 28 '17 at 12:38
@tilz0R I'm not saying it's *wrong* per se, just that it's both redundant and confusing as evinced by the fact that someone went and posted this question because they didn't understand why someone would do that – M.M Sep 28 '17 at 12:57
1

@tilz0R I looked at the code in Github, this is definitely another MISRA-C project gone bad, I see such projects quite often so I know what to look for. There is no rationale for the casts, it was just someone who didn't know MISRA-C who tried to silence their static analyser. I posted an answer explaining. – Lundin Sep 28 '17 at 13:05
@Lundin agree, but. Can you name me 3 professional misrac libs? – unalignedmemoryaccess Sep 28 '17 at 13:07
@tilz0R It is definitely nice that they write the code with MISRA-C in mind, it makes things so much easier for users of the lib that need MISRA-C compliance. The code is probably functionally correct... but it is [incredibly ugly code!](https://github.com/ARM-software/CMSIS/blob/master/CMSIS/Include/core_cm4.h) I would guess they wrote the code first, then added MISRA compliance later, why it turned out like it did. – Lundin Sep 28 '17 at 13:11

On core_cm4.h why is there casting like ((uint32_t)(int32_t)IRQn)?

3 Answers3

Linked

Related