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

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); }
}

• This doesn't do what you want since 0b1 & relayBitmap is either 0 or 1. So you only every get one of the first two options. Nov 30, 2015 at 5:08
• Why is it else if? What should happen if relayBitmap is 0b1011, and why? Nov 30, 2015 at 5:12
• Also, I think it would be helpful if you also showed your relayOn() and relayOff() functions. Nov 30, 2015 at 5:12
• 0b is not part of ISO C. Consider using hex instead (0x01, 0x02, 0x04, 0x08 respectively in your code)
– M.M
Nov 30, 2015 at 6:08
• If you're only testing a single bit, you dont need the == part
– M.M
Nov 30, 2015 at 6:08

## 5 Answers

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;
}
}


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); }
}

• The logic makes sense but not this line: const bool on = (relayBitmap & (1 << i)) != 0; Why const? And how can = and != live in one declaration? Nov 30, 2015 at 5:05
• @LShaver any expression can be used to build up larger expressions using the operators . Although this code isn't particularly readable
– M.M
Nov 30, 2015 at 6:10

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));
}


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); }

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


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); }
}
}