Skip to content

Conversation

@jlblancoc
Copy link

Added optional timeout to all read functions to avoid infinite locks upon sensor failure or disconnection.

@LowPowerLab
Copy link
Owner

Thanks Jose,
When do you see such conditions of sensor failure or disconnection occur?
Does the I2C communication timeout if no sensor is attached?

@jlblancoc
Copy link
Author

Hi Felix, and thank you for sharing this project!

We had a problem of the uC getting locked in one of the data request methods when no sensor was connected... it's weird because I thought that I2C should be able to detect a lack of ACK (so perhaps the problem was raised by our particular hardware design), but without this patch the firmware always locked when a SI7021 sensor was not connected.

Copy link
Owner

@LowPowerLab LowPowerLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking OK but I would like to request some changes to the formatting of the code. Please keep the spacing (single double-space for tabs, etc) so that unmodified code would not be changed.
Also:

  • rather than passing parameters to all functions, I would implement a #define TIMEOUT_MS and use that only where needed, would provide a much cleaner change
  • there should not be any TABs, only spaces (and 1 TAB=2 spaces).
  • the changes to int SI7021::getSerialBytes are invalid, it needs to return a uint32_t, this function should be left untouched other than the timeout implementation.

Many thanks.

@chaveiro
Copy link

chaveiro commented Mar 8, 2017

LowPowerLab you did the code review and instead of accepting the commit and submit your own version on top with the proposed fixes expected the original author to do that for you. He didn't.
And now this necessary fail safe to this lib is pending for 5 months...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants