k323r / flugkatze

MIT License
0 stars 0 forks source link

No need to wait after Wire.requestFrom(). #1

Open Koepel opened 6 years ago

Koepel commented 6 years ago

In the file "flugkatze/flightcontroller/v2_stm32/IMU.cpp", in the function "IMU::update()", there are two Wire.endTransmission() after each other. The second one is a Wire.endTransmission() on its own, and that is not allowed.

The function "IMU::getAddedRegisters()" is very complex, but all that timeout code is not needed. There is no such thing as a timeout after a Wire.requestFrom(). Explanation: Common-mistakes#1

When shifing 8 bits to the left, I prefer to do that with a 16 bits variable. If the Wire.available() tells that there are enough data bytes available, they can be read and will be valid (they will be 0...255). Only in case there is no data, then the Wire.read() will return -1.

uint16_t IMU::getAddedRegisters() {
  uint16_t reg1, reg2;
  if (Wire.available() >= 2) {   // still enough data in the buffer ?
    reg1 = (uint16_t) Wire.read();
    reg2 = (uint16_t) Wire.read();
  } else {
    return 0;  // return zero in case of a I2C bus error
  }
  return (reg1 << 8 |reg2);
}
k323r commented 6 years ago

Hi, thank you very much for your remarks, i fixed it. Are there any specific reasons for reading data from wire into a 16bit variable instead of a 8 bit variable? shouldn't make a difference, no?

Koepel commented 6 years ago

Only when shifting 8 bits to the left. Perhaps the compiler will fix it, but shifting a 8-bits variable to left 8 times is not nice.

This is also used a lot: return (Wire.read() << 8 | Wire.read()); However, the c++ language specifies that the order of evaluation is unspecified. When being strictly, that line is not correct. Therefor two separate Wire.read() calls on two separate lines is the best.

k323r commented 6 years ago

fair enough. i think the compiler fixed it, but using 16bits makes definetly more sense then.