2
\$\begingroup\$

So I have been wondering about the standard way of structuring your files/code when it comes to reading data from the driver file. So what I now have is a buffer that's supposed to store the data that we, as user, requested from the driver file. Since we can't pass arrays to the function (decays to a pointer), what I do is have a struct member variable pRxBuff point to rxBuffer array which is located in main instead of defining inside a function because once the function returns, the array is no longer valid, and inside the Read() function, I populate rxBuffer by dereferencing the data at specific index.


typedef struct {
    I2C_TypeDef *pI2Cx;
    I2C_Config_t I2C_Config;
    I2C_State I2C_State;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;


void ProcessData (uint8_t *rxBuffer) {
    uint8_t startIndex = 0;
    uint16_t temp;

    // process data
    uint8_t upperByte = rxBuffer[startIndex] & 0x1F; // mask out the 3 bits
    uint8_t signBit = upperByte & 0x10;

    if (signBit)
    {
        upperByte = upperByte & 0xF;    // clear out the sign bit
        temp = 256 - (upperByte << 4 | rxBuffer[startIndex+1] >> 4);
    }
    else
    {
        temp = upperByte << 4 | rxBuffer[startIndex+1] >> 4;
    }
}

// sensor.c
void ReadData(I2C_Handle_t *I2C_handle)
{
    // start I2C transaction
    while (HAL_I2C_StartInterrupt(I2C_TX_BUSY) != 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) != I2C_READY);
    }

    // at this point, I have `rxBuffer` populated with raw data
    // now I need to convert this raw data into human-readable

    for (int i = 0; i < I2C_handle->rxBufferSize; i+=2)
    {
        ProcessData(I2C_handle->pRxBuffer, i); // currently not storing processed data anywhere
    }
}

// main.c
const int bytesToRead = 6;  
static uint8_t rxBuffer[bytesToRead];
I2C_Handle_t i2c;

void I2C_Initilization()
{
    i2c.pI2Cx = I2C1;
    i2c.I2C_Config.I2C_AckControl = I2C_ACK_ENABLE;
    i2c.I2C_Config.I2C_SCLSpeed = I2C_SCL_SPEED_SM;
    i2c.I2C_Config.I2C_DeviceAddress = MCP9808_ADDR;
    i2c.I2C_Config.I2C_FMDutyCycle = I2C_FM_DUTY_2;
    I2C_Init(&i2c);
}

uint16_t read_temp(uint8_t interrupt)
{
    uint16_t temperature;

    i2c.txBuffer = txBuffer;
    i2c.txBufferLength = txSize;
    i2c.pRxBuffer = rxBuffer;
    i2c.rxStartIndex = 0;
    i2c.rxBufferLength = BYTES_PER_TRANSACTION;
    i2c.rxBufferSize = bytesToRead;

    if (interrupt == SET)
    {
        temperature = read_temp_interrupt(&i2c);
    }
    else
    {
        read_temp_polling(&i2c, bytesToRead);
    }
    return temperature;
}

int main(void) {
    I2C_Initilization();
    read_temp(SET);
}

Issues with this:

  • though I have been able to populate the rxBuffer which I can access in the main, is this still the right way to do it?
  • what if rxBuffer is of different size than what the processed data would need. For e.g: 2 raw bytes represent one processed decimal value. How do I avoid creating two different buffers each for storing raw and processed data?
  • With this approach, I had to create a separate member variable rxStartIndex to keep track of the index where the data is to be written.
\$\endgroup\$
  • 3
    \$\begingroup\$ CodeReview does not include questions of the form "Is there any way?", but rather "Is this the best way?" In other words, you need to come to us with working code. \$\endgroup\$ – Reinderien Apr 26 at 17:50
  • 3
    \$\begingroup\$ the code works. maybe I should rephrase the question to "is this the best way" \$\endgroup\$ – xyf Apr 26 at 17:55
  • 2
    \$\begingroup\$ To all who are in the close vote queue. The question meets the guidelines now. \$\endgroup\$ – pacmaninbw Apr 26 at 22:20
  • \$\begingroup\$ reading data from the driver[/application] file starts in the middle of uncharted area: Please provide enough information about the execution environment of the code presented. The current state has me wondering about what "the device" shall be good for instead of thinking about the code. (I can take hints like rx, tx, I2C, sensor, temp: I rather wouldn't guess.) \$\endgroup\$ – greybeard Apr 28 at 4:45
1
\$\begingroup\$

I will give you some tips about defining structs that later are going to be part of a file, of course you can do in other way but this is the way I follow in general. I understand that the following struct is the header of the file.

typedef struct {
    I2C_TypeDef *pI2Cx;
    I2C_Config_t I2C_Config;
    I2C_State I2C_State;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;

In general header files contains some bytes for identification(check libmagic library), the second tip is to have all the types group to avoid misalignment of data. So your struct will be

typedef struct {
    uint32_t magic; // The magic value
    I2C_Config_t I2C_Config;
    I2C_State I2C_State; 
    I2C_TypeDef *pI2Cx;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;

Also you can use the attribute packed of the compiler if needed alignment of the data struct.

| improve this answer | |
\$\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.