7

I'm facing MISRA C 2012 violation that I can't understand. Following is the code:

#define I2C_CCRH_FS      ((uint8_t)0x80)
#define I2C_CCRH_DUTY    ((uint8_t)0x40)
#define I2C_CCRH_CCR     ((uint8_t)0x0F)

typedef struct I2C_struct
{
  volatile uint8_t CR1;
  volatile uint8_t CR2;
  volatile uint8_t CCRL;
  volatile uint8_t CCRH;
} I2C_TypeDef;

#define I2C_BaseAddress         0x5210
#define I2C ((I2C_TypeDef *) I2C_BaseAddress)

I2C->CCRH &= ~(uint8_t)((I2C_CCRH_FS | I2C_CCRH_DUTY) | I2C_CCRH_CCR);

In the previous code, PC-Lint complains that:

Unpermitted operand to operator '|' [MISRA 2012 Rule 10.1, required]

Mismatched essential type categories for binary operand [MISRA 2012 Rule 10.4, required]

Rule 10.1 states that there should be no problem ORing unsigned ints. (PC-Lint passes the first OR operation and complains about the second one!!)

Rule 10.4 states that the operands of the operation shall have the same essential type.

I can't understand why these violations exist although all of the operands are declared as uint8_t?

I've tried puting parentheses around each two of the ORed constants. I've also tried casting all of them to uint8_t and to volatile uint8_t. Non solved the violation.

I checked these two posts (1, 2) but they don't answer my question.

cafce25
  • 15,907
  • 4
  • 25
  • 31
Salahuddin
  • 1,617
  • 3
  • 23
  • 37
  • Not your problem, but: Why did you write `(I2C_CCRH_FS | I2C_CCRH_DUTY) | I2C_CCRH_CCR`? Would there be something wrong with the unparenthesized `I2C_CCRH_FS | I2C_CCRH_DUTY | I2C_CCRH_CCR`? – Steve Summit Jun 07 '18 at 09:34
  • 1
    If the solution is to add extra casts, as in the accepted answer (which strikes me as a classic example of making code *worse* in order to placate a style guide, but that's another issue), it seems to me it would be better do eliminate the casts in the original bitmask definitions. Why not just say `#define I2C_CCRH_FS 0x80` and the like? (Or `#define I2C_CCRH_FS 0x80u` if it's important to emphasize that they're unsigned?) That way you won't need any casts in the expression, either. MISRA will be happy, and people who (rightly) frown on explicit casts will be happy, too. – Steve Summit Jun 07 '18 at 09:38
  • This code is a snippet of a big library I'm using in my project (stm library), therefore it is hard to change the definition of the variables. As I'm working on a safety-relevant project, I have to check this library against MISRA. The answer that I accepted just solved the problem. I thought there is no better solution. Yes, I have a line of code where there is ORing between 10 operands. Therefore, casting every result to `uint8_t` will make the code harder to read and more error prone. So I decided just to disable the error message for this specific line. Do you have a better solution? – Salahuddin Jun 07 '18 at 09:44
  • This ends up being an imponderable question, and I'm afraid I cannot advise you. Is your top goal to (a) make the code safe, (b) make the code readable, (c) satisfy MISRA, or (d) avoid massive changes that would require so much review and approval that they're prohibitively expensive? (Unfortunately goals (a), (b), and (c) are not always aligned.) If it were me, I wouldn't hesitate to remove the casts in the definitions -- but that's just me, and I can't adequately weigh my choice against goals (c) and (d), because those goals have never applied to my work. – Steve Summit Jun 07 '18 at 09:51
  • @SteveSummit I'd do all of a, b, c and d, see my answer :) Dropping the cast and using `u` suffix makes MISRA happy and the code safe. – Lundin Jun 08 '18 at 11:59

3 Answers3

3

I2C_CCRH_FS | I2C_CCRH_DUTY is MISRA-compliant by itself. Both operands are essentially unsigned so that sub-expression is fine. However, there is still an implicit conversion of each operand to int. The result in practice is of type int.

In pseudo code: when you do (result as int) | I2C_CCRH_CCR, the operands before implicit promotion have the types int | uint8_t. The uint8_t will get integer promoted to int here too. You have operands of different signedness.

(I guess that the tool complains about 10.4 since the integer promotions are part of the usual arithmetic conversiosn, which is what 10.4 is about.)

This whole expression doesn't cause any problems in practice so the warning is mostly pedantic. But imagine if you had done ~(I2C_CCRH_FS | I2C_CCRH_DUTY) | I2C_CCRH_CCR) with no casts - you'd end up with a negative number, something along the lines of 0xFFFFFFxx expressed in 2's complement. That would potentially be dangerous.

To fix this, you have two options:

  • For every operation, cast the result back to the intended type. This is often the spirit of MISRA-C.
  • Cast the operands to a large unsigned type before the operation. Usually more readable IMO.

Note also that the ~ operator should not be used with a signed operand! This is a violation of rule 10.1. The cast back to uint8_t should be done last.

TL;DR. How to get the code MISRA compliant:

You'd either have to do something semi-awful like this:

I2C->CCRH &= (uint8_t) ~ (uint8_t) ((uint8_t)(I2C_CCRH_FS | I2C_CCRH_DUTY) | I2C_CCRH_CCR)

That's a bit of a mess. I would instead cast in advance. Assuming 32 bit CPU:

I2C->CCRH &= (uint8_t) ~( (uint32_t)I2C_CCRH_FS    |  // comment explaining FS
                          (uint32_t)I2C_CCRH_DUTY) |  // comment explaining DUTY
                          (uint32_t)I2C_CCRH_CCR   ); // comment explaining CCR

The above style is useful when dealing with MCU registers and similar. This code is acceptable, but could be simplified further.

If it is possible to change the defines to #define I2C_CCRH_FS 0x80u, then you get:

I2C->CCRH &= (uint8_t) ~(I2C_CCRH_FS | I2C_CCRH_DUTY | I2C_CCRH_CCR);

and it would still be MISRA compliant, because of the handy little u suffix that MISRA likes.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • Thanks for the informative answer. The Integer promotion rule forces us to cast mostly every `unsigned char` to `unsigned int` in bitwise operations to stay MISRA compliant, right? Could you please clarify why is the result of `~` always `signed int`? Even if the operands are `unsinged int`? – Salahuddin Jun 08 '18 at 12:18
  • 1
    @SalahuddinAhmed Yes most of the time it is a good idea to cast small integer types into unsigned int. The result of ~ is signed int if its operand is a small integer type of any signedness, because of integer promotion. In case of unsigned int, there is no promotion. See [Implicit type promotion rules](https://stackoverflow.com/questions/46073295/implicit-type-promotion-rules). – Lundin Jun 08 '18 at 12:36
  • Actually, you should use U, not u (MISRA C:2012, 7.3). – Matthias Jun 11 '18 at 09:05
  • @Matthias No. The relevant rule is MISRA-C:2012 7.2. "A 'u' or 'U' suffix shall be applied..." – Lundin Jun 11 '18 at 09:09
2

When you perform the bitwise operation (I2C_CCRH_FS | I2C_CCRH_DUTY) the result gets promoted to an integer. See Integer Promotion rules Here

Therefore the mismatch is occurring between the result of the above operation and the next bitwise OR | I2C_CCRH_CCR

To fix this you need to add a cast to the result of BOTH Bitwise OR operations.

The first cast is required to cast the result of the ~ operator from int back to unsigned

I2C->CCRH &= (uint8_t)~(uint8_t)((uint8_t)(I2C_CCRH_FS | I2C_CCRH_DUTY) | I2C_CCRH_CCR);

To Explain 

I2C->CCRH &= (uint8_t)~    // Required to cast result of ~ operator to uint8_t
             (uint8_t)     // Casts the result of the 2nd OR to uint8_t
             ((uint8_t)    // Casts the result of the 1st OR    
              (I2C_CCRH_FS | I2C_CCRH_DUTY)  // 1st OR
              | I2C_CCRH_CCR);      // 2nd OR
Rishikesh Raje
  • 8,556
  • 2
  • 16
  • 31
  • It is still not MISRA compliant because the result of `~` is `int`, which shouldn't be assigned to a type of `uint8_t`. Different essential type category so it violates 10.3. Nice link though :) – Lundin Jun 08 '18 at 11:55
0

I suggest you are making things complicated for yourself... you can keep the #define macros in unsigned int:

#define I2C_CCRH_FS      0x80u   // No need for cast, but add u suffix
#define I2C_CCRH_DUTY    0x40u   // ditto
#define I2C_CCRH_CCR     0x0Fu   // ditto

{snip}

#define I2C_BaseAddress  0x5210u // Add u suffix

// Note: this violates R.11.1 but a MISRA C Permit
//       exists to help with the deviation
#define I2C ((I2C_TypeDef *) I2C_BaseAddress)

// Only one cast required, to bring into 8 bit
I2C->CCRH &= (uint8_t)( ~( I2C_CCRH_FS | I2C_CCRH_DUTY | I2C_CCRH_CCR ) );
Andrew
  • 2,046
  • 1
  • 24
  • 37