Is it bad practice to redefine register masks for PIC24 in order to improve readability?

Question

I am relatively new to working with PIC chips, so this might be a novice-level question, but I am attempting to write a header file containing, among other things, the TRIS/ODC/INIT masks for all of the I/O ports.

On the PCB this chip is built into, any given component is likely to use pins from multiple ports, and there are probably a dozen or so individual components that warrant detailed commenting. For instance, interfacing with a particular SPI ADC module uses pins from ports A, D, and F.

To me, it would seem that the reader-friendly way to write this would be to organize the file by component such that, at a glance, the reader can tell what pins are being used, whether they are configured as inputs or outputs, and how they are initialized.

For instance, showing only the TRIS mask information, here is a snippet of code for a particular ADC module I am using to demonstrate what I am talking about:

#define PORTD_TRIS_MASK 0x00
#define PORTF_TRIS_MASK 0x00

// ...
// lots of hardware configuration stuff goes here
// ...

// ANALOG INPUT - THERMOCOUPLE 1
// Thermocouple ADC chip MAX31856 DNP communicates over SPI
// Accepts any type of thermocouple
// TC1_CS pulled high
// TC1_DRDY pulled high

#define TC1_MOSI    LATAbits.LATA14
#define TC1_MISO    PORTDbits.RD10
#define TC1_SCK     LATDbits.LATD11
#define TC1_CS      LATFbits.LATF6
#define TC1_DRDY    PORTFbits.RF7

#define TC1_MISO_SHIFT  1<<10
#define TC1_DRDY_SHIFT  1<<7

#define PORTD_TRIS_MASK ( PORTD_TRIS_MASK | TC1_MISO_SHIFT )
#define PORTF_TRIS_MASK ( PORTF_TRIS_MASK | TC1_DRDY_SHIFT )

The above code does not throw any errors, but it does throw a warning:

HardwareProfile.h:1173:0: warning: "PORTD_TRIS_MASK" redefined
HardwareProfile.h:1029:0: note: this is the location of the previous definition
HardwareProfile.h:1174:0: warning: "PORTF_TRIS_MASK" redefined
HardwareProfile.h:1095:0: note: this is the location of the previous definition

The fact that the compiler is complaining about it suggests to me that this may not be encouraged behavior, but nothing about it seems intrinsically problematic to me. Am I missing something, or is this a reasonable way to organize code such that pin configuration details are kept near their definitions?

Alternatively, is there a more conventional way to accomplish what I want to accomplish as far as maintaining readability that is more widely used or acceptable?

---

Update:
Perhaps I was not clear enough in my original post. It is structured that way because there are a dozen or so such blocks of code in the header file. Supposing there are exactly 13 such blocks of code, any particular mask would be initially defined as 0x00 and redefined 13 times, the idea being that each redefinition adds the configuration information relevant to a particular block.

---

Update:
In response to a question about how these macros are used, they are simply used to configure all pins in a port at once. On this PIC24, each port has 16 pins, each of which has a TRIS (data direction control) register, ODC (open drain control) register, and LAT (latch) register that, if configured as an output, will need an initial value. Conventionally, writing to one bit at a time sixteen times is discouraged in favor of writing to the entire port once. For instance, consider a simplified case where there are four registers instead of sixteen. Instead of writing this:

// In source file
TRISABITS.TRISA0 = 1;
TRISABITS.TRISA1 = 1;
TRISABITS.TRISA2 = 0;
TRISABITS.TRISA3 = 0;

It is conventional (as I understand it) to write this:

// In header file
#define BIT_0_SHIFT ( 1<<0 )
#define BIT_1_SHIFT ( 1<<1 )
#define BIT_2_SHIFT ( 0<<2 )
#define BIT_3_SHIFT ( 0<<3 )
#define TRISA_MASK ( BIT_0_SHIFT | BIT_1_SHIFT | BIT_2_SHIFT | BIT_3_SHIFT )

// In source file
TRISA = TRISA_MASK;

Regarding a different question about readability, my argument in favor of this structure is that the way ports are organized on this chip is not physically meaningful. Pins on any particular port are not necessarily near each other or in order, and no individual device (e.g. external SPI module) is confined to a single port. Organizing the header file by port means that the reader needs to scroll through the entire header file to examine the configuration of a single device, while organizing the file by device allows an entire device's definitions and configurations to be clearly visible on a single screen.

Answer 1

The preprocessor does not work the same way as code works. For example, consider the following code:

int main(void)
{
    int A = (B+C);
    int B = (C+2);
    int C =  3;
    int x = A;
    return x;
}

That doesn't work because B and C are used before being declared. The output from the compiler is:

cc -Wall demo.c -o demo
demo.c:3:14: error: use of undeclared identifier 'B'
    int A = (B+C);
             ^
demo.c:3:16: error: use of undeclared identifier 'C'
    int A = (B+C);
               ^
demo.c:4:14: error: use of undeclared identifier 'C'
    int B = (C+2);
             ^

Now try the same thing using #defines for A, B, and C:

#define A (B+C)
#define B (C+2)
#define C  3

int main(void)
{
    int x = A;
    return x;
}

This time there are no warnings or errors, even though the #defines are out of order. When the preprocessor sees a #define it simply adds an entry to its dictionary. So after reading the three #defines the dictionary contains

Search   Replacement
 Text       Text
--------------------
   A       (B+C)
   B       (C+2)
   C       3

Note that the preprocessor has not evaluated the replacement text. It simply stores the text. When the preprocessor finds a search term in the code, then it uses the replacement text. So the line

int x = A;

becomes

int x = (B+C);

After performing the substitution, the preprocessor rescans the text to see if more substitutions are possible. After a second scan, we have:

int x = ((C+2)+3);

and the final result is:

int x = ((3 +2)+3);

Most compilers have an option to output the code after preprocessing is finished. With gcc or clang, use the -E option to see the preprocessor output.

---

OK, so now we should have enough background to actually address your question. Consider the following definitions:

#define PORTD_TRIS_MASK 0x00
#define PORTD_TRIS_MASK ( PORTD_TRIS_MASK | TC1_MISO_SHIFT )
#define PORTD_TRIS_MASK ( PORTD_TRIS_MASK | SB1_DATA_SHIFT )

We have 3 major problems here:

1. The substitution text is not being evaluated, so the bits are not being OR'd together.
2. Only one of those definitions (the last one) will be kept in the preprocessor dictionary. That's the reason for the warning. The preprocessor is telling you that it already has an entry for that search term, and it's going to discard the previous replacement text.
3. When PORTD_TRIS_MASK is found in the code, the preprocessor replaces it with ( PORTD_TRIS_MASK | SB1_DATA_SHIFT ). It then rescans, and finds PORTD_TRIS_MASK again. The result is infinite recursion. Fortunately, the preprocessor has protection against such things, and will stop. The compiler will generate an error later.

The solution is to create uniquely named definitions for each component:

#define TRIS_MASK_D1 TC1_MISO_SHIFT
#define TRIS_MASK_F1 TC1_DRDY_SHIFT

#define TRIS_MASK_D2 SB1_DATA_SHIFT
#define TRIS_MASK_F2 0

And then OR them all together:

#define PORTD_TRIS_MASK (TRIS_MASK_D1 | TRIS_MASK_D2 | ... | TRIS_MASK_D13)
#define PORTF_TRIS_MASK (TRIS_MASK_F1 | TRIS_MASK_F2 | ... | TRIS_MASK_F13)