6
\$\begingroup\$

I have an MCU (TI Tiva TM4C) that operates a GSM modem, LCD display, keypad, ADC inputs, and four relays for microgrid control/operation.

To date I have about 2000 lines of code, mostly for supporting peripherals. Now the hard work of creating an algorithm begins.

Before I do that, I am hoping a few embedded C experts could scan my code, notice my rookie mistakes, and point them out to me. Then I can do some digging and learn how to write more robust code, to make sure my foundational code is solid.

The main function is below. Other functions are on GitHub.

int
main(void)
{
    char aString[2][128];               // Generic string
    int anInt;                          // Generic int
    int msgOpen = 0;                    // Message being processed
    int ctr1;                           // Generic counter
    uint32_t pui32ADC0Value[1];         // ADC0 data value
    uint32_t ui32D0v;                   // mV value on external input D0

    // Initial settings - from Anil
    ROM_FPUEnable();                    // Enable floating point unit
    ROM_FPULazyStackingEnable();        // Enable lazy stacking of FPU
    ROM_IntMasterEnable();              // Enable processor interrupts

    // Enable device clocking
    ROM_SysCtlClockSet(SYSCTL_SYSDIV_1 | SYSCTL_USE_OSC | SYSCTL_OSC_MAIN | SYSCTL_XTAL_16MHZ);

    // Enable peripherals
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_ADC0);     // ADC1
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_EEPROM0);  // EEPROM (2048 bytes in 32 blocks)
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOA);    // Pins: UART0 
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOB);    // Pins: UART1, GSM, Relays, I2C0SCL & SDA
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOC);    // Pins: Neopixel, keypad INT2
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOD);    // Pins: LCD screen
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOE);    // Pins: Relays
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_GPIOF);    // Pins: RGB LED, Relays
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_I2C0);     // I2C for MPR121 touchpad controller
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_SSI3);     // SSI3 for EA DOGS102W6 LCD display
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_TIMER0);   // Timer for keylock
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_TIMER1);   // Timer for keypad timeout
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_UART0);    // Console UART
    ROM_SysCtlPeripheralEnable(SYSCTL_PERIPH_UART1);    // GSM UART

    // Configure GPIO outputs
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTB_BASE, GPIO_PIN_5);     // Rel3N
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTB_BASE, GPIO_PIN_6);     // GSM PWRKEY
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTB_BASE, GPIO_PIN_7);     // GSM RESET
    //ROM_GPIOPinTypeGPIOOutput(GPIO_PORTC_BASE, GPIO_PIN_4);   // Neopixel
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_2);     // Rel3
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_3);     // Rel2
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_5);     // Rel2N
    ROM_GPIOPinTypeGPIOOutput(GPIO_PORTF_BASE, GPIO_PIN_1|GPIO_PIN_2|GPIO_PIN_3);   // RGB LED
    if (hwRev == 1) {
        // ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_1);  // Rel4
        // ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_4);  // Rel1N
        // ROM_GPIOPinTypeGPIOOutput(GPIO_PORTF_BASE, GPIO_PIN_1);  // Rel1  (conflict with red LED)
        // ROM_GPIOPinTypeGPIOOutput(GPIO_PORTF_BASE, GPIO_PIN_4);  // Rel4N (conflict with USR SW1)
    }
    else if (hwRev == 2) {
        ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_0);     // Rel4N
        ROM_GPIOPinTypeGPIOOutput(GPIO_PORTE_BASE, GPIO_PIN_4);     // Rel1N 

        // Disable NMI on PF0
        HWREG(GPIO_PORTF_BASE + GPIO_O_LOCK) = GPIO_LOCK_KEY;       // Unlock the port
        HWREG(GPIO_PORTF_BASE + GPIO_O_CR) |= GPIO_PIN_0;           // Unlock the pin
        HWREG(GPIO_PORTF_BASE + GPIO_O_AFSEL) &= ~GPIO_PIN_0;  
        HWREG(GPIO_PORTF_BASE + GPIO_O_DEN) |= GPIO_PIN_0;
        HWREG(GPIO_PORTF_BASE + GPIO_O_LOCK) = 0;                   // Lock the port
        ROM_GPIOPinTypeGPIOOutput(GPIO_PORTF_BASE, GPIO_PIN_0);     // Rel1 (conflict with USR SW2)

        ROM_GPIOPinTypeGPIOOutput(GPIO_PORTF_BASE, GPIO_PIN_4);     // Rel4 (conflict with USR SW1)
    }

    // Turn on an LED to show that we're working
    GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, BL_LED);

    // Start I2C module (for keypad)
    initI2C();

    // Start the MPR121 (keypad controller) and set thresholds (do this early 
    // since it takes a moment to calibrate)
    initMPR121();

    // Set up the timers used to lock/unlock the keypad
    ROM_TimerConfigure(TIMER0_BASE, TIMER_CFG_ONE_SHOT);
    ROM_TimerConfigure(TIMER1_BASE, TIMER_CFG_ONE_SHOT);
    ROM_TimerLoadSet(TIMER1_BASE, TIMER_A, ROM_SysCtlClockGet()* 15);

    // Setup the interrupts for the timer timeouts
    ROM_IntEnable(INT_TIMER0A);
    ROM_IntEnable(INT_TIMER1A);
    ROM_TimerIntEnable(TIMER0_BASE, TIMER_TIMA_TIMEOUT);
    ROM_TimerIntEnable(TIMER1_BASE, TIMER_TIMA_TIMEOUT);

    // Start SSI3 and the LCD display
    initSSI3();
    initLCD();

    // Console UART0: Set PA0 and PA1 as UART0, configure for 115200, 
    // 8-N-1 operation, enable interrupts
    ROM_GPIOPinConfigure(GPIO_PA0_U0RX);
    ROM_GPIOPinConfigure(GPIO_PA1_U0TX);
    ROM_GPIOPinTypeUART(GPIO_PORTA_BASE, GPIO_PIN_0 | GPIO_PIN_1);
    UART0StdioConfig(0, 115200, 16000000);
    ROM_IntEnable(INT_UART0);
    ROM_UARTIntEnable(UART0_BASE, UART_INT_RX | UART_INT_RT);  

    // Notify the user what's going on
    UART0printf("\n\n\n\r>>> INITIALIZING");

    // Get MCU ID
    FlashUserGet(&boardID1,&boardID2);

    // Print to LCD (along with header)
    LCDstring(0,0,"MCU/IMEI/ON@/NUM:",INVERSE);
    snprintf (aString[1],18,"%X-%X",boardID1,boardID2);
    LCDstring(1,0,aString[1],NORMAL);
    LCDstring(2,0,"...",NORMAL);

    // GSM UART1: Set PB0 and PB1 as UART1, configure for 115200, 
    // 8-N-1 operation, enable interrupts
    ROM_GPIOPinConfigure(GPIO_PB0_U1RX);
    ROM_GPIOPinConfigure(GPIO_PB1_U1TX);
    ROM_GPIOPinTypeUART(GPIO_PORTB_BASE, GPIO_PIN_0 | GPIO_PIN_1);
    UART1StdioConfig(1, 115200, 16000000);
    ROM_IntEnable(INT_UART1);
    ROM_UARTIntEnable(UART1_BASE, UART_INT_RX | UART_INT_RT);  

    /// GSM TEST AREA
    if (testGSM)
    {
        // See if the GSM module is on: try three times to power up
        GSMcheckPower(3);

        // Request and print the IMEI (s/n) of the SIM module (used as board s/n)
        GSMgetIMEI();
        LCDstring(2,0,IMEI,NORMAL);
        LCDstring(3,0,"...",NORMAL);

        // Request SIM card status
        GSMgetSIMstatus();

        // Get the time. Use a loop here since the time doesn't always come 
        // through on the first try after powering up. Make ctr1 attempts.
        ctr1 = 10;
        YY = 2000;
        while (YY == 2000 && ctr1 > 0)
        {
            // Turn off the LED to indicate what's going on
            GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, 0);

            // Check the time, decrement counter
            GSMcheckTime();
            ctr1--;

            // Wait a bit, turn the LED back on
            ROM_SysCtlDelay(ROM_SysCtlClockGet()/3);
            GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, BL_LED);
        }

        // Print the on-time
        UART0printf("\n\r> On-time from GSM: %s",fullOnTime);
        LCDstring(3,0,fullOnTime,NORMAL);
        LCDstring(4,0,"...",NORMAL);

        // If SIM card is present, get the phone number and balance
        if ( SIMpresent ) 
        { 
            // Get number
            GSMgetNum(); 

            // Get balance
            GSMcheckBalance();
        }

        // Print phone number / SIM status to LCD
        LCDstring(4,0,SIMID,NORMAL);
    }

    /// ADC TEST AREA - start the ADC
    if (testADC){ ADCinit(); }

    // Notify the user what testing functions are active
    UART0printf("\n\r> ----------Testing function status:----------");
    if (testGSM) { UART0printf("\n\r> ENABLED : GSM power at boot"); }
    else {UART0printf("\n\r> DISABLED: GSM power at boot");}
    if (testEEPROM) { UART0printf("\n\r> ENABLED : Store/retrieve ontime from EEPROM"); }
    else {UART0printf("\n\r> DISABLED: Store/retrieve ontime from EEPROM");}
    if (testDelete) { UART0printf("\n\r> ENABLED : Delete messages during processing"); }
    else {UART0printf("\n\r> DISABLED: Delete messages during processing");}
    if (testNotify) { UART0printf("\n\r> ENABLED : Message controller at boot"); }
    else {UART0printf("\n\r> DISABLED: Message controller at boot");}
    if (testADC) { UART0printf("\n\r> ENABLED : Test ADC"); }
    else {UART0printf("\n\r> DISABLED: Test ADC");}
    UART0printf("\n\r> --------------------------------------------");

    // Initialize the SysTick interrupt to process buttons
    ButtonsInit();
    SysTickPeriodSet(SysCtlClockGet() / APP_SYSTICKS_PER_SEC);
    SysTickEnable();
    SysTickIntEnable();

    // Notify the user about buttons
    UART0printf("\n\r> LEFT BUTTON:  Enter \"talk to GSM\" mode (blue LED). Updates signal strength.");
    UART0printf("\n\r> RIGHT BUTTON: Toggle power to GSM module (red LED).");

    /// EEPROM TEST AREA: Store on-time, retrieve last on-time. 
    // Don't run this each time 'cause EEPROM wears out.
    if (testEEPROM) 
    {
        EEPROMInit();

        struct E2S_TIME E2writeTime = {YY,MM,DD,hh,mm,ss,zz};

        //Read from struct at EEPROM start from 0x0000
        EEPROMRead((uint32_t *)&E2readTime, E2A_ONTIME, sizeof(E2readTime));
        UART0printf("\n\r> Last on-time: %u/%u/%u, %u:%u:%u, %d", E2readTime.E2YY, E2readTime.E2MM, E2readTime.E2DD, E2readTime.E2hh, E2readTime.E2mm, E2readTime.E2ss, E2readTime.E2zz, E2A_ONTIME);

        //Write struct to EEPROM start from 0x0000
        EEPROMProgram((uint32_t *)&E2writeTime, E2A_ONTIME, sizeof(E2writeTime));

        // Some EEPROM functions
        /*esize = EEPROMSizeGet(); // Get EEPROM Size 
        UART0printf("E2> EEPROM Size %d bytes\n", e2size);
        eblock = EEPROMBlockCountGet(); // Get EEPROM Block Count
        UART0printf("E2> EEPROM Blok Count: %d\n", e2block);*/

    }

    // Clear the LCD and set up for normal use:
    LCDclear(0,0,XMAX,YMAX);

    // Print relay status:
    if ( !testEEPROM ) { relaySet(0); }
    else 
    {
        // Read the last relay status from EEPROM
        EEPROMRead(&E2relayStatus, E2A_RELAY_STATUS, sizeof(E2relayStatus));

        // Notify user
        UART0printf("\n\r> Relay status from EEPROM (%X): ",E2relayStatus);
        for ( int r=0; r<4; r++ ) { UART0printf("%u",(E2relayStatus >> r) & 1); }

        // Get only the LSB of E2relayStatus
        E2relayStatus &= 0x000F;

        // Toggle each bit in relayStatus in order to properly update the display
        relayStatus = 15-E2relayStatus;
        UART0printf("\n\r> 15-E2relayStatus = %u",relayStatus);

        // Apply the previous relay states
        relaySet(E2relayStatus);
    }

    // Get the GSM signal strength and print to LCD (along with balance)
    if (testGSM) 
    { 
        GSMcheckSignal(); 

        // Print balance to LCD
        LCDstring(1,(18-sizeof(balance))*6,"$",NORMAL);
        LCDstring(1,(19-sizeof(balance))*6,balance,NORMAL);
    }

    /// CONTROLLER NOTIFY
    if (testNotify && SIMpresent){ 
        snprintf(aString[1],83,"MCU %X-%X IMEI %s OT %s BAL %s",boardID1,boardID2,IMEI,fullOnTime,balance);
        GSMsendSMS( ctrlID, aString[1] ); 
    }

    // Disable talk mode (was letting GSM notifs in during setup)
    talkMode = false;

    /// SETUP COMPLETE!
    UART0printf("\n\r> Setup complete! \n\r>>> RUNNING MAIN PROGRAM");
    GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, 0);

    // Lock keypad
    MPR121toggleLock();
    LCDstring(7,0,"SETUP COMPLETE!  ", NORMAL);

    /// MAIN LOOP - 
    // 1. Wait for new message notification and process. 
    // 2. Update ADC.
    while(1){
        // Process new messages.
        while (msgCount > 0)
        {
            // Start working on the oldest message
            msgOpen = msgCount;
            msgCount--;

            // Process message for envelope and content
            GSMprocessMessage(msgOpen);

            // If message content is good, act on message
            if (strstr(msgSender,ctrlID) != NULL && strlen(msgContent) == 4) {
                for ( ctr1=0;ctr1<4;ctr1++ ){
                    if ( msgContent[ctr1] == '1' ) { anInt |= 1 << ctr1; }
                    else if (msgContent[ctr1] == '0' ) { anInt &= ~(1 << ctr1); } 
                }
                relaySet(anInt);
            }

            // After the last new message, update the balance and EEPROM
            if ( msgCount == 0 ) 
            { 
                GSMcheckBalance();
                relayStatusE2();
            }
        }

        // Run the ADC
        if ( testADC && msgCount == 0 ) {
            // Trigger the ADC conversion.
            ADCProcessorTrigger(ADC0_BASE, 3);

            // Wait for conversion to be completed.
            while(!ADCIntStatus(ADC0_BASE, 3, false)){}

            // Clear the ADC interrupt flag.
            ADCIntClear(ADC0_BASE, 3);

            // Read ADC Value.
            ADCSequenceDataGet(ADC0_BASE, 3, pui32ADC0Value);

            // Convert to millivolts
            ui32D0v = pui32ADC0Value[0] * (3300.0/4095);

            // Convert to a string (in volts, three decimal places)
            snprintf (aString[1],7,"%d.%03dV", ui32D0v / 1000, ui32D0v % 1000);

            // Display the AIN0 (PE0) digital value on the console.
            LCDstring(2,11*6,aString[1],NORMAL);

            // Wait a bit
            ROM_SysCtlDelay(ROM_SysCtlClockGet()/4);
        }
    }
    //return(0);
}
\$\endgroup\$
5
\$\begingroup\$

This is particularly hard to read:

UART0printf("\n\r> ----------Testing function status:----------");
if (testGSM) { UART0printf("\n\r> ENABLED : GSM power at boot"); }
else {UART0printf("\n\r> DISABLED: GSM power at boot");}
if (testEEPROM) { UART0printf("\n\r> ENABLED : Store/retrieve ontime from EEPROM"); }
else {UART0printf("\n\r> DISABLED: Store/retrieve ontime from EEPROM");}
if (testDelete) { UART0printf("\n\r> ENABLED : Delete messages during processing"); }
else {UART0printf("\n\r> DISABLED: Delete messages during processing");}
if (testNotify) { UART0printf("\n\r> ENABLED : Message controller at boot"); }
else {UART0printf("\n\r> DISABLED: Message controller at boot");}
if (testADC) { UART0printf("\n\r> ENABLED : Test ADC"); }
else {UART0printf("\n\r> DISABLED: Test ADC");}
UART0printf("\n\r> --------------------------------------------");

I understand that you may be trying to have fewer lines of code, but it's usually better to have that than to have code that's just hard to read. You can at least make it more open by separating them into additional lines. This will also make it much easier to add or remove statements from an if or else.

UART0printf("\n\r> ----------Testing function status:----------");

if (testGSM)
{
    UART0printf("\n\r> ENABLED : GSM power at boot");
}
else
{
    UART0printf("\n\r> DISABLED: GSM power at boot");
}

if (testEEPROM)
{
    UART0printf("\n\r> ENABLED : Store/retrieve ontime from EEPROM");
}
else
{
    UART0printf("\n\r> DISABLED: Store/retrieve ontime from EEPROM");
}

if (testDelete)
{
    UART0printf("\n\r> ENABLED : Delete messages during processing");
}
else
{
    UART0printf("\n\r> DISABLED: Delete messages during processing");
}

if (testNotify)
{
    UART0printf("\n\r> ENABLED : Message controller at boot");
}
else
{
    UART0printf("\n\r> DISABLED: Message controller at boot");
}

if (testADC)
{
    UART0printf("\n\r> ENABLED : Test ADC");
}
else
{
    UART0printf("\n\r> DISABLED: Test ADC");
}

UART0printf("\n\r> --------------------------------------------");

Now, if you were to attempt to refactor this, you could consider having a display function that takes one of these variables and the corresponding message (excluding the same "ENABLED" or "DISABLED" labels) to display.

\$\endgroup\$
4
\$\begingroup\$
  • Try to avoid globals. For example, it is very hard to say where YY and fullOnTime are coming from (apparently, GSMcheckTime() affects them).

    It is absolutely unclear what the fullOnTime is shall GSMcheckTime() to fail 10 times.

  • Keep the related logic together. For example, print the testEEPROM message when you are actually start testing it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for these tips, I will make some changes. I had forgotten to write an error handler for GSMcheckTime(). How do you recommend handling the variables this function populates without using globals? I guess I could write something that converts time to number of seconds since 1990 or something along those lines, but that seems clunky. \$\endgroup\$ – LShaver Dec 4 '15 at 17:59
0
\$\begingroup\$

In addition to the comments already made regarding code formatting and the avoidance of global variables, (hint: if there must be globals organize them into structures) here are a few more things that may help you improve your code.

Move long sequences to separate functions

I see that there are already some functions such as ADCinit and ButtonsInit which, I presume, handle setting up those subsystems. I'd suggest reducing the main code to something like this:

int main(void) {
    unsigned msgCount;
    initSystem(&msgCount);
    while(1) {
        while (msgCount) {
            processMessage();
        }
    }
}

That's a relatively extreme makeover, but what it does is very clearly show the overall structure of the program and that what is happening is first an initialization and then an infinite loop processing messages. The complexity of each of those can then be moved to separate functions with clearly delineated assumptions and responsibilities.

Think carefully about when variables change

The outline of the existing infinite loop in main looks like this:

while(1){
    // Process new messages.
    while (msgCount > 0)
    {
        // stuff
    }
    // Run the ADC
    if ( testADC && msgCount == 0 ) {
        // more stuff
    }
}

It doesn't appear to me that the testADC flag can actually change during the course of the loop, and by definition of the previous loop, msgCount was just checked. For those reason, I suspect that the second loop can either be made unconditional or omitted entirely. It's also already been mentioned, but consistent application of code style (e.g. placement of braces) will make your code easier to read and maintain.

Name similar things similarly

We have initI2C and initMPR121, but also ADCinit, EEPROMInit and ButtonsInit. I'd suggest establishing some convention and then using it consistently. I've used a few different conventions for embedded systems in the past, but the one I find that seems to work best for me is to do something like a C version of object-oriented code. That is, I'd have all of the functions for a particular subsystem prefixed with the name of the subsystem. It most resembles what you mostly already have for the EEPROM. I'd name the functions like this:

EEPROM_init();
EEPROM_read();
EEPROM_write();

Getting these interfaces both sufficient and minimal is one of the hardest but also most important task with embedded system programming.

Use abstractions to convey intent

When I read a line of code like this:

GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, 0);

I don't instantly understand the intent of the code. In this case, there's an associated comment which says:

// Turn off the LED to indicate what's going on

I'd be inclined to eliminate the need for the comment by writing it in code instead:

LED_off();

The advantage is that now the code itself is telling you what's going on so there is no longer a need for the comment, nor any possibility that the comment and code will say different things. It's all too easy over time to wind up with code like this:

// Turn off the LED to indicate what's going on
GPIOPinWrite(GPIO_PORTF_BASE, BL_LED, BL_LED);

In this case, (not from your code -- just an example!) the comment is wrong -- the code actually turns the LED on, not off.

Organize related variables in structures

In this code, there are a number of calls to things related to GSM. For example, we have GSMgetNum() and GSMcheckBalance(). I don't much like either of those because their names are somewhat misleading. I would expect a function named GSMgetNum to return a number, but it apparently returns nothing, instead putting the number somewhere ... but where? Also GSMcheckBalance() is not a very good name either, because I suspect it doesn't just "check" but actually gets the balance. There are a few approaches possible, but I'd suggest creating a struct to hold such values and then passing a pointer to the struct to the various routines. For example:

typedef struct GSM_s {
    char IMEI[16];
    char SIMID[16];
} GSM;

// fetch IMEI and store in passed structure
void GSM_getIMEI(GSM &gsm);

int main() {
    GSM gsm;
    // stuff
    GSM_getIMEI(&gsm);
}

Isolate code in modules

If you wanted to, say, change from an IIC LCD module to a SPI-based LCD, it appears that the code is already well suited to accomodate such a change. The main routine only appears to use initLCD and LCDstring which both seem to do a pretty good job of hiding the implementation details. (I'm guessing that the first two arguments to LCDstring are row and column?) However other components don't appear to be as nicely isolated. The ADC in particular does not appear to have a nice interface for your application. I'd suggest writing one, perhaps using the language of the problem domain as in getVoltage() rather than in the language of the problem solution (your particular hardware) such as ADCSequenceDataGet().

Do NOT reuse variables

The current code uses ctrl (which is an awfully nondescript name) for multiple purposes. In one case, it's the number of times the GSM test code will attempt. In another, it appears to be an index variable for stepping through a string. I'm betting that the intent was to minimize the use of memory or registers, which is a worthy goal, but in fact, the compiler will do a much better job of that than you will if you don't do things that way. For example, look at this code:

for ( ctr1=0;ctr1<4;ctr1++ ){
        if ( msgContent[ctr1] == '1' ) { anInt |= 1 << ctr1; }
        else if (msgContent[ctr1] == '0' ) { anInt &= ~(1 << ctr1); } 
}

Because ctrl is a variable with the scope of all of main, the compiler (and the human reader of the code!) will both have to figure out whether this value is used somewhere else later on. It is not, so the compiler can safely use that register for some other purpose after the code. Better would be to make that explicit to both the human reader of the code and the compiler:

for (int i=0; i < 4 ; ++i) {
        if ( msgContent[i] == '1' ) { 
            anInt |= 1 << i; 
        }
        else if (msgContent[i] == '0' ) { 
            anInt &= ~(1 << i); 
        } 
}

Or better still, because both anInt and msgContent appear to also have similar scope in practice:

int relayMask;
for (relayMask=0; *msgContent; ++msgContent) {
    relayMask <<= 1;
    relayMask |= (*msgContent == '1') ? 1 : 0;
}

With that, it's immediately clear to both compiler and human readers. Note too that unlike the original code, this code explicitly sets or clears all bits no matter what the input might have been.

\$\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.