Skip to content
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 Sensirion SEN5X lib + I2C driver #17736

Merged
merged 1 commit into from
Jan 29, 2023
Merged

Conversation

tyeth
Copy link
Contributor

@tyeth tyeth commented Jan 17, 2023

Description:

Adds SEN5x series of Particulate sensors (1-10PPM/Temp/Humidity/NOx/VOC)
also SEN54 (voc+ppm+temp+humidity) or SEN50 (PPM only)
see https://sensirion.com/products/catalog/SEN55/ etc

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • [-] The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.6
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@tyeth
Copy link
Contributor Author

tyeth commented Jan 17, 2023

I haven't got an esp8266 for testing.
Original commits: development...tyeth:Tasmota:add-sen5x

@Jason2866
Copy link
Collaborator

Where is the Tasmota driver? This PR provides only the Sensirion lib.

@Jason2866 Jason2866 marked this pull request as draft January 18, 2023 08:54
@tyeth
Copy link
Contributor Author

tyeth commented Jan 18, 2023 via email

@tyeth tyeth force-pushed the pr-sen5x branch 2 times, most recently from def690e to b61a9ea Compare January 18, 2023 20:14
@tyeth tyeth marked this pull request as ready for review January 18, 2023 20:15
@Jason2866
Copy link
Collaborator

Please undo the password SSID changes in my_user_config.h

@tyeth tyeth force-pushed the pr-sen5x branch 2 times, most recently from ccea21e to 3f9f6bd Compare January 19, 2023 16:21
@tyeth
Copy link
Contributor Author

tyeth commented Jan 20, 2023

After loading a copy via tasmotaCompiler, with the air sensors enabled, it's showing as an SPS30, so something needs to change. Fail.
image
-Edit- The sps30 detection is lazy in the sense it marks success after finding any device at that address, so I'll refactor it to do it after verifying serial number and starting measurement commands in same method.

@tyeth tyeth marked this pull request as draft January 20, 2023 00:08
@sfromis
Copy link
Contributor

sfromis commented Jan 20, 2023

I2C address clashes leading to faulty detection is not too unusual, and it is commonly recommended for the user to mitigate such issues by using the command I2CDriver to deactivate one of them. As the sensors cannot be used at the same time anyway, it is not required to mitigate the issue in the other driver, but if it is easy to do....

@Jason2866
Copy link
Collaborator

@tyeth As sfromis explained the current driver regarding i2c handling is okay.
You can remove the Draft status and it is good to be merged.
Enhancing and refactoring other drivers should be done in an extra PR.

@tyeth
Copy link
Contributor Author

tyeth commented Jan 28, 2023

I've made significant improvements, especially around logging, more thorough testing in situ, and error handling depending on sensor model (no VOC/NOx/Temp/Hum with SEN50 - only particules, SEN54 missing only VOC, sen55 full features). It's also been thoroughly tested on both I2C busses with other sensors attached.

I must confess my sins, it was poorly tested and actually broken when first submitted so lessons learned and my apologies. I had problems building firmwares with all needed features, using tasmocompiler seemed to be okay, but my custom builds always showed max or min values for all sensors. I was trying to do too much at once as most newbies do - requiring display/lvgl/custom-defines, especially stumbled with certain unexpected defines like the AHT1x when I'm using the AHT20, they conflict and the docs says AHT1x isn't included in default builds but it is...

Happy enough now once the SPS30 pull request is merged. They can be merged separately but would be grateful if they made it into the next release together.

@tyeth tyeth marked this pull request as ready for review January 28, 2023 17:36
@tyeth tyeth marked this pull request as draft January 28, 2023 17:37
@tyeth
Copy link
Contributor Author

tyeth commented Jan 28, 2023

Sorry, forgot there were missing units. Will fix immediately.

@tyeth tyeth marked this pull request as ready for review January 28, 2023 17:51
@Jason2866
Copy link
Collaborator

Please remove the sps30 driver changes from this PR. The SPS30 PR will be merged together or before this PR ;-)

@tyeth
Copy link
Contributor Author

tyeth commented Jan 28, 2023

Ah sorry, slipped back in with the units ;)

@tyeth tyeth force-pushed the pr-sen5x branch 2 times, most recently from dd63823 to 6809698 Compare January 28, 2023 18:37
@tyeth
Copy link
Contributor Author

tyeth commented Jan 28, 2023

Should be all good, retested (ignore double percent in screenshot as it's fixed) and checked for spurious commits :)
image

@tyeth
Copy link
Contributor Author

tyeth commented Jan 28, 2023

Seen the failure, I'll add a guard around wire1 for esp32, falling back to wire if necessary.
-Edit- Done

@arendst arendst merged commit 2ed6020 into arendst:development Jan 29, 2023
@tyeth tyeth deleted the pr-sen5x branch January 29, 2023 13:07
arendst added a commit that referenced this pull request Jan 29, 2023
- ESP32 support for eigth energy phases/channels
- ESP32 command ``EnergyCols 1..8`` to change number of GUI columns
- ESP32 command ``EnergyDisplay 1..3`` to change GUI column presentation
- support for SEN5X gas and air quality sensor by Tyeth Gundry (#17736)
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.

None yet

5 participants