FOSSASystems / FOSSASAT-1

GNU General Public License v3.0
536 stars 106 forks source link

No need to wait after Wire.requestFrom() and it should not be used together with Wire.beginTransmission(). #3

Open Koepel opened 5 years ago

Koepel commented 5 years ago

In the file "FOSSASAT-1/Code/FossaSat1/power_control.cpp", in the function "Power_Control_INA2256_Check()", there is Wire.beginTransmission() just before the Wire.requestFrom(). That Wire.beginTransmission() may be removed. The timeout after the Wire.requestFrom() may be removed as well.

Explanation: Common-mistakes, number 1 and 3.

This section:

  // try to read
  Wire.beginTransmission(INA_ADDR);
  Wire.requestFrom((uint8_t)INA_ADDR, (uint8_t)2);
  uint32_t start = millis();
  while(!Wire.available()) {
    if(millis() - start >= INA_TIMEOUT) {
      // timed out
      return(false);
    }
  }

can be reduced to:

  // try to read
  Wire.requestFrom((uint8_t)INA_ADDR, (uint8_t)2);

If you want to check if the data was received, then you can do this:

  // try to read
  Wire.requestFrom((uint8_t)INA_ADDR, (uint8_t)2);
  if(Wire.available() != 2) {
    return(false);
  }

or this:

  // try to read
  if(Wire.requestFrom((uint8_t)INA_ADDR, (uint8_t)2) != 2)
    return(false);
jgromes commented 5 years ago

You're right, there is some redundancy, though it doesn't affect the intended function. The code was adapted from this library, you might want to open another issue there: https://github.com/jarzebski/Arduino-INA226/blob/968a684ed44d0e076a90c93feed07e834b5fdad4/INA226.cpp#L298

On a side note, is there a non-blocking equivalent of Wire.requestFrom()?

Koepel commented 5 years ago

In this case the extra code is harmless, but others could think that it is needed.

I have given up on jarzebski. See for example this: https://github.com/jarzebski/Arduino-MPU6050/issues/4 I tried to convince him that it is better to write code without bugs, then I tried to be annoying, but nothing helps.

There is a library for the Teensy boards that have non-blocking I2C functions and there is a I2C library that uses DMA, but those functions are not compatible with the Arduino Wire library.