Wrong sensor readings (Byte reversal)

[SimGas]

Description:

Hi, I am using the XENSIV PAS sensor together with a PSoC 4 device. Communication works fine so far. However, I discovered a bug in the xensiv_pasco2_get_result function. int32_t xensiv_pasco2_get_result(const xensiv_pasco2_t * dev, uint16_t * val) The function should store the CO2 concentration in the variable val. If you read out val while the function is running (e.g. by an interrupt-based event) you can get a wrong reading which is just the two bytes containing the concentration switched. This is because the xensiv_pasco2_get_result_function uses the output variable as a storage for the raw sensor reading as well.

Steps to Reproduce: Example: Assume CO2 Concentration is 835 ppm. If a new data result is available in the sensor, this will be read out as LSB, MSB: 0x43, 0x03. Within the get_result function, those two bytes get reversed (htons-function) so that they become 0x03, 0x43 => 0x0343 = 835 ppm

if you read out the content of the output variable "val" between the I2C-transmission and the byte reversal, you end up with: 0x4303 = 17155 ppm

Expected Result: val should always contain a correct ppm reading, independently when you read it

Actual Result: if you read out val while getting a new result, you get a wrong reading.

An easy fix would be to introduce another variable, which stores the data read from the register before it gets reversed.:

`int32_t xensiv_pasco2_get_result(const xensiv_pasco2_t dev, uint16_t val) { xensiv_pasco2_plat_assert(dev != NULL); xensiv_pasco2_plat_assert(val != NULL);

xensiv_pasco2_meas_status_t meas_status;
int32_t res = xensiv_pasco2_get_measurement_status(dev, &meas_status);
uint16_t data_buffer=0;

if (XENSIV_PASCO2_OK == res)
{
    if (meas_status.b.drdy != 0U)    //if new measurement status register Data ready bit is not cleared (if it is set)
    {
        res = xensiv_pasco2_get_reg(dev, (uint8_t)XENSIV_PASCO2_REG_CO2PPM_H,(uint8_t *) &data_buffer, 2U);

        *val = xensiv_pasco2_plat_htons(data_buffer); 
    }
    else    //no new measurement
    {
        res =  XENSIV_PASCO2_READ_NRDY;
    }
}

return res;

}`

Build/Commit: Please specify the release, like 1.4.1. If there is no release, give us the commit for the code you used.

Target: PSoC 4000S series: CY8C4025LQI-S411

Host OS and Version: Windows

Compiler: PSoC Creator IDE

[jaenrig-ifx]

Hi @SimGas,

It seems that the function adapting the endianness from the sensor register to the MCU one might not be properly implemented -> https://github.com/Infineon/sensor-xensiv-pasco2/blob/master/xensiv_pasco2.c#L365

Depending on your microcontroller endianness, you need to swap the bytes read from the sensor, or not. Its default implementation is here -> https://github.com/Infineon/sensor-xensiv-pasco2/blob/d83763095dbc6dfab5ab59e4fbd771c591090288/xensiv_pasco2_mtb.c#L232-L235

[SimGas]

Hi Jaenrig,

Thanks for your reply. If I read out the sensor result AFTER getting the result, everything is fine and the bytes are in correct order. However, if you have a interrupt-based function which reads out the content of "val" between res = xensiv_pasco2_get_reg(dev, (uint8_t)XENSIV_PASCO2_REG_CO2PPM_H,(uint8_t *) val, 2U); and *val = xensiv_pasco2_plat_htons(*val);

then you get an incorrect reading of "val". As soon as the second line is finished, "val" contains a correct reading again.

Thanks Simon

[jaenrig-ifx]

I am not fully following... How is the interrupt-based function implemented? If you directly read from the register xensiv_pasco2_get_reg(), then later you have to covert the register value to the right endianness with xensiv_pasco2_plat_htons().

[SimGas]

Let's assume I have a program which reads out two sensors via I2C (e.g. DPS368 and XENSIV PAS) and writes the sensor readings to a UART interface. The write-to-uart function gets called interrupt based every 100 ms (sure enough, the PAS will only update every 10 seconds, but the DPS is much faster).

In the main-loop everything the program does is reading out the sensors one after each other seconds.

int main(void)
{
//Configure all sensors and interfaces (not included here)
for(;;)
 {
dps_status = dps310_read_background(&dps_temp,&dps_press);   
pas_status = xensiv_pasco2_get_result(pas_sensor, &pas_co2_ppm);
}
}

dps_temp, dps_press and pas_co2_ppm should always contain correct sensor readings.

However, if the interrupt-triggered write-to-uart function is triggered while executing xensiv_pasco2_get_results inbetween the two lines mentioned above (between get_reg and plat_htons), then there is a wrong reading in pas_co2_ppm.

Can you follow me now?

Best, Simon

[jaenrig-ifx]

Understood now. And I understand now your solution, but it seems more of a concurrency problem due to the library not implementing those 2 steps( read_reg + reverse) atomically, before allowing any reading access (as it can happens by interrupts).

What about disabling interrupts before calling xensiv_pas_co2_get_result()? Actually, why not to try to avoid that both sensor reading function are interrupted by the UART writing? is that UART writing priority over reading the data(i.e. I have to send something every exactly 100 ms ? A longer sending write would work in your system? is the UART write only relevant when the sensor data is properly updated? or there are other constrains?

BR, Juan

[SimGas]

Well, as I now use the solution proposed above, this topic is not really an issue for me anymore. I just wanted to share my findings here. You are right, sending data has not that much of a priority and timing does not need to be 100% correct. However, disabling of all interrupts is not really a solution as I2C communication also depends on interrupts (at least in PSoC 4). Why do you think that the way using an intermediate buffer (data_buffer in my case) not acceptable? From my point of view, this is the cleanest implementation possible, which requires barely any additional memory.

This method of using an intermediate buffer for raw values (in case of the PAS the reversed bytes) is also used in other infineon libraries such as dps3xx pressure sensors: https://github.com/Infineon/sensor-xensiv-dps3xx/blob/bdf5a3c8be3d29ff0b082c6ed119f7df095b26b8/xensiv_dps3xx.c#L729-L743

//--------------------------------------------------------------------------------------------------
// xensiv_dps3xx_read
//--------------------------------------------------------------------------------------------------
cy_rslt_t xensiv_dps3xx_read(xensiv_dps3xx_t* dev, float* pressure, float* temperature)
{
    uint8_t read_buffer[XENSIV_DPS3XX_PSR_TMP_READ_LEN];

    cy_rslt_t rc = _xensiv_dps3xx_read_raw_values(dev, read_buffer);
    if (CY_RSLT_SUCCESS == rc)
    {
        _xensiv_dps3xx_convert_raw_values(dev, read_buffer, pressure, temperature);
    }

    return rc;
}

[jaenrig-ifx]

Yes, I agree. Only saying that an overall evaluation of the library regarding concurrency (interrupts, multi-threading environments, ...) should be considered not only for this particular function, but for the complete library.