writeBits/readBits may result in negative bit shift

[deiteris]

Description

I've been porting a library for BMI160 to I2Cdev library and noticed that registers are not read/updated properly when manipulating bits, while bytes are read/written correctly.

I see that writeBits() has this code:

bool I2Cdev::writeBits(uint8_t devAddr, uint8_t regAddr, uint8_t bitStart, uint8_t length, uint8_t data) {
  ...
  uint8_t b;
  if (readByte(devAddr, regAddr, &b) != 0) {
  // If length = 3 and bitStart = 0
    uint8_t mask = ((1 << length) - 1) << (bitStart - length + 1); // Results in (7) << (-3)
    data <<= (bitStart - length + 1); // Same problem here
    ...
}

The same problem is in readBits().

If a bit start position is less than length + 1, this may result in a negative bit shift that's UB.

I managed to workaround this by simply shifting by bit start position, but is it correct? I'm not sure I understand why this manipulation with bit start position is done.

How to reproduce

The following raw example fails:

#include "I2Cdev.h"

#define BMI160_RA_GYRO_RANGE 0x43
#define BMI160_GYRO_RANGE_SEL_BIT 0
#define BMI160_GYRO_RANGE_SEL_LEN 3

uint8_t gyroRange = 0x03;
// Write 0x03 to register
I2Cdev::writeBits(0x68, BMI160_RA_GYRO_RANGE, BMI160_GYRO_RANGE_SEL_BIT, BMI160_GYRO_RANGE_SEL_LEN, gyroRange);
// Read value to the same variable
I2Cdev::readBits(0x68, BMI160_RA_GYRO_RANGE, BMI160_GYRO_RANGE_SEL_BIT, BMI160_GYRO_RANGE_SEL_LEN, &gyroRange);
printf("Gyro range: %d", gyroRange); // Outputs 0, expected 3

[ZHomeSlice]

The error is in the #define BMI160_GYRO_RANGE_SEL_BIT 0 Value This should be greater than length

/** Read multiple bits from an 8-bit device register.
 * @param devAddr I2C slave device address
 * @param regAddr Register regAddr to read from
 * @param bitStart First bit position to read (0-7)
 * @param length Number of bits to read (not more than 8)
 * @param data Container for right-aligned value (i.e. '101' read from any bitStart position will equal 0x05)
 * @param timeout Optional read timeout in milliseconds (0 to disable, leave off to use default class value in I2Cdev::readTimeout)
 * @return Status of read operation (true = success)
 */
int8_t I2Cdev::readBits(uint8_t devAddr, uint8_t regAddr, uint8_t bitStart, uint8_t length, uint8_t *data, uint16_t timeout, void *wireObj) {
    // 01101001 read byte
    // 76543210 bit numbers
    //    xxx   args: bitStart=4, length=3
    //    010   masked
    //   -> 010 shifted
    uint8_t count, b;
    if ((count = readByte(devAddr, regAddr, &b, timeout, wireObj)) != 0) {
        uint8_t mask = ((1 << length) - 1) << (bitStart - length + 1);
        b &= mask;
        b >>= (bitStart - length + 1);
        *data = b;
    }
    return count;

Z

[deiteris]

Hi @ZHomeSlice,

Thank you for your response. But it doesn't seem to be necessarily be greater than length.

However, thanks to your tip and now that I'm re-reading this, if I understand correctly, bit numbers are read from right to left, but the length of read bit positions is read from left to right.

I just tried to set #define BMI160_GYRO_RANGE_SEL_BIT 2 with the same #define BMI160_GYRO_RANGE_SEL_LEN 3, and it seems to set the bit value as expected. So the behavior with these values is as follows:

bool I2Cdev::writeBits(uint8_t devAddr, uint8_t regAddr, uint8_t bitStart, uint8_t length, uint8_t data) {
  ...
  uint8_t b;
  if (readByte(devAddr, regAddr, &b) != 0) {
    // If length = 3 and bitStart = 2
    uint8_t mask = ((1 << length) - 1) << (bitStart - length + 1); // Results in (7) << (0) - 0000 0111 masked
    ...
}

I feel like this should be explicitly mentioned, as from the looking at the param descriptions, I would expect both position and length read from right to left and this is why I found this confusing at first.

[ZHomeSlice]

@deiteris You're right If you want to read bitStart at Zero you can only have a max length of 1 If you wanted to read to bitStart at 7 you can have a max length of 8 but you might as well read or write the whole thing with another function as it is faster.

and Yep :) Been there and done that. I found Jeff's library to be extremely useful in laying the groundwork for working with i2c once you understand how the bit Manipulation math works. I had to beat through this in-depth to use the i2cdevlib in my version of the MPU6050 library (found Here: Simple_MPU6050) Note: Jeffs and My MPU6050 library are very similar in the resulting function. I extend Jeff's i2cdev library into My "Simple_MPU6050" and since everything is similar I contribute a lot of improvements to jeffs MPU6050 library as it is more widely used and understood. The biggest difference between my Simple_mpu6050 library Jeff's MPU6050 library is the use of functions vs macros. Jeff's uses functions for each of the configurable items while I created a series of macros to do the job. but the end result is likely the same. Also, I have callback functions that trigger when data is available which I find convenient, but this will likely confuse most beginners.

Z