How do I configure microcontroller registers with no linter messages (embedded C)

Question

I like writing embedded C firmware to the BARR-C:2018 standard and I have recently started using PC-Lint to check the code. I am currently using a PIC32 processor and I want to configure a register such that it:

1. is easy to read,
2. generates no linter messages,
3. compiles to a small amount of assembler.

I will outline my options below, with pros and cons. I am interested to hear your solutions. The register I have chosen to illustrate the problem is not important.

Bit-fields with Microchip-supplied header

✓ No lint messages, easy to read, easy to change individual settings

✗ 20 assembly instructions (O0) (increases in more complex registers)

ADC0TIMEbits.SAMC   = 25U;
ADC0TIMEbits.ADCDIV = 50U;
ADC0TIMEbits.BCHEN  = 0U;
ADC0TIMEbits.SELRES = 2U;
ADC0TIMEbits.ADCEIS = 6U;

Simple assignment

✓ No lint messages, 2 assembly instructions (O0)

✗ Not easy to read, not easy to change individual settings

ADC0TIME = 0x1A190019U;

Explicitly consider every bit in every register

✓ No lint messages, 2 assembly instructions (O0), easy to change individual settings

✗ Not easy to verify that the shift values are correct

ADC0TIME = ((25U << 0U) |     /* ADC0 Sample Time bits */
            (50U << 16U) |    /* ADC0 Clock Divisor bits */
            (0U  << 23U) |    /* Buffer Channel Enable bit */
            (2U  << 24U) |    /* ADC0 Resolution Select bits */
            (6U  << 26U));    /* ADC0 Early Interrupt Select bits */

Explicitly consider every bit in every register using definitions from Microchip's supplied header file

✓ 2 assembly instructions (O0), easy to change individual settings, shift values are controlled by manufacturer and are human-readable

✗ Values in the supplied header are signed, which lead to Lint info 8524: combining signed and unsigned types with operator '<<' [BARR-C:2018 Rule 5.3c]

ADC0TIME = ((25U << _ADC0TIME_SAMC_POSITION) |      /* ADC0 Sample Time bits */
            (50U << _ADC0TIME_ADCDIV_POSITION) |    /* ADC0 Clock Divisor bits */
            (0U  << _ADC0TIME_BCHEN_POSITION) |     /* Buffer Channel Enable bit */
            (2U  << _ADC0TIME_SELRES_POSITION) |    /* ADC0 Resolution Select bits */
            (6U  << _ADC0TIME_ADCEIS_POSITION));    /* ADC0 Early Interrupt Select bits */

As above + casting the constants to get rid of the linter message

✓ 2 assembly instructions (O0), easy to change individual settings, shift values are controlled by manufacturer and are human-readable, no lint messages

✗ Casting looks cumbersome

ADC0TIME = ((25U << (uint32_t)_ADC0TIME_SAMC_POSITION) |      /* ADC0 Sample Time bits */
            (50U << (uint32_t)_ADC0TIME_ADCDIV_POSITION) |    /* ADC0 Clock Divisor bits */
            (0U  << (uint32_t)_ADC0TIME_BCHEN_POSITION) |     /* Buffer Channel Enable bit */
            (2U  << (uint32_t)_ADC0TIME_SELRES_POSITION) |    /* ADC0 Resolution Select bits */
            (6U  << (uint32_t)_ADC0TIME_ADCEIS_POSITION));    /* ADC0 Early Interrupt Select bits */

Alternatively, I could modify the Microchip-supplied header and define the constants as unsigned. That means the header is non-standard and I need to include it in my project repository. Is that a problem?

Edit: Extract from Microchip header file. Values are signed:

#define _ADC0TIME_SAMC_POSITION                  0x00000000
#define _ADC0TIME_SAMC_MASK                      0x000003FF
#define _ADC0TIME_SAMC_LENGTH                    0x0000000A

#define _ADC0TIME_ADCDIV_POSITION                0x00000010
#define _ADC0TIME_ADCDIV_MASK                    0x007F0000
#define _ADC0TIME_ADCDIV_LENGTH                  0x00000007

#define _ADC0TIME_BCHEN_POSITION                 0x00000017
#define _ADC0TIME_BCHEN_MASK                     0x00800000
#define _ADC0TIME_BCHEN_LENGTH                   0x00000001

#define _ADC0TIME_SELRES_POSITION                0x00000018
#define _ADC0TIME_SELRES_MASK                    0x03000000
#define _ADC0TIME_SELRES_LENGTH                  0x00000002

#define _ADC0TIME_ADCEIS_POSITION                0x0000001A
#define _ADC0TIME_ADCEIS_MASK                    0x1C000000
#define _ADC0TIME_ADCEIS_LENGTH                  0x00000003

Answer 1

How do I configure microcontroller registers with no linter messages

Use the option As above + casting the constants to get rid of the linter message.

Answer 2

The amount of assembler instructions under O0 is irrelevant. However, since these are hardware peripheral registers and volatile-qualified, the optimizer can only do so much about the code when enabled.

Bit-fields with Microchip-supplied header

As-is, this is definitely the wrong solution to any problem, because you will end up writing to the same register multiple times. And bit-fields are problematic for numerous other reasons too. You could only use this in a sensible way if storing all results into a temporary, non-volatile variable in RAM and then copy it into the actual register with a single assignment.

ADC0TIME = 0x1A190019U;

This is an unreadable mess of "magic numbers" and therefore obviously it should be avoided like the plague.

Explicitly consider every bit in every register

This is a bit better but you are still using "magic numbers".

Explicitly consider every bit in every register using definitions from Microchip's supplied header file

This is close to the "de facto" standard way of using hardware registers. It is quite readable but the left operands of the shifts should be named constants or at least explained with comments.

As above + casting the constants to get rid of the linter message

This is kind of nonsense. In case the provided Microchip constants are of a small integer type, then they would get implicitly integer promoted to int which I take it is 32 bits on your MCU.

I don't know the Michael Barr standard, but it's basically a light version of MISRA C and in MISRA C, you aren't allowed to have implicit integer promotions. And MISRA C isn't specific enough as to make an exception to the right operand of shifts.

Because the right operand of shifts is harmless. It is integer promoted but otherwise does not affect the end result at all. The result of shifts is always of the type of the (promoted) left operand.

Please note that all MISRA/Barr concerns about shifts are mainly focused on using a signed left operand, which is pretty much always wrong. Or more far-fetched, maybe a negative right operand which would be senseless.

---

What you should do:

Use the "using definitions from Microchip's supplied header file" version but if possible with even less magic numbers.

On a 32-bit MCU then use 32 bit unsigned constants. If the constants provided by Microchip aren't unsigned 32, then their library is broken. If they are but your linter is whining about them still, then your linter is broken.

There may be commercial compilers with register support for this particular MCU, which could be an option in case the Microchip libraries are broken. Otherwise rolling out your own register map isn't that hard, I've done this on several occasions by parsing the text from the pdf manual and translated it to C source.

Misc tips & tricks can be found here: How to access a hardware register from firmware?

Answer 3

This is not a good solution. But when the last statement is too long for you, you can create a complicated network of macros to make that statement automatically.

Consider this monster (stolen from https://stackoverflow.com/a/5048661/6082851):

//Multiple macros to create a single statement to set a SFR
//v is the name of the SFT, a the Position in the register and b the value to set
#define CREATE_STATEMENT_1(v,a,b)  ( (b) << (uint32_t) ( _ ## v ## _ ## a ## _POSITION )  ) 

#define CREATE_STATEMENT_2(v,a,b,c,d) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_1(v,c,d)
#define CREATE_STATEMENT_3(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_2(v,__VA_ARGS__)
#define CREATE_STATEMENT_4(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_3(v,__VA_ARGS__)
#define CREATE_STATEMENT_5(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_4(v,__VA_ARGS__)
#define CREATE_STATEMENT_6(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_5(v,__VA_ARGS__)
#define CREATE_STATEMENT_7(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_6(v,__VA_ARGS__)
#define CREATE_STATEMENT_8(v,a,b,...) CREATE_STATEMENT_1(v,a,b) | CREATE_STATEMENT_7(v,__VA_ARGS__)


//MACRO_SELECTOR(...) evaluates to the literal number of the passed-in arguments divided by 2 
//(because we need 2 parameters per macro).
#define MACRO_SELECTOR_N1(X,_8,_8_1,_7,_7_1,_6,_6_1,_5,_5_1,_4,_4_1,_3,_3_1,_2,_2_1,_1,_1_1,N,...) N
#define MACRO_SELECTOR(...) MACRO_SELECTOR_N1(0, __VA_ARGS__ ,8,9,7,7,6,6,5,5,4,4,3,3,2,2,1,1,0,0)

#define CREATE_STATEMENT_N2(V,N,...)  CREATE_STATEMENT_ ## N(V,__VA_ARGS__)
#define CREATE_STATEMENT_N1(V,N,...)  CREATE_STATEMENT_N2(V,N,__VA_ARGS__)
//Create a single statement to set a SFR.
//V is the name of the register, after that is a list of position in that register and after that,
// the value it should be set to.
#define CREATE_STATEMENT(V,...)       V=CREATE_STATEMENT_N1(V,MACRO_SELECTOR(__VA_ARGS__),__VA_ARGS__);

If you put that in a header, you can use this shorter way to set a register:

CREATE_STATEMENT
  (
    ADC0TIME,
    SAMC,     25U,
    ADCDIV,   50U,
    BCHEN,     0U,
    SELRES,    2U,
    ADCEIS,    6U
  );

This will result in

ADC0TIME=( (25U) << (uint32_t) (_ADC0TIME_SAMC_POSITION) ) | ( (50U) << (uint32_t) (_ADC0TIME_ADCDIV_POSITION) ) | ( (0U) << (uint32_t) (_ADC0TIME_BCHEN_POSITION) ) | ( (2U) << (uint32_t) (_ADC0TIME_SELRES_POSITION) ) | ( (6U) << (uint32_t) (_ADC0TIME_ADCEIS_POSITION) );

EDIT: Add _<registername>_ and _POSITION automatically.