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

[P167] Enable use of SEN5x sensor, other improvements #5038

Conversation

tonhuisman
Copy link
Contributor

@tonhuisman tonhuisman commented Apr 20, 2024

Resolves #4794 phase 2: Stand-alone SEN5x support

Features:

  • Enable use of Sensirion SEN5x standalone sensor
  • Added plugin to Climate and MAX builds
  • Formatted source using Uncrustify
  • Clean up code
    • Use existing dew point calculation
    • Remove unused variables and code
    • Fix issue with status bits
    • String handling improved
    • Enable multi-instance use (requires I2C multiplexer because of fixed I2C Address)
    • Use enum classes where applicable
  • Add support for sen5x,startclean command to clean the fan
  • Add support for sen5x,techlog,<0|1> command to disable/enable the Technical logging option

TODO:

  • Testing standalone sensor (tested locally) (confirmed)
  • Testing when used with Ikea Vindstyrka (confirmed)
  • Add documentation

@tonhuisman
Copy link
Contributor Author

@saint-hh You might be interested in testing this 😉 please report your findings here.

@saint-hh
Copy link

Gorgeous, will do!

@saint-hh
Copy link

saint-hh commented Apr 23, 2024

Updated to the latest build this morning, running fine on two devices with the SEN55. The measured values are pretty similar / parallel with both sensors / units.
NOx seems also fine, I saw values up to 3 µg/m³, so it's not sticking to 1 and our air seems to be pretty good in this matter.
Only one thing is interesting: the load is permanently quite high on both devices (Unit 6+7):
Bildschirmfoto 2024-04-23 um 21 42 34

IMG_3238

@tonhuisman
Copy link
Contributor Author

Only one thing is interesting: the load is permanently quite high on both devices (Unit 6+7):

On ESP32 I usually see a higher load then on ESP8266, that partially depends on what plugins are active, but is nothing to worry about, as long as it's not constantly near 100%.

@andibaciu
Copy link
Contributor

I'm very interested how system will work with 2 SEN5x devices on the same i2c bus using a i2c multiplexer, some print screens with Devices page will be very interesting.
Good job @tonhuisman !
Thank you @saint-hh for testing and printscreens (The system load is around 25% - 30% on esp8266 too)

@tonhuisman
Copy link
Contributor Author

I'm very interested how system will work with 2 SEN5x devices on the same i2c bus using a i2c multiplexer, some print screens with Devices page will be very interesting.

@andibaciu Have you tested this code on an actual Ikea Vindstyrka device? As I don't own such device, I can't verify myself if it still works as intended.
Important thing here is, you should then not configure multiple tasks with the extra output values, but retrieve those values from the single task by using rules (value names are exactly the same, just the taskname changes). Optionally storing them in a Dummy Device, to automagically send them out via a Controller, if desired.

@andibaciu
Copy link
Contributor

I'll test this version on IKEA Vindstyrka later today (I saw this post with modification/improvements a few hours ago), but give me a hint about configuring second task .... i understand there are 2 methods ....

@tonhuisman
Copy link
Contributor Author

I'll test this version on IKEA Vindstyrka later today (I saw this post with modification/improvements a few hours ago), but give me a hint about configuring second task .... i understand there are 2 methods ....

Well, there is actually just 1 method: Configure exactly 1 task per device.
To get the remaining 5 values (or 4 when using the Ikea Vindstyrka or a Sensirion SEN54), you can simply use the matching variables, either from rules or from a display configuration (like saint-hh does), these values, though not directly visible, are available for use by what we call the 'Get Config Value' plugin function (originally added to get plugin configuration settings), that will be called when a requested value can not be found in the list of (max) 4 visible values.
Additionally, if you want or need the other values to be visible from the web-UI, you can configure a Dummy Device, and use a rule to populate the values (again, max. 4) of that Dummy, using the TaskValueSet command.

In your specific situation, it might be best to disable the existing tasks, and configure a new task with this version of the plugin, or your configuration might get lost (unless you backup your config, to later restore when returning to your current ESPEasy version).

…feature/P167-add-support-for-standalone-sen5x-sensor
@andibaciu
Copy link
Contributor

andibaciu commented Apr 30, 2024

Check with IKEA Vindstyrka ... everything look good ...

image
image

and system load is less then 10% ... (esp8266)

I found one problem with plugin: Technical logging CHECK button don't work .... not remain in check state when press Submit.

@tonhuisman
Copy link
Contributor Author

I found one problem with plugin: Technical logging CHECK button don't work .... not remain in check state when press Submit.

Hmm, that seems to be a recurring issue I have with these checkboxes (completely my fault, usually 😊), I'll fix that soon.

@andibaciu
Copy link
Contributor

@tonhuisman , For me still don't work Technical logging ... when i press "Submit" check state disapear ...

…feature/P167-add-support-for-standalone-sen5x-sensor
@tonhuisman
Copy link
Contributor Author

@tonhuisman , For me still don't work Technical logging ... when i press "Submit" check state disapear ...

I've installed the ESP8266 Climate build on a Wemos D1 clone, and the checkbox for Technical logging is working here as intended (was testing a MAX build before, and ESP32 is sometime a little bit more forgiving in this area).
Are you also using the Climate build, or are you compiling a Custom build? (AFAIR, you're on ESP8266 ? )

@andibaciu
Copy link
Contributor

I'm on ESP8266 ...
normal_ESP8266_4M1M
(I add in define_plugin_sets.h in #ifdef PLUGIN_SET_STABLE , #define USES_P167 // IKEA Vindstyrka

image

@andibaciu
Copy link
Contributor

@tonhuisman if i change this lines:

222 //addFormCheckBox(F("Technical logging"), P167_ENABLE_LOG_LABEL, P167_ENABLE_LOG);
addFormCheckBox(F("Technical logging"), F("technicallog"), P167_ENABLE_LOG);

236 //P167_ENABLE_LOG = isFormItemChecked(P167_ENABLE_LOG_LABEL);
P167_ENABLE_LOG = isFormItemChecked(F("technicallog"));

262 Plugin_167_SEN->setLogging(P167_ENABLE_LOG);

everithing work ok with check button.

@tonhuisman
Copy link
Contributor Author

P167_ENABLE_LOG = isFormItemChecked(F("technicallog"));

Can't explain it (yet) but using PCONFIG_LABEL() has worked 'weirdly' in the past so that might explain the behavior you had. But haven't seen it here, this time.
Replacing use of that label function by fixed F() strings should solve this. And also reverted the == 1 stuff, as that shouldn't be needed.

@tonhuisman
Copy link
Contributor Author

tonhuisman commented May 2, 2024

@andibaciu Are you (or where you) using the sourcecode of an older release of ESPEasy, with additionally the files for this plugin?
If so: when developing for ESPEasy, it is strongly advised to work with latest mega branch sources (as described in the PlatformIO/Development guide page in the documentation), that should avoid unexpected behavior like we saw now. Especially as we are working on the latest mega code by default.
If you are working on latest mega sources, then I'm really baffled why this happened 🤔

@andibaciu
Copy link
Contributor

@tonhuisman i work with this release "Changes in release mega-20240229 (since mega-20231225)" ... before i make my firts commit for P167 ....
with your last commit everything is ok with check button.

@TD-er
Copy link
Member

TD-er commented May 2, 2024

Yep back then it wasn't fixed as I did fix it about a month ago in the ESPEasy core files.

@andibaciu
Copy link
Contributor

andibaciu commented May 2, 2024

I apologize for the mistake
I have to learn to update working code with the latest release as often as possible.

Now i update my local version of mega to last version and everything is ok with check button with first version of files.

…feature/P167-add-support-for-standalone-sen5x-sensor
@saint-hh
Copy link

saint-hh commented May 7, 2024

Hi Ton,
just installed the latest build from 5th. Everything seems fine with the SEN55 but somehow my screen gets messed up. Text positions are shifted and background colour changed (nothing changed in the rules):
IMG_3282

In comparison with the not yet updated device, how it is supposed to be:
IMG_3281

Any idea where this comes from? I guess not related to this plugin, but maybe some updates to the Display - ST77xx TFT plugin, being made meanwhile?

@tonhuisman
Copy link
Contributor Author

I've found a typo in the definition of the default font, where the width and height are swapped 😱 I've fixed that in another PR that I expect to be merged soon, but I'll cherry pick it to this PR as well.
What exact font do you use here? Maybe there's another typo there, when I moved the code.

The (AFAICS) unexpected background color change may be related to an issue with the background color not working as expected, I fixed a couple of months ago in the same overhaul of the AdafruitGFX_Helper module that is used for most display plugins, but I'll have to investigate what the exact cause here is.
A workaround should be to use an explicit color for background (when writing the text) as I suspect the configured background color (in the plugin settings) is used when not specified.

The offset in positioning is probably also related to bugs I fixed when working on that helper module, some fonts used a different 0-point than others, AFAIR.

@saint-hh
Copy link

saint-hh commented May 7, 2024

I've found a typo in the definition of the default font, where the width and height are swapped 😱 I

Ah - that might also explain why I had to sort the positions with trial&error and couldn't really sort it out logically. Somehow position of text and images, boxes etc were not logically matching in numbers to me.

What exact font do you use here? Maybe there's another typo there, when I moved the code.

For the time in the upper right corner I use robotomono8pt, for the rest I use amerikasans16pt

@tonhuisman
Copy link
Contributor Author

Have you already shared your rules files somewhere? I can't find them in the forum threads.
Possibly I've fixed the bugs in the code, but didn't apply the corresponding corrections to the fonts table 🤔

@saint-hh
Copy link

saint-hh commented May 7, 2024

Yes, here you go:
https://www.letscontrolit.com/forum/viewtopic.php?t=10076

It changed a little bit, but basically this is it.

@tonhuisman
Copy link
Contributor Author

A workaround should be to use an explicit color for background (when writing the text) as I suspect the configured background color (in the plugin settings) is used when not specified.

Additionally: When using the same color for background and foreground, results in the background being transparent, that should get rid of the current dark-blue background.

The positioning of the text depends on the font definition (some fonts are offset vertically by half or full font-height 😱), I'm quite sure it is now working as originally intended, and as you stated it was quite fiddly to get it right, most likely it will start to work correct once I push my fixes (did a lot of testing last night). This may require you to adjust your rules slightly, as the coordinates will match with coordinates for lines, images etc.

I corrected a few of the font definitions. Will make a commit in the other PR, and cherry pick that here too (the small fix I mentioned yesterday isn't yet pushed, but also won't make a difference here).

@saint-hh
Copy link

saint-hh commented May 8, 2024

This may require you to adjust your rules slightly, as the coordinates will match with coordinates for lines, images etc.

That sounds good - thank you!

most likely it will start to work correct once I push my fixes (did a lot of testing last night).

Should we, in order to keep it clean (no SEN5X plugin problem), continue in the related PR? Which one is it?
Or should we continue here?

I've flashed the device for my wife's office back to the 22.04. build. She took it with her today. But I can test and adjust on my device home.

@tonhuisman
Copy link
Contributor Author

Should we, in order to keep it clean (no SEN5X plugin problem), continue in the related PR? Which one is it?

I can't cherry-pick the changes here without mixing/fighting in a lot of changes from 'the other PR', so we'd better continue this 'over there' in #4016

@tonhuisman
Copy link
Contributor Author

I'm very interested how system will work with 2 SEN5x devices on the same i2c bus using a i2c multiplexer, some print screens with Devices page will be very interesting.

@andibaciu It seems I kind of ignored your question here, but that's not intentional. We have extensive documentation on how to configure an I2C multiplexer in ESPEasy, here.
When using devices with the same I2C address, each should go on a separate channel of the multiplexer. And each channel should get their own set of pull-up resistors (also the I2C connection from the ESP to the multiplexer!). Simplest and cheapest is to get the TCA9548a 8-channel multiplexer, as on Ali a 4-channel TCA9546a is double the price 😱 (~ $2,50 vs $1.25, and 2 channel TCA9543a is around $3.50), except when mounting space is problematic.

Having multiple SEN5x sensors on the same ESP isn't the best choice, as I2C wiring should be short (max 30 cm), and measuring air quality roughly half a meter apart with 2 of these sensors isn't going to add much value... but it will work, if only to compare measurements 😄 but you could install 1 in two separate, adjacent, rooms with the ESP in between 👍

@andibaciu
Copy link
Contributor

@tonhuisman - if there are 2 SEN5x devices attached to an i2c multiplexer, I would like to know how many tasks must be declared - one for each device?

@tonhuisman
Copy link
Contributor Author

@tonhuisman - if there are 2 SEN5x devices attached to an i2c multiplexer, I would like to know how many tasks must be declared - one for each device?

Yes, from ESPEasy perspective they are 2 independent hardware devices, that need their own separate configuration.

@TD-er TD-er merged commit c4102c0 into letscontrolit:mega May 29, 2024
171 checks passed
@tonhuisman tonhuisman deleted the feature/P167-add-support-for-standalone-sen5x-sensor branch May 29, 2024 21:33
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.

[FR] Add Sensirion SEN5x (IKEA VINDSTYRKA)
4 participants