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 ENS16x (air quality) and ENS210 (temp & RH) sensors #19479

Merged
merged 21 commits into from
Sep 24, 2023

Conversation

chrfriese123
Copy link
Contributor

Description:

Related issue (if applicable): fixes #

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.11
  • I accept the CLA.

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

Added ENS16x library enabling read-out of ENS160 and ENS161 sensor component (follow-up of CCS811 and iAQcore)
Added ENS210 library to read out ENS210 temperature & humidity sensor
Add air quality sensor readout for ENS160 and ENS161 checking two possible I2C addresses (follow up sensor for CCS811 and iAQcore)
Add temperature and humidity sensor readout checking two possible I2C addresses
Add USE_ENS16x and USE_ENS210
Add USE_ENS16x and ENS210
Add USE_ENS16x and USE_ENS210
Add USE_ENS16x and USE_ENS210
Add USE_ENS16x and USE_ENS210
Add USE_ENS16x and ENS210
Add USE_ENS16x and USE_ENS210
Corrected I2X number
Corrected I2C number
@barbudor
Copy link
Contributor

barbudor commented Sep 8, 2023

Please remove *.bak files

Copy link
Contributor

@barbudor barbudor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally you should have done 2 PR, one for each sensor

ALso please advise code increase and memory increase for each of the drivers for both ESP8266 and ESP32
Thanks

Comment on lines 114 to 115
#define USE_ENS16x // [I2cDriver85] Enable ENS160 and ENS161 sensor (I2C addresses 0x52 and 0x53)
#define USE_ENS210 // [I2cDriver86] Enable ENS210 sensor (I2C addresses 0x43 and 0x44)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not enable your drivers by default
Also comment the increase in code size for ESP8266 (and memory too would be great)
Comment those lines so people are aware of what should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabled by default, memory information added

BUILDS.md Outdated
Comment on lines 135 to 136
| USE_ENS16x | - | - / x | - | x | - | - |
| USE_ENS210 | - | - / x | - | x | - | - |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not add your drivers by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabled by default, memory information added

Comment on lines 691 to 692
#define USE_ENS16x // [I2cDriver85] Enable ENS160 and ENS161 sensor (I2C addresses 0x52 and 0x53)
#define USE_ENS210 // [I2cDriver86] Enable ENS210 sensor (I2C addresses 0x43 and 0x44)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not enable your drivers by default
Also comment the increase in code size for ESP8266 (and memory too would be great)
Comment those lines so people are aware of what should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabled by default, memory information added

Comment on lines 38 to 60
ScioSense_ENS16x ens16xa(ENS16x_I2CADDR_0);
struct {
uint16_t TVOC;
uint16_t eCO2;
uint16_t AQIS;

uint8_t ready = 0;
uint8_t type = 0;;
uint8_t tcnt = 0;
uint8_t ecnt = 0;
} ENS16xdataA;

ScioSense_ENS16x ens16xb(ENS16x_I2CADDR_1);
struct {
uint16_t TVOC;
uint16_t eCO2;
uint16_t AQIS;

uint8_t ready = 0;
uint8_t type = 0;;
uint8_t tcnt = 0;
uint8_t ecnt = 0;
} ENS16xdataB;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use dynamically allocated memory
This is using memory in any built your driver is included even if no-one is using it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposal. I'll try to implement later.

@chrfriese123
Copy link
Contributor Author

Should I still split the pull request for both sensors?

@barbudor
Copy link
Contributor

barbudor commented Sep 8, 2023

Hi @chrfriese123
Thanks
I'll let @arendst comment if he wants you to split

I find the code increase is quite high. Is the whole libraries needed for each sensors?
I've seen quite some large sensors libraries with code that is not used in general use case. But once a C/Cpp module is merge, all the code comes with it. There a few cases where it was most efficient to extract only the needed pieces.
I'll have further look tomorrow

Thanks

@Jason2866
Copy link
Collaborator

@chrfriese123 Is the ENS160 working with this driver? Are there development boards available for the ENS210? Where to get and what do the cost?

@chrfriese123
Copy link
Contributor Author

@chrfriese123 Is the ENS160 working with this driver? Are there development boards available for the ENS210? Where to get and what do the cost?

@Jason2866 the driver is working with ENS160 and ENS161. TVOC and eCO2 output is the same. Difference is only the air quality output and the low power option for ENS161.
Development boards for ENS16x are available from Adafruit and Sparkfun. The ENS210 from Sparkfun seems to be out of stock. The combined eval kit from Mouser is (unfortunately) bloody expensive.

@chrfriese123
Copy link
Contributor Author

Hi @chrfriese123 Thanks I'll let @arendst comment if he wants you to split

I find the code increase is quite high. Is the whole libraries needed for each sensors? I've seen quite some large sensors libraries with code that is not used in general use case. But once a C/Cpp module is merge, all the code comes with it. There a few cases where it was most efficient to extract only the needed pieces. I'll have further look tomorrow

Thanks

@barbudor, thanks for suggestion. Ideally I don’t want to diverge too much from the official driver. But I’ll give it a try to reduce to a minimum.

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

We don't care about the official driver. What we care about is Flash and RAM usage in tasmota

I can confirm that there is a LOT a useless code in that library that you will never be used, starting with all the debug code full of Serial.print : 😱 This is totally forbidden in tasmota as the serial could be in use for something else.

I'm trying a first cut down

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

Working just on the ENS16x while both are enabled, I get

Without 
RAM:   [=====     ]  51.3% (used 42000 bytes from 81920 bytes)
Flash: [======    ]  63.1% (used 646340 bytes from 1023984 bytes)

With both enabled
RAM:   [=====     ]  52.9% (used 43376 bytes from 81920 bytes)          + 1376
Flash: [======    ]  63.8% (used 653108 bytes from 1023984 bytes)       + 6768

Cut down on ENS16x
RAM:   [=====     ]  52.7% (used 43136 bytes from 81920 bytes)          + 1136 => gain 240 bytes
Flash: [======    ]  63.7% (used 652204 bytes from 1023984 bytes)       + 5864 => gain 904 bytes

So gaining 940 bytes on announced 3100 bytes cost (from your own comments) is not neglictable
I believe that reworking the I2C access could probably allow to reach the 1KB of gain

I also see a while loop with an delay(10) inside. That should be totally forbidden on a shared resource system like Tasmota.

I understand who you are and totally respect your will to provide an official library to your customers that support maximum of use cases. That work fine for someone who build a a product with a handfull of given sensors with a custom built firmware for that product.
But here we are trying to have a general purpose software that can handle as many features that can collaborate seamlessly together.

Let me now have a look to the ENS210

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

I've seen ENS160 modules at Mouser from Sparkfun and DFrobot but none for ENS210
I've also seen combined modules with ENS160+ENS210 at AliExpress: originals or clones?

@chrfriese123
Copy link
Contributor Author

The size cutdown is impressive. The serial.print is indeed a left over from the initial phase.
The while loop with delay is implemented to wait for new available data. If the read out cycle off longer than 1 sec, the register content can be read directly.
Also everything related to reading raw values is not required for everyday users and can be removed. The same is valid for defining the customer mode. These functions are not needed in a standard environment.
For the ENS210 I don’t know the code so well but there might be options to reduce the code as well.
(Btw. this is a completely private activity from my side. I use Tasmota in my own devices to operate lights, plugs & covers and at the same time monitor the room air quality. Of course I have a sensor preference due to my professional backgrounds :-).)

@chrfriese123
Copy link
Contributor Author

@barbudor I haven’t seen a combined eval kit at Aliexpress. Only one with a third party RHT sensor.
The ones you find on Mouser from Sciosense are the official ones (white I2C to USB bridge + sensor board). But IMHO too expensive. Mainly because it’s not considered as end user product.
every other eval kit is a clone but not necessarily bad.

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

When I was saying "clone" I wasn't talking about hte module/eval-kit. I don't consider Adafruit/Sparkfun/DFRobot to do clones as far as they use legit ScioScience parts.
But this one https://www.aliexpress.com/item/1005004399782807.html is advertised to use ENS160 and ENS210. Indeed there is a lot of ENS160 + AHT, so may be this one is also lying about the ENS210

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

Lastest cuts on both libs is now

Final Cut
RAM:   [=====     ]  51.7% (used 42344 bytes from 81920 bytes)          +  344 => total gain 1032 bytes
Flash: [======    ]  63.5% (used 650188 bytes from 1023984 bytes)       + 3848 => total gain 2920 bytes

We are almost at 50% saving in flash and 75% saing in RAM

Please have a look at chrfriese123#1
This is how I suggest to introduce 2 defines per lib which allows to cut down the code
You could leave them commented for in your official driver for non-breaking, and uncomment them when used in Tasmota
May be some customers of yours could also find valuable the savings

So why is that so huge and also impact the RAM ?
You have a lot of Serial.print() for debug with strings
That is using quite some flash for the code, but also for the strings.
But why over 1KB of saving in RAM ?
When you have a code like:

Serial.println("Hello world!");

The string is saved in flash but at boot time it is copied to the RAM because the ESP are an modified Harward architecture and you need different instructions to read RAM or Flash. So to simplify the code, all constants are loaded from Flash to RAM.
But in Tasmota we want to save that precious RAM.
The compiler offer options such as the PROGMEM qualifier or PSTR(".....") so that constants including strings remains in Flash. In that case, the code using them must be able to distinguish between Flash and RAM addresse and uses the appropriate instructions.

I'll have a look at the Tasmora code now

@barbudor
Copy link
Contributor

barbudor commented Sep 9, 2023

2 hours later:

RAM:   [=====     ]  51.3% (used 42052 bytes from 81920 bytes)          +   52 => final gain 1324 bytes
Flash: [======    ]  63.5% (used 649916 bytes from 1023984 bytes)       + 3576 => final gain 3182 bytes

I optimized the Tasmota code:

  • moving to dynamically allocated memory saved a lot
  • de-duplicating the code with indexed data

As you can see, spending a little bit of time helps a lot and leave more room for other stuff.
I don't pretend that all drivers are at that level, and I don't pretend it wouldn't possible to go further on this one.

STILL: I am unable to test the changes*, especially the latests ones which may impact more the logic.
So please test what I have pushed to your fork or available from my branch here: https://github.com/barbudor/Tasmota/tree/barbudor_optimize_take1

I hope you will not take offense for my razor cut and will accept the changes

Thanks

image

* if ScioSense has some budget for supporting open source projects with a little eval board ... 👀

@chrfriese123
Copy link
Contributor Author

Thanks, for the cut down and no offence taken. I'll compile the code and run a physical test.

* send you a mail

@Jason2866
Copy link
Collaborator

@chrfriese123 Will you implement the suggestions?

@Jason2866 Jason2866 added the awaiting feedback Action - Waiting for response or more information label Sep 22, 2023
@barbudor
Copy link
Contributor

@Jason2866 I have already done the changes and will test this week end as I have received samples

@Jason2866
Copy link
Collaborator

@barbudor No hurry, just asking if there is activity done or planned.

@barbudor
Copy link
Contributor

Working so far except for a bug ENS16X's WebDisplay that is crashing the ESP
I've pushed the change to @chrfriese123 repo. As soon as he will merge we are good to go
chrfriese123@adc9902

@barbudor
Copy link
Contributor

Arg, detection of alternate addresses still have a problem

@barbudor
Copy link
Contributor

All good now

image

14:33:59.211 RSL: SENSOR = {"Time":"2023-09-23T14:33:59","EN16X_1":{"AQIS":8,"eCO2":400,"TVOC":0},"EN16X_2":{"AQIS":86,"eCO2":590,"TVOC":61},"EN210":{"Temperature":23.4,"Humidity":54.7,"DewPoint":13.7},"TempUnit":"C"}

ENS16X: +1984 Flash bytes, +24 RAM bytes
ENS210: +1740 Flash bytes, +24 RAM bytes
Strangely when both are enabled, the increase is lower than the sum of both separately
No idea where that additional optimization comes from as there is no common variables or code between both
ENS16X + ENS210: +3527 Flash bytes (vs 3724 expected), +28 RAM bytes (vs 56 expected)

@barbudor
Copy link
Contributor

@Jason2866 Still need @chrfriese123 to merge my changes in his repo to see all changes here

@chrfriese123
Copy link
Contributor Author

@Jason2866, @barbudor the last changes have been merged in my repo. Thanks for the great support in fixing the code.

@Jason2866 Jason2866 removed the awaiting feedback Action - Waiting for response or more information label Sep 24, 2023
@arendst arendst merged commit 5d97a73 into arendst:development Sep 24, 2023
63 checks passed
dilyanpalauzov pushed a commit to dilyanpalauzov/Tasmota that referenced this pull request Oct 1, 2023
* Add files via upload

Added ENS16x library enabling read-out of ENS160 and ENS161 sensor component (follow-up of CCS811 and iAQcore)
Added ENS210 library to read out ENS210 temperature & humidity sensor

* Add files via upload

Add air quality sensor readout for ENS160 and ENS161 checking two possible I2C addresses (follow up sensor for CCS811 and iAQcore)
Add temperature and humidity sensor readout checking two possible I2C addresses

* Update BUILDS.md

Add USE_ENS16x and USE_ENS210

* Update decode-status.py

Add USE_ENS16x and ENS210

* Update I2CDEVICES.md

Add USE_ENS16x and USE_ENS210

* Update my_user_config.h

Add USE_ENS16x and USE_ENS210

* Update support_features.ino

Add USE_ENS16x and USE_ENS210

* Update tasmota_configurations.h

Add USE_ENS16x and ENS210

* Update tasmota_configurations_ESP32.h

Add USE_ENS16x and USE_ENS210

* Update xsns_111_ens16x.ino

Corrected I2X number

* Update xsns_112_ens210.ino

Corrected I2C number

* Disable USE_ENS16x and USE_ENS210 by default

* Added code size information

* cut down in libs

* optimize tasmota side

* fix ens16x web display

* final fix on alternate addresses

* update code & RAM usage

---------

Co-authored-by: Barbudor <barbudor@barbudor.net>
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.

4 participants