-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add a usermod for AHT10, AHT15 and AHT20 temperature/humidity sensors #3977
Conversation
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.
There are a couple of indentation errors as well as the in-line commented issues.
You might also want to add build flags and libraries to platformio_override.sample.ini
VSC AFAIK detects it and uses appropriate indentation by default. You can enter anything, though. |
I belive I've handled the issues. I used format document in vscode based on an editorconfig it came up with. Quick note - the CI isn't building my usermod, right?.. If you want, I could add a sample override file directly in the usermod folder, which could be referenced by a CI job for usermods. Something like the below.
|
Should I implement MQTT publishing, guarded by the "if mqtt compiled in" ifdefs? |
Indeed. That would be nice as currently none of the usermods go through compilation process. One more observation: Is it necessary to have "AHT Temperature" and "AHT Humidity"? There is no reason for "AHT" IMO. |
That would be welcome by many users. Including HA autodiscovery broadcast. |
Re. naming of info things I was not sure what to put there. In my setup, I'll have both the BME280, AHT20 and an INA226 (tbd). As the BME280 will provide "Temperature", I wasn't sure if also providing Temperature through the AHT was going to be an issue. |
As I read it, the two usermods would both be creating the |
Why on earth would you want two temperature sensors on one, LED driving, device? |
Well. The device I got from china had both in it to provide me with 3 values (temp, humidity and pressure).. So yay me .. :D Hilarious part is that a BMxxxx chip variant has all three values, but ... 🤷 |
You can always disable one of them... |
Entirely true. Either code it so that temp isn't read from one of them, either a checkbox or "255" decimals meaning "don't". Or discard pressure and keep temp+humidity. :P I'll have a look at the mqtt stuff. As for the naming of the ui json .. I can name it without AHT, but if the two usermods are run at the same time it's .. undefined.. |
So I:
|
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'll neither approve nor request changes as technically the code looks correct (as far as logical analysis goes), but IMO it could be made clearer by using unnamed struct bitfields.
Also reexamine all strings, please. There are advantages and disadvantages when using F() macro.
You could also avoid inlining (reduced code size) by moving function definitions outside of class definition. See other usermods.
- Reduce some strings - Use an unnamed struct to pack the bitfield instead
This pull request introduces a new usermod for integrating the AHT10, AHT15, and AHT20 humidity and temperature sensors into WLED. It uses the enjoyneering/AHT10 library to handle the wire stuff, and implements settings like:
Info view.
Settings view.