search

adafruit / Adafruit_HMC5883_Unified

48 stars 35 forks source link

No need to wait for Wire.available(). #1

Open Koepel opened 9 years ago

Koepel commented 9 years ago

After Wire.requestFrom() there is not need to wait for data like this : "while (Wire.available() < 6);". When the Wire.requestFrom() returns, the I2C transmission has completely finished.

There is also a Wire.endTransmission() after a Wire.requestFrom(), that Wire.endTransmission() does not belong there.

A test for the return value of Wire.endTransmission can be used to detect if the device is detected. Also the return value of Wire.requestFrom can be tested to detect a collision in a multi-Master bus.

budryerson commented 8 years ago

The public function "bool begin(void);" always returns TRUE.

The function is apparently meant to detect whether an HMC5883 device is detected, and to trigger the "Ooops, no HMC5883 detected ... Check your wiring!" message in the Adafruit sensor initialization example code if no device is detected. It does no such thing; and the function's code always returns TRUE.

grossadamm commented 8 years ago

@budryerson isn't that a separate issue from what @Koepel is talking about?

budryerson commented 8 years ago

@grossadamm Thank you for your gentle correction. I have resubmitted the comment as a "new issue."

grossadamm commented 8 years ago

Made a pull request that will help with those that run into their code hanging due to this disagreement. https://github.com/adafruit/Adafruit_HMC5883_Unified/pull/4 This allows a timeout for the while loop, since my guess is that this disagreement is at a stalemate.

Koepel commented 8 years ago

I'm sorry grossadamm, but a timeout is wrong as well. After a Wire.requestFrom() there is no need to wait. Either the return value of Wire.requestFrom() or the return value of Wire.available() can be used to check if the I2C transaction failed, but waiting for something is not needed. The Wire.requestFrom() waits until the I2C transaction has finished, and waiting for the receive buffer makes no sense.

The Arduino Wire library can not be used in a way that waiting like that is needed.

There are (software) implementations that are not compatible with the Arduino Wire library. The Wire library for the Due started wrong but is now okay. The Wire library for the ESP8266 is not yet fully compatible. But as far as I know all the Wire.requestFrom() functions wait until the I2C transaction has finished.

grossadamm commented 8 years ago

What happens when the wire becomes disconnected? Or, more likely, as mentioned in other threads there is a collision between two masters? The existing while loop will never exit. I can assure you that having a while loop that never exits must also be wrong. I personally have run into this situation on a number of occasions and my project can not tolerate a possibility of hanging.

You and the developer(s) seem at odds on if your understanding on the usage of Wire.available() and Wire.requestFrom() is correct. I tend to your side, but perhaps this intermediary step will satisfy parties in my situation until you two come to an agreement.

In the event you consider this pull request unacceptable, please feel free to make a pull request with what you consider the proper solution.

Koepel commented 8 years ago

The Wire.endTransmission() returns an error, and the Wire.requestFrom() returns the number of received bytes and the Wire.available() return how many bytes are available in the buffer. Those can be used to check for error or collision and so on.

The forever while loop will never return when the sensor is disconnected or with a collision. However, there is no need for this intermediary step. Just do it right, that's all. Adafruit is doing it better and better, and in most new code the data is just read after a Wire.requestFrom().

An extra check for something wrong on the I2C bus can be by checking the Wire.requestFrom() return variable or with Wire.available() (just once).

The official Arduino Wire library is sadly blocking internally. When something is wrong, some functions of the Wire library will never return and the sketch will halt. That is where those timeouts are needed (inside the Wire library).

grossadamm commented 8 years ago

As I said earlier, I understand and agree with many of your points. But as I also said earlier, I merely want a way to not hang indefinitely. I solved that as far as I can tell. It may not be the correct solution per you, but it seems to work per me.

I appreciate your input, but if in fact you want it done right per you, I suggest you submit a pull request.