Transmit success flag to be able to know when something fails and be able to flag bad vs good values

[jerabaul29]

I am working on an application where it is critical to be sure that the data received are valid / correct, otherwise some real world hardware will do bad stuff. If I do not miss something, I think that checking data validity is not really doable / enforced at the moment with this library, but that additional safety could be added relatively easily. I discuss this a bit in details here:

Example of how things can go wrong

My scenario is the following; at present:

• the sensors starts as it should and is used for a long time...
• ... then something bad happens: hardware failure, a broken wire on a drone, etc, and the sensor gets disconnected / stops working. The problem is, the library will just continue providing (bad, invalid) data, without a way for the user to know that these data are bad
• then bad data is fed into another critical system, and there is no way for this system to know that the data are bad, which is even worse (otherwise, the system could at least try to use another sensor, find a correction solution, restart everything, etc).

For example, at present, if I start using one of these IMUs on I2C, and it starts well, then if I just unplug the sensor, I start getting erroneous values, without any way to check if these are actually valid or invalid values.

Where the issue comes from

I will follow the stream of what my code uses. I rely on this function to read from my IMU:

Adafruit_LSM6DS/Adafruit_LSM6DS.cpp

Lines 251 to 261 in 1ea3bd2

	bool Adafruit_LSM6DS::getEvent(sensors_event_t *accel, sensors_event_t *gyro,
	sensors_event_t *temp) {
	uint32_t t = millis();
	_read();
	// use helpers to fill in the events
	fillAccelEvent(accel, t);
	fillGyroEvent(gyro, t);
	fillTempEvent(temp, t);
	return true;
	}

but this function always returns true, even if the _read() fails are the data are grossly invalid.

Really there is no need for this, because this:

Adafruit_LSM6DS/Adafruit_LSM6DS.cpp

Line 254 in 1ea3bd2

_read();

which comes from here:

Adafruit_LSM6DS/Adafruit_LSM6DS.cpp

Lines 450 to 502 in 1ea3bd2

	void Adafruit_LSM6DS::_read(void) {
	// get raw readings
	Adafruit_BusIO_Register data_reg = Adafruit_BusIO_Register(
	i2c_dev, spi_dev, ADDRBIT8_HIGH_TOREAD, LSM6DS_OUT_TEMP_L, 14);
	uint8_t buffer[14];
	data_reg.read(buffer, 14);
	rawTemp = buffer[1] << 8 | buffer[0];
	temperature = (rawTemp / temperature_sensitivity) + 25.0;
	rawGyroX = buffer[3] << 8 | buffer[2];
	rawGyroY = buffer[5] << 8 | buffer[4];
	rawGyroZ = buffer[7] << 8 | buffer[6];
	rawAccX = buffer[9] << 8 | buffer[8];
	rawAccY = buffer[11] << 8 | buffer[10];
	rawAccZ = buffer[13] << 8 | buffer[12];
	lsm6ds_gyro_range_t gyro_range = getGyroRange();
	float gyro_scale = 1; // range is in milli-dps per bit!
	if (gyro_range == ISM330DHCX_GYRO_RANGE_4000_DPS)
	gyro_scale = 140.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_2000_DPS)
	gyro_scale = 70.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_1000_DPS)
	gyro_scale = 35.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_500_DPS)
	gyro_scale = 17.50;
	if (gyro_range == LSM6DS_GYRO_RANGE_250_DPS)
	gyro_scale = 8.75;
	if (gyro_range == LSM6DS_GYRO_RANGE_125_DPS)
	gyro_scale = 4.375;
	gyroX = rawGyroX * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	gyroY = rawGyroY * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	gyroZ = rawGyroZ * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	lsm6ds_accel_range_t accel_range = getAccelRange();
	float accel_scale = 1; // range is in milli-g per bit!
	if (accel_range == LSM6DS_ACCEL_RANGE_16_G)
	accel_scale = 0.488;
	if (accel_range == LSM6DS_ACCEL_RANGE_8_G)
	accel_scale = 0.244;
	if (accel_range == LSM6DS_ACCEL_RANGE_4_G)
	accel_scale = 0.122;
	if (accel_range == LSM6DS_ACCEL_RANGE_2_G)
	accel_scale = 0.061;
	accX = rawAccX * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	accY = rawAccY * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	accZ = rawAccZ * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	}

relies actually on this:

https://github.com/adafruit/Adafruit_BusIO/blob/b903c7e0154cfe20ea618f972fad3277feae6fb2/Adafruit_BusIO_Register.cpp#L201-L234

which does in fact return a valid and carefully checked flag indicating if the communication worked or not (this can be checked by looking at the function used to write and read:

https://github.com/adafruit/Adafruit_BusIO/blob/b903c7e0154cfe20ea618f972fad3277feae6fb2/Adafruit_I2CDevice.cpp#L238-L246

which does take care of the quality of what is read and written, and the low level read and write are written and checked with great care:

https://github.com/adafruit/Adafruit_BusIO/blob/b903c7e0154cfe20ea618f972fad3277feae6fb2/Adafruit_I2CDevice.cpp#L77-L158

)

How to fix the issue

• the _read function should return a validity flag, that is just the propagation of the flag returned by the Adafruit_BusIO_Register read call:

Adafruit_LSM6DS/Adafruit_LSM6DS.cpp

Lines 450 to 502 in 1ea3bd2

	void Adafruit_LSM6DS::_read(void) {
	// get raw readings
	Adafruit_BusIO_Register data_reg = Adafruit_BusIO_Register(
	i2c_dev, spi_dev, ADDRBIT8_HIGH_TOREAD, LSM6DS_OUT_TEMP_L, 14);
	uint8_t buffer[14];
	data_reg.read(buffer, 14);
	rawTemp = buffer[1] << 8 | buffer[0];
	temperature = (rawTemp / temperature_sensitivity) + 25.0;
	rawGyroX = buffer[3] << 8 | buffer[2];
	rawGyroY = buffer[5] << 8 | buffer[4];
	rawGyroZ = buffer[7] << 8 | buffer[6];
	rawAccX = buffer[9] << 8 | buffer[8];
	rawAccY = buffer[11] << 8 | buffer[10];
	rawAccZ = buffer[13] << 8 | buffer[12];
	lsm6ds_gyro_range_t gyro_range = getGyroRange();
	float gyro_scale = 1; // range is in milli-dps per bit!
	if (gyro_range == ISM330DHCX_GYRO_RANGE_4000_DPS)
	gyro_scale = 140.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_2000_DPS)
	gyro_scale = 70.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_1000_DPS)
	gyro_scale = 35.0;
	if (gyro_range == LSM6DS_GYRO_RANGE_500_DPS)
	gyro_scale = 17.50;
	if (gyro_range == LSM6DS_GYRO_RANGE_250_DPS)
	gyro_scale = 8.75;
	if (gyro_range == LSM6DS_GYRO_RANGE_125_DPS)
	gyro_scale = 4.375;
	gyroX = rawGyroX * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	gyroY = rawGyroY * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	gyroZ = rawGyroZ * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0;
	lsm6ds_accel_range_t accel_range = getAccelRange();
	float accel_scale = 1; // range is in milli-g per bit!
	if (accel_range == LSM6DS_ACCEL_RANGE_16_G)
	accel_scale = 0.488;
	if (accel_range == LSM6DS_ACCEL_RANGE_8_G)
	accel_scale = 0.244;
	if (accel_range == LSM6DS_ACCEL_RANGE_4_G)
	accel_scale = 0.122;
	if (accel_range == LSM6DS_ACCEL_RANGE_2_G)
	accel_scale = 0.061;
	accX = rawAccX * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	accY = rawAccY * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	accZ = rawAccZ * accel_scale * SENSORS_GRAVITY_STANDARD / 1000;
	}

Ie become something like:

 void Adafruit_LSM6DS::_read(void) { 
   // get raw readings 
   Adafruit_BusIO_Register data_reg = Adafruit_BusIO_Register( 
       i2c_dev, spi_dev, ADDRBIT8_HIGH_TOREAD, LSM6DS_OUT_TEMP_L, 14); 
  
   uint8_t buffer[14]; 
   if(!data_reg.read(buffer, 14)){
    return(false);
  }
  
   rawTemp = buffer[1] << 8 | buffer[0]; 
   temperature = (rawTemp / temperature_sensitivity) + 25.0; 
  
   rawGyroX = buffer[3] << 8 | buffer[2]; 
   rawGyroY = buffer[5] << 8 | buffer[4]; 
   rawGyroZ = buffer[7] << 8 | buffer[6]; 
  
   rawAccX = buffer[9] << 8 | buffer[8]; 
   rawAccY = buffer[11] << 8 | buffer[10]; 
   rawAccZ = buffer[13] << 8 | buffer[12]; 
  
   lsm6ds_gyro_range_t gyro_range = getGyroRange(); 
   float gyro_scale = 1; // range is in milli-dps per bit! 
   if (gyro_range == ISM330DHCX_GYRO_RANGE_4000_DPS) 
     gyro_scale = 140.0; 
   if (gyro_range == LSM6DS_GYRO_RANGE_2000_DPS) 
     gyro_scale = 70.0; 
   if (gyro_range == LSM6DS_GYRO_RANGE_1000_DPS) 
     gyro_scale = 35.0; 
   if (gyro_range == LSM6DS_GYRO_RANGE_500_DPS) 
     gyro_scale = 17.50; 
   if (gyro_range == LSM6DS_GYRO_RANGE_250_DPS) 
     gyro_scale = 8.75; 
   if (gyro_range == LSM6DS_GYRO_RANGE_125_DPS) 
     gyro_scale = 4.375; 
  
   gyroX = rawGyroX * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0; 
   gyroY = rawGyroY * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0; 
   gyroZ = rawGyroZ * gyro_scale * SENSORS_DPS_TO_RADS / 1000.0; 
  
   lsm6ds_accel_range_t accel_range = getAccelRange(); 
   float accel_scale = 1; // range is in milli-g per bit! 
   if (accel_range == LSM6DS_ACCEL_RANGE_16_G) 
     accel_scale = 0.488; 
   if (accel_range == LSM6DS_ACCEL_RANGE_8_G) 
     accel_scale = 0.244; 
   if (accel_range == LSM6DS_ACCEL_RANGE_4_G) 
     accel_scale = 0.122; 
   if (accel_range == LSM6DS_ACCEL_RANGE_2_G) 
     accel_scale = 0.061; 
  
   accX = rawAccX * accel_scale * SENSORS_GRAVITY_STANDARD / 1000; 
   accY = rawAccY * accel_scale * SENSORS_GRAVITY_STANDARD / 1000; 
   accZ = rawAccZ * accel_scale * SENSORS_GRAVITY_STANDARD / 1000; 

  return(true);
 } 

• all function relying on _read, including but not only:

Adafruit_LSM6DS/Adafruit_LSM6DS.cpp

Lines 251 to 261 in 1ea3bd2

	bool Adafruit_LSM6DS::getEvent(sensors_event_t *accel, sensors_event_t *gyro,
	sensors_event_t *temp) {
	uint32_t t = millis();
	_read();
	// use helpers to fill in the events
	fillAccelEvent(accel, t);
	fillGyroEvent(gyro, t);
	fillTempEvent(temp, t);
	return true;
	}

should actually read the (newly introduced) status flag returned by the _read function and use it as their own return flag.

discussion about implementation

• do you agree?
• anything I missed?

[jerabaul29]

If you agree with this, I could work on a suggestion of pull request, but I would like to double check with you first that I do not miss anything before proceeding with the pull request work etc :) .

[ladyada]

sure go ahead and submit a PR

[jerabaul29]

PR submitted :) .