Is my design for data reading over an I2C bus and writing back to UART good enough?

Question

So I implemented drivers for I2C and USART using interrupts with some guidelines online, and was wondering if I can get some suggestions from a design point of view even though the code works (tried at 9600 and 115200 baud rate), but I do get a hardfault upon using two different baud rates at RX/TX. One reason could be I'm using \r as an indication to disable the interrupts, and in case of different baud rates, it might not even disable the interrupts since the received byte is different than what was sent. So I'm not sure if I should be concerned with it.

The program:

• runs a loop where it listens for the bytes over UART after the control bits are enabled
• triggers ISR for each byte received while storing it into a respective linear buffer, until \r is received, indicating the end of message
• disables the control bits so it's no longer acting on any new bytes
• parses the data in the linear buffer till \r, and does some stuff based on what we receive. One of the things that the program does is read the value off the temperature sensor over I2C and serial it out!

My thoughts:

• I am not sure if I'm making the right usage of interrupts (or maybe I am) cause what I do is enable the peripheral control bits, and then sort of wait for the ISR to be fired (that's application-specific I guess), while storing each byte into the buffer till we get \r.
• I am using a linear buffer instead of a preferred circular buffer cause I thought it wouldn't make much of a difference for this application. I am sort of using it a circular buffer (maybe I'm wrong) by restarting the index for storing data into RX buffer to 0; so every time there's a new data, it gets appended from the start. In case of a circular buffer, I'd continue storing data continuously and it would eventually wrap around overriding the old data which has already been parsed by then.
• To make this application more generic, I might need to remove the device address member from the struct and instead pass it to respective I2C HAL functions.

I have included the relevant parts of the code. Feel free to leave a comment if there's any confusion.

hal_i2c.h

typedef struct {
    uint32_t I2C_SCLSpeed;
    uint8_t I2C_DeviceAddress;
    uint8_t I2C_AckControl;
    uint16_t I2C_FMDutyCycle;
} I2C_Config_t;

hal_i2c.c

I2C_State HAL_I2C_StartInterrupt(I2C_State expectedState, uint8_t txSize, uint8_t rxSize)
{
    if (I2C_handle_p->I2C_State == I2C_INIT)
    {
        // set transaction state
        I2C_handle_p->I2C_State = expectedState;

        // set respective buffer sizes
        I2C_handle_p->txBufferLength = txSize;
        I2C_handle_p->rxBufferLength = rxSize;

        // generate start condition
        I2C_GenerateStartCondition(I2C_handle_p);

        // enable i2c control bits
        I2C_SetCtrlBits();
    }
    return I2C_handle_p->I2C_State;
}


void I2C1_EV_IRQHandler (void)
{
    uint8_t eventInterrupt = (I2C_handle_p->pI2Cx->CR2 & I2C_CR2_ITEVTEN) >> I2C_CR2_ITEVTEN_Pos;
    uint8_t bufferInterrupt = (I2C_handle_p->pI2Cx->CR2 & I2C_CR2_ITBUFEN) >> I2C_CR2_ITBUFEN_Pos;
    uint8_t temp;           // stores register values

    if (eventInterrupt)
    {
        //  validate the completion of START condition
        temp =  (I2C_handle_p->pI2Cx->SR1 & I2C_SR1_SB) >> I2C_SR1_SB_Pos;
        if (temp)
        {
            if (I2C_handle_p->I2C_State == I2C_TX_BUSY)
            {
                I2C_WriteSlaveAddress(I2C_handle_p, WRITE);     // write slave address along with write bit
            }
            else if (I2C_handle_p->I2C_State == I2C_RX_BUSY)
            {
                I2C_WriteSlaveAddress(I2C_handle_p, READ);      // write slave address along with read bit
            }
        }

        // ADDR
        temp = (I2C_handle_p->pI2Cx->SR1 & I2C_SR1_ADDR) >> I2C_SR1_ADDR_Pos;
        if (temp)
        {
            I2C_ClearADDRFlag(I2C_handle_p->pI2Cx);             // clear address flag
        }

        // TXE, RXNE
        if (bufferInterrupt)
        {
            // TXing
            temp = (I2C_handle_p->pI2Cx->SR1 & I2C_SR1_TXE) >> I2C_SR1_TXE_Pos;

            if (temp && I2C_handle_p->I2C_State == I2C_TX_BUSY)
            {
                I2C_TXE_Interrupt();
            }

            // RXing
            temp = (I2C_handle_p->pI2Cx->SR1 & I2C_SR1_RXNE) >> I2C_SR1_RXNE_Pos;

        }

        //BTF
        temp = (I2C_handle_p->pI2Cx->SR1 & I2C_SR1_BTF) >> I2C_SR1_BTF_Pos;
        if (temp)
        {
            if (I2C_handle_p->I2C_State == I2C_TX_BUSY)                 // TXE=1, BTF=1
            {
                if (!I2C_handle_p->txBufferLength)                      // if there are no more TX bytes to be sent
                {
                    I2C_GenerateStopCondition(I2C_handle_p);
                    I2C_StopTransmission();
                }
            }
            else if (I2C_handle_p->I2C_State == I2C_RX_BUSY)            // RXNE=1, BTF=1, LEN=0 --> STOP
            {
                if (I2C_handle_p->rxBufferLength == 2)
                {
                    I2C_GenerateStopCondition(I2C_handle_p);

                    I2C_handle_p->pRxBuffer[I2C_handle_p->rxStartIndex++] = (uint8_t) I2C_handle_p->pI2Cx->DR; // read second last byte
                    I2C_handle_p->rxBufferLength--;

                    I2C_handle_p->pRxBuffer[I2C_handle_p->rxStartIndex++] = (uint8_t) I2C_handle_p->pI2Cx->DR; // read last byte
                    I2C_handle_p->rxBufferLength--;

                    I2C_StopTransmission();
                }
            }
        }
    }
}


void I2C_TXE_Interrupt (void)
{
    if (I2C_handle_p->txBufferLength)
    {
        I2C_handle_p->pI2Cx->DR = (*I2C_handle_p->txBuffer)++;
        I2C_handle_p->txBufferLength--;
    }
}

static void I2C_StopTransmission(void)
{
    // disable control bits
    I2C_handle_p->pI2Cx->CR2 &= ~(1 << I2C_CR2_ITEVTEN_Pos);
    I2C_handle_p->pI2Cx->CR2 &= ~(1 << I2C_CR2_ITBUFEN_Pos);

    // restore struct
    I2C_handle_p->I2C_State = I2C_READY;

    I2C_handle_p->rxStartIndex = 0;
}

usart_app.h

typedef struct {
    USART_TypeDef *pUSARTx;
    USART_Config_t USART_Config;
    USART_State USART_State;
    char *txBuffer;
    char *rxBuffer;
    uint8_t txLength;
    uint8_t rxLength;
    uint8_t rxSize;
    uint8_t dmaTransfer;
    uint8_t dmaReception;
    DMA_Handle_t *dmaRx;
    DMA_Handle_t *dmaTx;
} USART_Handle_t;

usart_app.c

void StartSerial (USART_Handle_t *usart, char *usart_rxBuffer, uint8_t rxBufferSize, I2C_Handle_t *I2C_Handle)
{
    char tempBuffer[rxBufferSize];
    memset(tempBuffer, 0, rxBufferSize);
    while(true)
    {
        ReceiveSerialData(usart);
        ParseSerialData(usart, tempBuffer, usart_rxBuffer);
        bool status = ExecuteSerialData(usart, tempBuffer, I2C_Handle);
        if (!status)        // break if "q" is entered
        {
            break;
        }

        // clear out the buffers -- probably don't need it!
        usart->rxBuffer = usart_rxBuffer;
        memset(usart_rxBuffer, 0, sizeof(rxBufferSize));
        memset(tempBuffer, 0, sizeof(tempBuffer));

        // reset the USART state
        usart->USART_State = USART_INIT;
    }
}

void ReceiveSerialData(USART_Handle_t *usart)
{
    while (USART_RxData(USART_RX_BUSY) != USART_READY);
}

void ParseSerialData(USART_Handle_t *usart, char *tempBuffer, char *rxBuffer) 
{
    char *start = rxBuffer;
    char *end = strstr(rxBuffer, "\r");
    uint8_t bytes = end - start;
    memcpy(tempBuffer, start, bytes);
}

bool ExecuteSerialData(USART_Handle_t *usart, const char *str1, I2C_Handle_t *I2C_Handle)
{
    if (!strcmp(str1, "temp"))
    {
        uint16_t temp = GetTemperature(I2C_Handle);
        SendSerialData(usart, "Current temperature: %d\n", temp);
    }
    else if (!strcmp(str1, "q"))
    {
        SendSerialData(usart, "Ending serial\n");
        return false;
    }
    return true;
}

main.c

void I2C_Initilization(I2C_Config_t *I2C_Config, I2C_TypeDef *i2cPeripheral)
{
    I2C1_handle.pI2Cx = i2cPeripheral;
    I2C1_handle.I2C_Config = *I2C_Config;
    I2C_Init(&I2C1_handle);
}

void USART_Init (void)
{
    USART2_handle.pUSARTx = USART2;
    USART2_handle.USART_Config.USART_baudRate = USART_BAUD_9600;
    USART2_handle.USART_Config.USART_mode = USART_MODE_TXRX;
    USART2_handle.USART_Config.USART_parityControl = USART_PARITY_DISABLED;
    USART2_handle.USART_Config.USART_stopBits = USART_STOP;
    USART2_handle.USART_Config.USART_wordLength = USART_8_DATA_BITS;
    USART2_handle.rxBuffer = usart_rxBuffer;
    USART2_handle.rxLength = rxLength;
    USART2_handle.rxSize = rxLength;
    USART2_handle.dmaTransfer = DMA_TX_DISABLE;
    USART2_handle.dmaReception = DMA_RX_DISABLE;

    USART_Initization(&USART2_handle);
}

int main(void)
{
    HAL_Init();

    /* Configure the system clock */
    SystemClock_Config();

    /* Initialize all configured peripherals */
    MX_GPIO_Init();

    /* Initialize I2C config struct */
    I2C_Config_t i2c_config = {
                I2C_AckControl: I2C_ACK_ENABLE,
                I2C_SCLSpeed: I2C_SCL_SPEED_SM,
                I2C_DeviceAddress: MCP9808_ADDRESS,
                I2C_FMDutyCycle: I2C_FM_DUTY_2
    };
    I2C_Initilization(&i2c_config, I2C1);

    /* Initialize USART struct */
    USART_Init();

    StartSerial (&USART2_handle, usart_rxBuffer, usart_rxLength, &I2C1_handle);

    while (1);
}

mcp9808.c

// static variables
static uint8_t txBuffer[1] = {MCP9808_REG_AMBIENT_TEMP_REG}; 
static uint8_t rxBuffer[BYTES_TO_READ];
static uint8_t txSize = sizeof(txBuffer)/sizeof(txBuffer[0]);
static uint8_t rxSize = BYTES_PER_TRANSACTION;

uint16_t GetTemperature(I2C_Handle_t *I2C_Handle)
{
    uint16_t temperature;

    temperature = ReadTemperature(I2C_Handle);
    return temperature;
}

uint16_t ReadTemperature(I2C_Handle_t *I2C_handle)
{
    I2C_handle->txBuffer = txBuffer;
    I2C_handle->pRxBuffer = rxBuffer;
    I2C_handle->rxBufferSize = rxSize;

    // Start I2C transaction
    while (HAL_I2C_StartInterrupt(I2C_TX_BUSY, txSize, rxSize) != I2C_READY);

    I2C_handle->I2C_State = I2C_INIT;

    // read the data from the sensor
    for (int i = 0; i < I2C_handle->rxBufferSize/2; i++)
    {
        I2C_handle->I2C_State = I2C_INIT;
        while (HAL_I2C_StartInterrupt(I2C_RX_BUSY, txSize, rxSize) != I2C_READY);
    }

    uint16_t temperature = ProcessData(I2C_handle->pRxBuffer);
    return temperature;
}

Answer 1

Big picture/design

If you have the option to use DMA, then go with that. DMA can be somewhat complex in its own, but it doesn't screw up all the real-time requirements of the whole program, in the way that asynchronous receiver interrupts do.

That being said, storing incoming Rx data from a UART in a (ring) buffer is the old school way of doing things. It should work out fine unless your program has a lot of real-time deadlines.

Interrupts

The all-time most common bug in embedded systems is failing to protect data shared with interrupts from race conditions, so it is not at all surprising if this is the cause of the bug you describe.

It is not entirely clear how the interrupts handle re-entrancy with the main application, as the magic I2C_handle_p struct definition is absent. I don't understand what you mean to do with \r, there is no code posted that disables the interrupts based on that.

You need some manner of semaphores from protecting the caller from reading part of the data, then get interrupted in the middle of it. I like to provide these as a feature in the ring buffer ADT itself, making that one intrinsically interrupt safe.

Alternatively, you could temporary disable interrupts in the caller while you grab the data, but that only works if the caller can do this in less time than it takes for the serial bus to send another byte.

Usually this is done by providing double-buffering (no matter if you have a ring buffer or a linear one). You have one software buffer where the incoming data is getting written, and another buffer which contains the latest completely received data. When the ISR is done receiving, it only swaps the pointers between these two buffers.

So if you have a memcpy somewhere doing a hard copy of the whole buffer, you are doing it wrong. This is yet another very common problem in defective ISR code. Similarly, there should be no need to memset everything to zero repeatedly, that's just wasting time for nothing.

And finally, all variables shared with the ISR must be declared volatile. That's another common bug - read this: Using volatile in embedded C development.

Other issues/best practices

• What about framing/overrun errors and similar? What do you do when such errors occur? Your program should handle them and discard the data when they strike. Also, I don't see any checksum or CRC. UART in particular is very unreliable.

• Never bit shift on signed or negative types. This means, never write 1 << .. because the integer constant 1 is of type signed int. Use 1u suffix and in case of variables, make sure to cast to a large unsigned type before shifting.

• ~ is notorious for changing signedness of its operand and thereby causing all manner of implicit integer promotion bugs. It's a good habit of casting its operand to a large unsigned type before applying ~. Be aware of the Implicit type promotion rules, they are especially known to cause havoc on small 8- or 16-bit microcontroller systems.

• Never use char for storing raw data, even if you expect incoming data from UART to be text. It comes with implementation-defined signedness (Is char signed or unsigned by default?), and embedded compilers in particular are known to implement char differently from case to case. Read everything as uint8_t and then when everything is verified and you know that the input is valid text, cast to char if you must.

• Avoid variadic functions. These are known to have non-existent safety and are needlessly slow. They might seem convenient to the programmer, but they are not convenient to the program, as they make things slower and buggier in general. There should be no need to use variadic functions in an embedded system.

• It's bad practice to write empty while loops like while (something);, because to the reader it is completely unclear if the semi-colon is intentional or just a slip of the finger. Therefore, always use one of these forms instead:

  while (something)
    ;
  

  or

  while(something)
  {}
  

• uint8_t bytes = end - start; is rather questionable, you need to guarantee that this won't be larger than 255 bytes.

  Also note that upon pointer subtraction, you are actually getting back an obscure large integer type called ptrdiff_t which does you no good. I'd recommend to do (uint8_t)end - (uint8_t)start instead.

• Never use int anywhere in an embedded system. You should be using the types from stdint.h, or size_t in case you are declaring a for loop iterator.

• static uint8_t txSize = sizeof(txBuffer)/sizeof(txBuffer[0]);. This should either have been a macro or a const, instead of a read/write variable.

• The format of main() in an embedded bare metal system is always void main(void), unless your compiler requires some other exotic form. Who are you going to return to? With gcc-like compilers, you need to compile embedded systems with the -ffreestanding option.

• All your files are missing #include so it isn't clear if you are even including the correct libraries or otherwise have strange file dependencies.

• Where is the watchdog code? Microcontroller firmware which is not utilizing a watchdog is defective, period. You may disable it in debug release, but where to place it and where to feed it needs to be considered early on, and the code must be present.

  Ideally you only feed it at one single point of your program, on top of the internal loop in main().

---

Overall, a lot of these common issues/dormant bugs could have been avoided if you used MISRA-C. I'd strongly recommend to at least read it as study material, even if you don't want to go all the way and get formal compliance.