Making code MISRA C compliant makes code less readable.

I notice you are using a defined value BIT0. I don't know from the example whether this is a constant or a macro. Either way, would you be happy to use the same approach for an unsigned 8 bit value 1? Your example could then look something like this:

uint8_t some_register = 0;
some_register |= (ONE << BIT0) // ONE is defined as uint8_t with value 1

Note that unlike your example, this won't work reliably if BIT0 is ever greater than 7. If you need to support that case, you probably need to stick with performing the leftshift on a larger type and casting to uint8_t afterwards.

posted 2 days ago

CC BY-SA 4.0

2d ago

trichoplax‭

934 reputation 1 21 91 31

The core problem that this rule is supposed to address is that 8 bit arithmetic actually doesn't exist in C. All expressions are promoted to larger integer types and that's a language flaw one has to be aware of. The rule also seeks to prevent accidental overflows like my_uint8 = 1u << 15;. But it's not really meant to block out idiomatic C code like assigning an integer constant expression to auint8_t variable using = (simple assignment).

Therefore 10.3 comes with exceptions (MISRA-C:2012+Amendments/MISRA-C:2023):

1. An essentially signed integer constant expression, with a rank no greater than signed int, may be assigned to an object of essentially unsigned type if its value can be represented in that type.

This is meant to cover cases like my_uint8 = 1; but should apply in your case too.

• 1u << BIT0 is essentially unsigned. The type of BIT0 is actually not relevant in determining the type of the expression, since the type of a shift expression is always that of the (promoted) left operand.

  Had you written 1 << BIT0 then it would have been essentially signed, but obviously we shouldn't do that since that opens up for signed bitwise arithmetic and would therefore violate a bunch of other MISRA rules.

  So the exception really ought to cover cases of essentially unsigned integer constant expressions too - I'd regard this as a minor defect in the MISRA guidelines.

• 1u << BIT0 is an integer constant expression, given that BIT0 is just some #define (and not a variable etc).

• uint8_t is essentially unsigned.

• Given that BIT0 is one of the numbers 0 to 7, it can fit just fine in a uint8_t.

If all of the above is true and your code had actually been some_register = (1u << BIT0);, then you should be able to claim MISRA compliance without deviations. The explicit cast is not required, but doesn't cause any harm either.

However, that would be true if you used "simple assignment", plain old =. You are using compound assignments such as |=. Compound assignments are intrinsically problematic for type safety as they always come with implicit promotions and that's the actual root of the problem here.

some_register |= (1u << BIT0) is equivalent to
some_register = some_register | (1u << BIT0);

Except some_register, which is probably volatile qualified in a real application, is guaranteed to only be evaluated once during compound assignment. Still, the above means we are no longer dealing with an integer constant expression - some_register | (1u << BIT0) is run-time evaluated.

And what's worse, the resulting type of it is not uint8_t but unsigned int, thanks to the implicit promotion caused by the |. Subtle promotions like that are dangerous and something MISRA tries to stomp out. In this specific case the promotion is harmless, but in other cases, perhaps not.

So actually, if you use compound assignment the cast achieves nothing. Because you'll end up writing code equivalent to

some_register = some_register | (uint8_t)(1u << BIT0);

And that doesn't prevent the implicit promotion of | - there is no way to prevent it. There's not really a fix to it, this is a problem in the C language itself. While both operands of this temporary expression are "essentially unsigned" there are actual no problems in the code - if they aren't, then there are problems and there exists no way to fix them.

So the best practice is actually IMO to avoid compound assignments altogether, due to their built-in type promotion problems.

Furthermore on the hardware level, you can't be quite sure what machine code you end up with when doing compound assignment - do you get a read-modify-write instruction on the assembler level or do you get actual read, then modify, then write separate instructions on a higher level (non-atomic)? If it does the wrong thing, it might cause hiccups if dealing with various oddball flag/status registers in microcontroller peripherals. In case writing 1 clears flags etc, then compound assignment wouldn't work anyway and you'd have to use simple assignment, or otherwise you'd nuke every set flag in the register rather than preserving it.

In your specific case, there was never a need to use compound assignment in the first place - just use simple assignment and overwrite the old register value. In case you do need to preserve various flags, maybe consider doing it in a more explicit way:

uint8_t some_register = SOME_REGISTER; // read actual hw register to RAM
some_register = (uint8_t)(some_register | (1u << BIT0); // prep the contents in RAM
SOME_REGISTER = some_register; // write to hw register

This sates other MISRA rules such as not accessing a volatile variable twice within an expression. Is it less readable? Not really. Is it more verbose? Indeed. But it shows that "hey I actually know what I'm doing, I'm not just aimlessly typing C code with no clue of what types I'm even using".

Other remarks:

• static_cast<uint8_t> is absolutely not compliant since that's C++. If you got a tool telling you otherwise, you got some serious tooling problems. MISRA C:2012/2023 requires ISO C90, C99 or C11. You cannot use C++, you can not use non-standard extensions like GNU C etc etc.

posted about 6 hours ago

CC BY-SA 4.0

5h ago

Lundin‭

9971 reputation 44 135 1154 189