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

Flow setup pulls incorrect variables from combined sensors into incorrect windows #121

Closed
convicte opened this issue Feb 1, 2022 · 18 comments

Comments

@convicte
Copy link

convicte commented Feb 1, 2022

Trying to set up the Thermal Comfort using the newly added GUI flow will produce the following table:
image

As you can see, the sensors for my Xiaomi Zigbee outside unit will pull RH value into the temperature sensor selection window, while it pulls temperature into the humidity variable.

image
image

It would be entirely OK if it doesn't differentiate between RH and temperature and the user must choose the appropriate sensor, but it's actually only allowing one of them in each, and the incorrect one at that.

Second, for most of my sensors it was able to correctly allocate the temperature and RH to the appropriate selection windows, so I am not sure what is wrong with this one.

@convicte
Copy link
Author

convicte commented Feb 1, 2022

If there is a chance to improve it further, to exclude completely unrelated sensors from the selection window, to minimize the need for sifting through them (e.g. battery or phone link quality), that would probably tie well into this issue.

If you disagree, I can put another ticket up, as an improvement idea.

@rautesamtr
Copy link
Collaborator

Hi thank you for your report.

Regarding the wrong sensors could you make a screenshot of the state of them in developer tools?

Regarding available sensor selection. With advanced mode disabled you should only get sensors with the correct sensor device class and correct units of measurements. With advanced mode we don't filter out entities without device class or unit of measurement. But we filter those out that have the explicit wrong type of unit of measurement, domain or device class. The reason for this is, that not all sensor integrations support device class e.g. command_line or input_number and we want advanced users to still be able to select them. If you have any unit of measurement, domain or device_class in mind that we additionally should exclude from the selection your can open an issue for that.

@convicte
Copy link
Author

convicte commented Feb 1, 2022

Thank you very much for the prompt response and a great integration I hope to be able to use!

Just for your understanding, it's this exact Xiaomi module integrated via Zigbee2mqtt.
All published values can be found here:
https://www.zigbee2mqtt.io/devices/WSDCGQ11LM.html
Please see the screenshot requested:
image

I am grateful for the explanation of the exclusion/inclusion principles at play.
In this case, it may be exactly the root cause of the issue, since while the state is clearly indicated, it seems more than one variable is reported in the attributes of this sensor, which may through the integration into a tailspin if it considers both state and variable when looking for the sensor type.

Finally, my apologies if it's there, but I was not able to find the advanced mode toggle anywhere, to allow a richer or more stripped down list of sensors included.

@rautesamtr
Copy link
Collaborator

@IATkachenko do you have any idea what could cause this?

@convicte
Copy link
Author

convicte commented Feb 1, 2022

Upon further consideration and reviewing the list it actually also including unrelated entities like switches and completely different class variables such as sun.sun.

image

Neither of these should be found under sensors in the list, unless I am misunderstanding your description of the exclusion principles.

@IATkachenko
Copy link
Contributor

IATkachenko commented Feb 1, 2022

In get_sensors_by_device_class() we allow "advanced" users select any type of sensor, but exclude obviously useless.
We have no filters by % unit of measurement for temperature sensor and Celsius for humidity. May be it is a good idea to have additional filter.

About switch filtering. Are you sure that nobody have switches for temperature state, for example? Like "day/night temperature switch". But it looks like switch should be excluded from list too, because it have state on or off, not a digital value.

@rautesamtr
Copy link
Collaborator

rautesamtr commented Feb 1, 2022

I just checked. Switch and sun domain are still missing from the exclusion list. Makes sense to add them.

@IATkachenko
Copy link
Contributor

Done.
Colleagues, what about climate domain? Should it be excluded too? Looks like thermal_comfort may get data from climate, but not now.

@rautesamtr
Copy link
Collaborator

Exclude it for now. That would require flow ui redesign. We can add it as feature in the future.

@convicte
Copy link
Author

convicte commented Feb 1, 2022

Thank you for the swift action on the excessive list of modules!

What remains an issue is the main one in this ticket, which is that the RH is presented in the temp sensor, but not a temp sensor for the same device, and vice versa.

I don't believe the changes in question would do anything for that.

Just to follow-up, how would one enable or disable the advanced mode, to limit the sensors available?

@IATkachenko
Copy link
Contributor

Just to follow-up, how would one enable or disable the advanced mode, to limit the sensors available?

Click at user avatar at bottom-left conner in UI.

@IATkachenko
Copy link
Contributor

What abut RH? What is wrong now, in master version of the integration, after #125?

@rautesamtr
Copy link
Collaborator

@IATkachenko for reference regarding climate usage for a similar integration https://github.com/Limych/ha-temperature-feels-like

@rautesamtr
Copy link
Collaborator

@convicte i released 1.4.3 that includes #125. Could you give it another try and compare the sensor lists between advanced and non advanced mode. I am not sure yet what causes the issue and will only have time later this week to dig deeper into the filters.

@convicte
Copy link
Author

convicte commented Feb 1, 2022

@rautesamtr I have now managed to add my first sensor combination successfully using the GUI under 1.4.3.

The number of sensors in Advanced mode shrunk down significantly, which is very appreciated.
I believe to have found a potential reason for my problems, which were due to a confusion rather than the RH/temp sensors truly missing. I know it sounds silly, but please bear with me as I explain.

The list of sensors/switches in 1.4.1 was very long for me (probably 200-300 entries) which required long scrolling to get to sensor.outside..... etc.
What I realized now is that the a-z order inside the dropdown menu restarts more than once (2-3 times?) and thus there are several sensor.o(...) sections, which probably means I skipped over the first one and ended up going to the next one where I found the sensor.outside.rh as opposed to temp which was in the first sensor.o(...) section up top.

I wonder if list restarting is due to their being different selection criterions for which sensors to include in the dropdown menu, and each is creating its own alphabetically ordered list?

@IATkachenko thank you for clarifying the advanced mode. I must admit having never toggled it off since I installed home assistant years ago and totally forgot about its existence.

I would reason however that using the global advanced/basic option from the general home assistant settings makes this very limiting. Likewise, I may be wrong, but I would think the vast majority of users are utilizing the advanced feature set (like dev tools), so an integration specific toggle for limiting the available sensors would be more convenient, but it's one man's opinion, for your consideration.

@rautesamtr
Copy link
Collaborator

The list of sensors/switches in 1.4.1 was very long for me (probably 200-300 entries) which required long scrolling to get to sensor.outside..... etc.
What I realized now is that the a-z order inside the dropdown menu restarts more than once (2-3 times?) and thus there are several sensor.o(...) sections, which probably means I skipped over the first one and ended up going to the next one where I found the sensor.outside.rh as opposed to temp which was in the first sensor.o(...) section up top.

Ok that's very long. I didn't know about the a-z order repeating several times, thats not intentional. It's only intended to restart once. We have to look into this. The idea behind restarting the a-z order was to keep the better matching candidates, basically the list you get in non advanced, still on top in the advanced mode so you would find them faster.

I would reason however that using the global advanced/basic option from the general home assistant settings makes this very limiting. Likewise, I may be wrong, but I would think the vast majority of users are utilizing the advanced feature set (like dev tools), so an integration specific toggle for limiting the available sensors would be more convenient, but it's one man's opinion, for your consideration.

I fully understand your concern and reasoning. We also discussed between us how to best handle the issue before implementing this, and what you got is mostly a compromise between opposing concerns. The main problem is that as a custom integration we are kind of restricted in the frontend features we can use. In an ideal world I would have preferred to add text fields with autosuggestions for the entities including their friendly name and icon similar to other parts of the frontend. Right now it seems we have a solution that is usable but neither ideal for the average user nor the power user. The advanced mode toggle isn't ideal but just how home assistant implements this feature for config flow. It would be nice if they added a simple button to switch the mode just for the config flow. The best I can think of would be to add an additional step before the actual sensor selection that asks you if you want to do the setup in advanced mode or not.

@convicte
Copy link
Author

convicte commented Feb 1, 2022

Thank you for all your detailed feedback and the look behind the developer's curtain.

  1. In my case, even if it restarts twice, with over 50-60 good matches and 200-300 bad once was enough for my dyslexic mind to fail to realize. ;)
  2. If there could be a differentiator between the lists, that could help (e.g.bold text)

Re:limitations: I understand and would think that potentially a context aware search would solve the issue all together, since differentiation would not be required. Though, from what you describe, this will also not work due to HA frontend.

@rautesamtr
Copy link
Collaborator

Since the original issue isn't valid anymore i opened #135 so it's possible to discuss better alternatives for the input sensor selection there.

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

No branches or pull requests

3 participants