HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Microgrid device driver controlling GSM modem, LCD, keypad, and relays

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lcdkeypadgsmmodemdevicemicrogridcontrollingdriverandrelays

Problem

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,

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#112472, answer score: 5

Revisions (0)

No revisions yet.