Wisp Sensor Integrations to Main #186
Conversation
…ded example file for T6793 Sensor.
|
The sensor integrations have been verified by multiple parties. |
|
Waiting on a new release (4.8) with https://github.com/DFRobot/DFRobot_MultiGasSensor integrated for merge. |
| @@ -28,5 +28,20 @@ class I2CDevice : public Module{ | |||
| return false; | |||
| }; | |||
|
|
|||
| uint8_t scanI2C(){ | |||
There was a problem hiding this comment.
I feel like if we are gonna have this function in here it should be static. It just feels like the specific I2C device has no need to scan but I still think this file would be a good location, thoughts @rainewheary?
There was a problem hiding this comment.
I'd pull it out as a separate function - the class is I2CDevice, and I don't feel great about using it as a namespace for all I2C related actions that aren't device-specific.
|
|
||
| ////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| uint8_t Loom_MultiGasSensor::get_gas_i2c(const std::string& gasType) { | ||
| static const std::unordered_map<std::string, uint8_t> GAS_ADDRESSES = { |
There was a problem hiding this comment.
We make a new one of these every time, but it seems like it doesn't change, move to a member variable
| {"PH3", 0x45} | ||
| }; | ||
|
|
||
| auto it = GAS_ADDRESSES.find(gasType); |
There was a problem hiding this comment.
Not a fan of auto I feel like it just leads to ambiguity
There was a problem hiding this comment.
This is pretty conventional - the actual type is some horrible iterator thing.
There was a problem hiding this comment.
Fair feels gross still but okay I guess lowkey would rather a if(contains) and then a [] but this works too
| return; | ||
| } | ||
|
|
||
| const std::vector<std::string> gasTypes = { |
There was a problem hiding this comment.
Initialize as member variable
| }; | ||
|
|
||
| if (gasData == nullptr) { | ||
| gasData = new std::unordered_map<std::string, float>; |
There was a problem hiding this comment.
Ummmm... there better be a really good reason for this
There was a problem hiding this comment.
There isn't. Don't use new on smart pointers!
| } | ||
|
|
||
|
|
||
| String queryResult; |
There was a problem hiding this comment.
Absolutely not, use a c-string instead
There was a problem hiding this comment.
We may need to modify the library - there's a huge amount of pointless back-and-forth conversions going on (mainly due to an inadequate API that pervasively exposes Arduino strings all over the place). Very frustrating...
There was a problem hiding this comment.
Hmmm this is pain, but it may be required String is gross
| char output[OUTPUT_SIZE]; | ||
| char sensorError[OUTPUT_SIZE]; | ||
|
|
||
| byte data[4]; |
There was a problem hiding this comment.
What is this series of writes actually doing
| FUNCTION_START; | ||
| byte data[4]; | ||
| Wire.beginTransmission(i2s_addr); | ||
|
|
| }; | ||
|
|
||
| if (gasData == nullptr) { | ||
| gasData = new std::unordered_map<std::string, float>; |
There was a problem hiding this comment.
There isn't. Don't use new on smart pointers!
| uint8_t addr; | ||
| bool moduleInitialized = false; | ||
| bool needsReinit = false; | ||
| std::unordered_map<std::string, float> *gasData = nullptr; |
There was a problem hiding this comment.
There's no need to have a pointer here, unordered_map already manages it's own allocation. Just hold one directly.
| } | ||
|
|
||
|
|
||
| String queryResult; |
There was a problem hiding this comment.
We may need to modify the library - there's a huge amount of pointless back-and-forth conversions going on (mainly due to an inadequate API that pervasively exposes Arduino strings all over the place). Very frustrating...
|
|
||
|
|
||
| private: | ||
| uint8_t get_gas_i2c(const std::string& gasType); |
There was a problem hiding this comment.
Use camelCase for methods
This serves as a pull request to merge new Wisp sensor integrations into the main branch. The two sensors are the T6793 CO2 Sensor and the DFRobot Gravity sensor suite. For the DFRobots sensors, it is a modular setup that works with any of the gravity gas sensors. Here is the github with the code library that is used in the DFRobot code:
https://github.com/DFRobot/DFRobot_MultiGasSensor
The Wiki: https://wiki.dfrobot.com/SKU_SEN0465toSEN0476_Gravity_Gas_Sensor_Calibrated_I2C_UART
All code for the T6793 CO2 Sensor is base C with no external dependencies