Temp humid sensor #24
Conversation
|
Added a timeout feature after 5 sections of failed Wire return values. Also added a constructor instead of hard-coding register and address values. |
|
|
||
| #include <Arduino.h> | ||
| #include "dfrobot/DFRobot_SHT3x.h" | ||
| #include "Wire.h" |
There was a problem hiding this comment.
Not important for this PR, but in general, you can have two "categories" of includes:
- libraries that contains types you are using in your API (include in your .h file)
- libraries that contain code you'll need to implement your logic (include in your cpp file)
Basically, think of it as "do others who use this file also need this library?" If so, the include goes into the header file. If not, then it can just be in the ,cpp file. So in this case, you can move the include to the .cpp file.
Again, it doesn't actually break or change any functionality, but it's good practice as it means that including one header gives you the least amount of transitive dependencies.
There was a problem hiding this comment.
Try adding .DS_STORE to your global gitignore
| TempHumiditySensor::TempHumiditySensor() : sensor(&Wire, 0x44, 4) { } | ||
| void TempHumiditySensor::readReg(){ | ||
| Wire.beginTransmission(addr); | ||
| // Rewrote with overloaded method so that you don't need to use & on a literal |
There was a problem hiding this comment.
So the reason we used &Wire instead of just Wire is that there are actually several different Wire objects, one for each I2C chip on the Teensy itself -- Wire, Wire1, and Wire3. Depending on which pins the electrical team decides to use, a different chip may be needed.
In the past, we've found it tricky to switch back and forth (eg for testing or debugging) because we were using Wire all over the file. So we used a reference to a wire object to allow passing in different wire objects and be sure they all changed at once. Also acceptable is #define WIRE Wire and then that can be changed as well.
| void TempHumiditySensor::setup() { | ||
| Serial.print("Initializing temp/humidity sensor..."); | ||
| while (sensor.begin() != 0) { | ||
| while (Wire.begin() != 1) { |
There was a problem hiding this comment.
Is this supposed to be != 0?
There was a problem hiding this comment.
Actually, I just checked the docs, looks like Wire.begin() doesn't return anything at all?
| Serial.print("."); | ||
| if (millis() - startTime > timeout) { | ||
| // Timeout reached, so issue an error | ||
| Serial.println("Sensor setup failed."); |
There was a problem hiding this comment.
Careful when leaving setup() early -- the rest of the code assumes that setup() finished without problems. What happens to readReg()'s Wire.beginTransmission(), Wire.write(), and Wire.requestFrom() if Wire.begin() did not finish? I see you did check Wire.endTransmission(), so if that is enough then good job there.
What I usually do here is add a field to the class bool isReady = false, then set that to true at the end of the setup() function. Then, all other functions can do a quick if (!isReady) return; to avoid getting into undefined behavior
| readReg(); | ||
| uint16_t temp = buf[0] << 8 | buf[1]; | ||
| return ((float) temp * 165 / 65535.0) - 40.0; |
There was a problem hiding this comment.
Definitely a style choice, but consider this: getTemperature() and getHumidity() are probably going to be called in the same loop() function, and so readReg() will be run twice in a row. Ignoring the delays, it's unlikely that the sensor itself will have new data in such a short time.
Our other classes have a void update() method -- something meant to be run once per loop(). Maybe you can put readReg() in there, and save the temperature and humidity into their own fields. Then, when these functions are called, just return the last read value from the fields directly.
It's a bit of a trade-off from efficiency vs real-time accuracy, so your call.
There was a problem hiding this comment.
Thank you for all the feedback! I will implement all of these changes. As for the .begin() error, my code is an adaptation of example code that was provided to me, so I definitely should've looked into the function definitions a little more thoroughly!
5 participants
Inegrated the temperature/humidity sensor into the main system, removing the old code and replacing it with the new implementation. I tried my best to find any old instances of the old sensor and replace them with my own or rename my functions to the old ones so that there was no need to rewrite existing code, but please let me know if I missed anything.