Toggle four relays on/off based on bitmap stored in uint8_t

Question

I am using a one-byte bitmap to control four relays. I am writing a function which will take the bitmap stored as uint8_t, look to see which bits are set/cleared, then turn on/off the relays appropriately. The input value to my function could be anything from 0 to 15.

This is in a controls application, so I'd like this to thing to work pretty fast (not insane optimization, since I still have to wait 10ms for my relays to latch).

Here's the function so far (I stripped out the actual relay switching to simplify):

void 
relayToggle(uint8_t relayBitmap)
{
    if ( 0b00000001 & relayBitmap == 0b00000001 ) { relayOn(1); }
    else if ( 0b00000001 & relayBitmap == 0 ) { relayOff(1); }
    else if ( 0b00000010 & relayBitmap == 0b00000010 ) { relayOn(2); }
    else if ( 0b00000010 & relayBitmap == 0 ) { relayOff(2); }
    else if ( 0b00000100 & relayBitmap == 0b00000100 ) { relayOn(3); }
    else if ( 0b00000100 & relayBitmap == 0 ) { relayOff(3); }
    else if ( 0b00001000 & relayBitmap == 0b00001000 ) { relayOn(4); }
    else if ( 0b00001000 & relayBitmap == 0 ) { relayOff(4); }
}

Answer 1

Use a loop to eliminate all that copy-pasta and get rid of some superfluous tests, branches and hard-coded constants:

void relayToggle(uint8_t relayBitmap)
{
    for (int r = 1; r <= 4; ++r)
    {
         if (relayBitmap & 1)
             relayOn(r);
         else
             relayOff(r);
         relayBitmap >>= 1;
    }
}

Answer 2

Even on a very slow CPU/uC, 10ms-order-of-magnitude is all day long.

You need not optimize for execution speed. Instead you should optimize for development. Make sure your code is idiomatic and readable.

Unless there's some real problem with a loop here, you should use that instead.

Iterate from 1 to n, shift-and-mask your way to glory.

For example:

for (int i = 0; i < 4; i++)
{
    const bool on = (relayBitmap & (1 << i)) != 0;
    if (on) { relayOn(i+1); }
    else    { relayOff(i+1); }
}

Answer 3

I'd combine relayOff and relayOn and extract some named constants, and your bit checking. This would give something like:

//Maybe use a function instead.
#define isbitset(X, Y) ((X) & 1<<(Y))

//Name these better
#define RELAY_0 0
#define RELAY_1 1
#define RELAY_2 2
#define RELAY_3 3

void 
relayToggle(uint8_t relayBitmap)
{
    relayState(RELAY_0, isbitset(relayBitmap, RELAY_0));
    relayState(RELAY_1, isbitset(relayBitmap, RELAY_1));
    relayState(RELAY_2, isbitset(relayBitmap, RELAY_2));
    relayState(RELAY_3, isbitset(relayBitmap, RELAY_3));
}

Or giving each relayState(X) a better name

void 
relayToggle(uint8_t relayBitmap)
{
    relay0State(isbitset(relayBitmap, RELAY_0));
    relay1State(isbitset(relayBitmap, RELAY_1));
    relay2State(isbitset(relayBitmap, RELAY_2));
    relay3State(isbitset(relayBitmap, RELAY_3));
}

Answer 4

This does not "see which bits are set/cleared". Code only operates on 1 of relayBitmap. To work as described, code would need to

void relayToggle(uint8_t relayBitmap){
    if ( 0b00000001 & relayBitmap == 0b00000001 ) { relayOn(1); }
    else if ( 0b00000001 & relayBitmap == 0 ) { relayOff(1); }

    if ( 0b00000010 & relayBitmap == 0b00000010 ) { relayOn(2); }
    else if ( 0b00000010 & relayBitmap == 0 ) { relayOff(2); }

    ...
}

Note that binary constants are not standard C. Consider using a decimal/hexadecimal constant and simplified code

   if (1 & relayBitmap) { relayOn(1); }
   else { relayOff(1); }

   if (2 & relayBitmap) { relayOn(2); }
   else { relayOff(2); }

Or compact method

   void (*f[2])(int) = {relayOff; relayOn};
   for (unsigned i=0; i<4; i++) {
     f[!!((1u<<i) & relayBitmap)](i+1);
   }

Answer 5

Figured this made more sense as a new answer. I made some changes to the code and included the relayOn(),relayOff(), and relayLatch() functions.

The main changes are using the for loop as many of you suggested (duh!), and a new feature, checking to see if the relay is already in the state requested.

Set state of all relays:

uint8_t relayStatus;        // Global holder for relay status

void 
relaySet(uint8_t relayCmd)
{
    char printStatus[7];    // For updating status on LCD screen

    // Set the relays on/off as required. Update display.
    for ( int r=0; r<4; r++ )
    {
        if ( relayCmd & (1 << r) != relayStatus & (1 << r) ) 
        { 
            if ( relayCmd & (1 << r) ) 
            { 
                relayOn(r);
                snprintf (printStatus,7,"R%u ON ", r+1 );
                LCDstring(r,0,printStatus,NORMAL);
            }
            else 
            { 
                relayOff(r); 
                snprintf (printStatus,7,"R%u OFF", r+1 );
                LCDstring(r,0,printStatus,INVERSE);
            }
        }
    }

    // Wait 10ms
    ROM_SysCtlDelay(ROM_SysCtlClockGet()/100);

    // Set appropriate pins back to ground
    for ( int r=0; r<4; r++ )
    {
        if ( relayCmd & (1 << r) != relayStatus & (1 << r) ) 
        { 
            relayLatch(r);
        }
    }

    // Update global relay status
    relayStatus = relayCmd;
}

Turn single relay on:

void 
relayOn(int relayNum)
{
    if ( relayNum == 0 )
    {
        // Rel1
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_1, GPIO_PIN_1); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_0, GPIO_PIN_0); }
    }
    else if ( relayNum == 1 )
    {
        // Rel2
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_3, GPIO_PIN_3);
    }
    else if ( relayNum == 2 )
    {
        // Rel3
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_2, GPIO_PIN_2);
    }
    else if ( relayNum == 3 )
    {
        // Rel4
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_4, GPIO_PIN_4); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_4, GPIO_PIN_4); }
    }
}

Turn single relay off:

void 
relayOff(int relayNum)
{
    if ( relayNum == 0 )
    {
        // Rel1N
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_4, GPIO_PIN_4);
    }
    else if ( relayNum == 1 )
    {
        // Rel2N
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_5, GPIO_PIN_5);
    }
    else if ( relayNum == 2 )
    {
        // Rel3N
        GPIOPinWrite(GPIO_PORTB_BASE, GPIO_PIN_5, GPIO_PIN_5);
    }
    else if ( relayNum == 3 )
    {
        // Rel4N
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_1, GPIO_PIN_1); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_0, GPIO_PIN_0); }
    }
}

Latch single relay:

void 
relayLatch(int relayNum)
{
    if ( relayNum == 0 )
    {
        // Rel1
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_1, 0); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_0, 0); }
        // Rel1N
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_4, 0);
    }
    else if ( relayNum == 1 )
    {
        // Rel2
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_3, 0);
        // Rel2N
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_5, 0);
    }
    else if ( relayNum == 2 )
    {
        // Rel3
        GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_2, 0);
        // Rel3N
        GPIOPinWrite(GPIO_PORTB_BASE, GPIO_PIN_5, 0);
    }
    else if ( relayNum == 3 )
    {
        // Rel4
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_4, 0); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTF_BASE, GPIO_PIN_4, 0); }
        // Rel4N
        if (hardwareRev == 1) { GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_1, 0); }
        else if (hardwareRev == 2) { GPIOPinWrite(GPIO_PORTE_BASE, GPIO_PIN_0, 0); }
    }
}