3
\$\begingroup\$

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); }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Michael Anderson Nov 30 '15 at 5:08
  • 1
    \$\begingroup\$ Why is it else if? What should happen if relayBitmap is 0b1011, and why? \$\endgroup\$ – 200_success Nov 30 '15 at 5:12
  • \$\begingroup\$ Also, I think it would be helpful if you also showed your relayOn() and relayOff() functions. \$\endgroup\$ – 200_success Nov 30 '15 at 5:12
  • \$\begingroup\$ 0b is not part of ISO C. Consider using hex instead (0x01, 0x02, 0x04, 0x08 respectively in your code) \$\endgroup\$ – M.M Nov 30 '15 at 6:08
  • \$\begingroup\$ If you're only testing a single bit, you dont need the == part \$\endgroup\$ – M.M Nov 30 '15 at 6:08
2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
1
\$\begingroup\$

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); }
}
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – LShaver Nov 30 '15 at 5:05
  • \$\begingroup\$ @LShaver any expression can be used to build up larger expressions using the operators . Although this code isn't particularly readable \$\endgroup\$ – M.M Nov 30 '15 at 6:10
1
\$\begingroup\$

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));
}
\$\endgroup\$
1
\$\begingroup\$

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);
   }
\$\endgroup\$
0
\$\begingroup\$

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); }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.