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

Plugin P167 IKEA Vindstyrka #4991

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Plugin P167 IKEA Vindstyrka #4991

merged 4 commits into from
Apr 15, 2024

Conversation

andibaciu
Copy link
Contributor

@andibaciu andibaciu commented Feb 27, 2024

In the file attached you have all description you need.
ESPeasy plugin for IKEA VINDSTYRKA.pdf

Resolves #4794

@tonhuisman
Copy link
Contributor

@andibaciu If you add the text Resolves #4794 in your comment (just as plain text), at the start of a new line, this PR will be linked to the issue, and will auto-close the issue once the PR is merged.

@TD-er
Copy link
Member

TD-er commented Feb 28, 2024

Resolves #4794

In the start post of the pull request :)

const uint8_t i2cAddressValues[] = { P167_I2C_ADDRESS_DFLT };
if (function == PLUGIN_WEBFORM_SHOW_I2C_PARAMS)
{
if (P167_SEN_FIRST == event->TaskIndex) // If first SEN, serial config available
Copy link
Member

Choose a reason for hiding this comment

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

Why so complex with checking the first taskindex?
This is prone to cause a lot of issues and should only be done with a very specific and very good reason.

Also below with PLUGIN_INIT you're doing stuff different if it is not the first task.

I will explain in a separate post why this is a bad idea.

Copy link
Contributor Author

@andibaciu andibaciu Mar 3, 2024

Choose a reason for hiding this comment

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

There are so many reason for this type of implementation:

  1. All SEN5x devices are the same i2c address 0x69 .... so this limitation permit ONLY one device on the bus
  2. for IKEA Vindstyrka it's necessary to minimize reading pull i2c communication (because multimaster problems) and in this case ONLY for first task i activate pin interruption monitoring SCL i2c communication and read ALL parameters from SEN5x. If you want to read more then 4 parameters from the same device you need to ad another one with different name and get from first task requested parameters (without activate another interrupt and make another pull i2c request on the bus)
  3. for what reason somebody will add more then 2 task plugin and after that will disable the first one ???
  4. i see this kind of implementation on other plugins (it's not my creation)

Thank You!

Copy link
Contributor

@tonhuisman tonhuisman Mar 3, 2024

Choose a reason for hiding this comment

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

2. If you want to read more then 4 parameters from the same device you need to ad another one

That is not necessary, you can implement the PLUGIN_GET_CONFIG_VALUE function, that allows to fetch as many values from a plugin as needed, as demonstrated in many plugins (search the source code via VSCode to find examples), from very simple implementations like P021 (Level Control), to 'tricky' ones like P137 (AXP192) and somewhat complicated implementations like P095/P116/P131/P141 (Displays using AdafruitGFX_helper).

This PLUGIN_GET_CONFIG_VALUE functions is used exactly like the regular [<taskname>#<valuename>] variables in rules.

For sending data via a Controller you can use as many Dummy Device plugins as desired, that receive their data via rules (TaskValueSet/TaskValueSetAndRun) from that 1 P167 instance, or use the Publish and SendToHTTP families of commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @tonhuisman for this short description of PLUGIN_GET_CONFIG_VALUE.
For further plugin i'll keep in mind to use this.

Copy link
Member

Choose a reason for hiding this comment

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

You can technically use multiple I2C devices on the same I2C address, by using I2C multiplexers.

There is a number of devices where you may get multiple values at once, without asking for them, like GPS.
For those you may need a single GPS string parser instance which can be used by multiple tasks to get over 4 values.
If you need to specifically trigger a measurement and later collect its reading, then it also makes sense to have some instance running which can handle the interactions with the sensor.
But this should not be done in the ino file.
It is best to have a shared pointer which is a member of the PluginStructs.

If you try to do this manually in the plugin's .ino file, you will override a lot of ESPEasy-core code and -checks which will make this really hard to debug and to maintain as it is working completely different from all other plugins.

Maybe you can take a look at the Eastron plugin as this one is using a separate class to take in requests for registers to read from various tasks and returns the results to the appropriate task values.

The 1-wire plugin (P004) is also doing something special as it needs to combine several concurrent requests while using the same GPIO pin for all connected 1-wire sensors.

The GPS plugin deals with those "free" extra values by adding some "virtual" taskvalues (PMSx003 does the same) as all other values which are received anyway are made available via standard taskvalues even though those were never defined as task value output.
What is set as task value will be sent to any connected controller.
All others may be accessible via rules so you can copy those to a dummy task which can then send it to a controller if needed.

About the pin description....
Every I2C plugin/task is using the same GPIO pins for I2C.
If that needs to be dealt with, then this should be done in ESPEasy core code, not in the plugin code.
But for now, do not try to work around checks etc. which makes the plugin "special" as it is an absolute nightmare to maintain it.
There are enough examples of plugins where you need to read way more than 4 values from the same sensor, so please follow those instead of re-inventing the wheel.
I have spent a lot of time in making the plugins act more uniform, so please help keep the number of variants to a minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK @TD-er , I understand and I try to remake plugin construction to meet standard plugin requirements.

@TD-er
Copy link
Member

TD-er commented Mar 3, 2024

You're making it really complex by adding checks for being the first task of this type.

So why do you need this?

One example where this will fail:

  • Add task 1, 2, 3 with this plugin and enable all
  • Disable task 1

This will not call PLUGIN_INIT for any of the remaining tasks thus as a result it will no longer work until you either reboot or call taskdisable,2 followed by taskenable,2.

And there are probably other issues too.

@saint-hh
Copy link

I have 2 SEN55 here, ready to be connected to 2 WT32-SC01, currently running on ESP_Easy_mega_20240203_max_ESP32_16M1M Feb 3 2024.
Please let me know, if I can support you with testing.
Here is my forum thread

@TD-er TD-er merged commit 00a68a7 into letscontrolit:mega Apr 15, 2024
170 checks passed
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