-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding driver for the Texas Instruments HDC1080 #7888
Conversation
…ta bits/parity/stop bits setting in the hardware serial port.
… the internationalized resource files.
… development # Conflicts: # tasmota/settings.h # tasmota/settings.ino # tasmota/support.ino # tasmota/support_command.ino # tasmota/tasmota.ino
a primitive instead of pointer to the expected string)
…of the sensor in the support_features.ino.
tasmota/xsns_92_hdc1080.ino
Outdated
Wire.write((uint8_t)reg); | ||
Wire.endTransmission(); | ||
|
||
delay(HDC1080_CONV_TIME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Most of the time delay()
is to be avoided since you're stopping the whole Tasmota for a full 50ms cycle. Could you just let it run and read the value at next 50ms tick, if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @s-hadinger, understood. I'll take a look at that and test. Also, 50 ms is a conservative time which I kept while I was troubleshooting the communication. Theoretically 15 ms should be sufficient, according to the datasheet anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-hadinger do you have a good example where we are doing this? I am pondering on using FUNC_EVERY_SECOND as long as the sensor doesn't care that we read the values that long after sending the command. Apart from that it should not be a problem as our sample rate in Tasmota doesn't go below that anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you could is simply send the command to the sensor, use a boolean to know that you expect a reading at next tick, and return.
Then use FUNC_EVERY_50_MSECOND
. At the next FUNC_EVERY_50_MSECOND call, just read back the value and reset the boolean. You could argue that there might be less than 50ms between the initial call and the read request.
If you want to protect, just set a uint32_t timer = millis() + 30;
. At each FUNC_EVERY_50_MSECOND
, test using TimeReached(timer)
. If true, do the reading and reset the timer, if false return and wait for next tick.
It's important to use TimerReached()
and not a simple comparison, because uint32_t
overflows after ~50 days. TimerReached()
does an accurate check even in case of an overflow.
I2CDEVICES.md
Outdated
@@ -66,3 +66,4 @@ Index | Define | Driver | Device | Address(es) | Description | |||
42 | USE_DS1624 | xsns_59 | DS1624 | 0x48 - 0x4F | Temperature sensor | |||
43 | USE_AHT1x | xsns_63 | AHT10/15 | 0x38 | Temperature and humidity sensor | |||
44 | USE_WEMOS_MOTOR_V1 | xdrv_34 | | 0x2D - 0x30 | WEMOS motor shield v1.0.0 (6612FNG) | |||
92 | USE_HDC1080 | xsns_92 | HDC1080 | 0x40 | Temperature and Humidity sensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep incrementing the Index as it has no relation to file name but only to startup sequenc. So change the index to 45.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arendst got it.
@@ -182,6 +182,9 @@ extern "C" void custom_crash_callback(struct rst_info * rst_info, uint32_t stack | |||
#define USE_DHT12 // Add I2C code for DHT12 temperature and humidity sensor (+0k7 code) | |||
#define USE_DS1624 // Add I2C code for DS1624, DS1621 sensor | |||
//#define USE_AHT1x // Enable AHT10/15 humidity and temperature sensor (I2C address 0x38) (+0k8 code) | |||
#define USE_HDC1080 // Enable HDC1080 temperature/humidity sensor | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable HDC for now as I need to tests it's consequences first. This also is in line with your change in BUILD.md where you rightfully disabled the sensor in all binaries.
tasmota/xsns_92_hdc1080.ino
Outdated
@@ -0,0 +1,288 @@ | |||
/* | |||
xsns_92_hdc1080.ino - Texas Instruments HDC1080 temperature and humidity sensor support for Tasmota | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use filename xsns_65_hdc1080.ino
tools/decode-status.py
Outdated
@@ -178,7 +178,7 @@ | |||
"USE_INA219","USE_SHT3X","USE_MHZ19","USE_TSL2561", | |||
"USE_SENSEAIR","USE_PMS5003","USE_MGS","USE_NOVA_SDS", | |||
"USE_SGP30","USE_SR04","USE_SDM120","USE_SI1145", | |||
"USE_SDM630","USE_LM75AD","USE_APDS9960","USE_TM1638" | |||
"USE_SDM630","USE_LM75AD","USE_APDS9960","USE_TM1638", "USE_HDC1080" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DON'T DO THIS. It breaks the program is it extents the array over it's limit.
If you are not sure what it does, do not make changes. Again, this is admin code that I will update once your driver is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right ok, Kind of expected it would just be a not very brittle dictionary of driver names.
tasmota/support_features.ino
Outdated
@@ -254,8 +254,8 @@ void GetFeatures(void) | |||
#ifdef USE_SHT | |||
feature_sns1 |= 0x00000100; // xsns_07_sht1x.ino | |||
#endif | |||
#ifdef USE_HTU | |||
feature_sns1 |= 0x00000200; // xsns_08_htu21.ino | |||
#if defined(USE_HTU) || defined(USE_HDC1080) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this. If both HTU and HDC are connected use the correct online commands to disable one of them (your code must support this. See HTU code what I mean).
Don't update support_features.ino any way. I'll dot the administration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok understood, I would be curious however as to why exactly we are using these flags, and the choice of having a driver being assigned a specific bit from this 4 byte word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bits pop-up in status message status 4
and tell all assisting users what features are enabled in the binary. Only for debugging and helping the ones in need (why does my driver not work). If you only knew how much debug info is in this code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Regarding "the correct online commands", you mean the I2cDriverXX [0|1] commands? But aren't these using the driver index # to achieve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. The index in I2CDEVICES.md is used as i2c is used by sensors, drivers and displays all with their own xxxx_yy_name.ino file.
I have a board with one of these too, looking forward to testing it out! |
Improved runtime impact by replacing the sleep between the I2C operations with separate code triggered by timer events.
Hi all, added improvements and reverted the "admin" code changes. As suggested by @s-hadinger, replaced the sleep instruction with the splitting of the write and read operations in different functions, and having the second be executed only after FUNC_EVERY_50_MSECOND. Functionally everything is working as before, but not all the reads are succesful:
I've played a bit with the timer value but it doesn't seem to make much difference. My first guess is that the sensor may be timing out before we read it on occasion. Cheers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at your code, I have the impression the timer always fires too soon.
} | ||
|
||
is_reading = true; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where you should put timer_hdc1080 = millis() + HDC1080_CONV_TIME;
tasmota/xsns_65_hdc1080.ino
Outdated
uint8_t hdc_valid = 0; | ||
|
||
bool is_reading = false; | ||
uint32_t timer = millis() + HDC1080_CONV_TIME; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timer = millis() + HDC1080_CONV_TIME;
is ran at initialization time of the code, it's not useful here. Just declare uint32_t timer_hdc1080
. Also it may be wise not to call it just timer
as this may clash with other global variables.
tasmota/xsns_65_hdc1080.ino
Outdated
if(!HdcRead()) { | ||
AddLogMissed((char*) hdc_type_name, hdc_valid); | ||
} | ||
timer = millis() + HDC1080_CONV_TIME; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not needed. The timer must be set just after the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-hadinger which line? When it ticks I need to update the timer variable (not a timer in itself, just a the value of the next instant when something needs to be done) to the closest instant after which I expect data to be available in the sensor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ok @s-hadinger ok I see the point. The time it takes to finish the read from the previous iteration will condition how much time is left as the timer is armed again.
Add support for HDC1080 Temperature and Humidity sensor by Luis Teixeira (#7888)
@@ -494,6 +494,8 @@ | |||
// #define USE_DS1624 // [I2cDriver42] Enable DS1624, DS1621 temperature sensor (I2C addresses 0x48 - 0x4F) (+1k2 code) | |||
// #define USE_AHT1x // [I2cDriver43] Enable AHT10/15 humidity and temperature sensor (I2C address 0x38) (+0k8 code) | |||
// #define USE_WEMOS_MOTOR_V1 // [I2cDriver44] Enable Wemos motor driver V1 (I2C addresses 0x2D - 0x30) (+0k7 code) | |||
// #define USE_HDC1080 // [I2cDriver92] Enable HDC1080 temperature/humidity sensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a [I2cDriver45]
? Looks like an artifact from the first patch.
Description:
After having learned that some breakout boards (normally available at the usual online retail providers) do not necessarily have the description corresponding to the actual parts (in this case sensors) present on the PCB, I found that a problem can become an opportunity.
In this particular case, I obtained a board labelled as CJMCU-8128, which was described as containing the sensors CCS811+SI7021+BMP280. I soon found that instead of the Si7021 (which is currently supported in this project), it had the Texas Instruments HDC1080, which in spite of being pin compatible, having a very similar package and by default being set on the same I2C address (0x40), it is however a totally different chip: the registers are different, the modes of operation are different, and the characteristics are also different (e.g. the HDC1080 has 14-bit resolution for the humidity whereas the Si7021 has only 12-bit resolution).
As the support for the HDC1080 was missing here in Tasmota, I decided to implement the driver, test it in my setup (a NodeMCU with multiple air quality sensors attached to it), and open this pull request.
In order to validate if conflicts could occur, I have tested with this set of drivers with working sensors:
and also enabled the driver for a sensor not present but which uses the same I2C address as the HDC1080:
It can be the case that here or there, project convention(s) may not have been followed, in particular regarding the numbers to assign to the driver:
and also what to do in the feature_sns1 flags. As shown below, I was unsure where to place it, given that apparently all bits are in use for existing drivers:
Potential improvements on top of this feature
An eventual improvement (which in this phase I did not consider) would be to provide the ability to turn on the heater when the sensor is stuck or saturated for too long at 100% humidity and the temperature is sufficiently low.
Also, adjusting the RH offsets as a function of the ambient temperature would be beneficial for improving the sensor accuracy. This is currently not contemplated as well.
Sample output:
Checklist: