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.

Sign up for GitHub

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

Jump to bottom

Massive number of "sensors" #57

Open
rsnodgrass opened this issue Mar 14, 2022 · 12 comments
Open

Massive number of "sensors" #57

rsnodgrass opened this issue Mar 14, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@rsnodgrass
Copy link

Description

Thanks for creating such a SIMPLE integration...and for making something that is device-type specific for the Protects instead of a generate "Nest" integration.

When installed, this seems to create a large number of sensors, rather than the key sensors necessary for 95%+ of use cases along with attributes added to devices and/or entities for the remainder. This requires disabling and/or adding recorder YAML config options to filter out sensors to not impact performance on HA tracking and recording so many sensors (especially on a RPi 3).

It would be more in line with other integrations that many of the "sensors" (like the color of light) actually be attributes of the device rather than independent sensors. One can always easily break out attributes into sensors if needed (and in many cases, just access attributes directly).

Device (optional)

No response

Additional information

No response
@rsnodgrass rsnodgrass added the enhancement New feature or request label Mar 14, 2022
@iMicknl
Copy link
Owner

iMicknl commented Mar 14, 2022

Thanks for reporting! I understand your issue and I have asked for feedback on the community forum on this topic, where an user responded that all sensors by default would be a good approach. I am open to more opinions, to make a definitive choice:

  1. Disable (especially diagnostic) entities by default, and let users enable specific ones they want to use.
    or
  2. All sensors enabled by default and better document how to disable default entities or how to disable tracking for the recorder. This is a one-time task.

Since I follow the Home Assistant contributing guidelines, everything is exposed as a separate entity. Use of attributes is not advised and/or allowed for most of the data we expose. I wonder if these diagnostic entities have a big performance impact on the recorder / HA core, since the state almost never changes.

@rsnodgrass
Copy link
Author

Auto create key sensors:

Rename to binary_sensor.next_protect_hallway as the PRIMARY entity...as this is Protect is all about smoke status first and foremost....plus all attributes that don't have sensors should be under this primary entity.

binary_sensor.nest_protect_hallway_smoke_status

...and the following auto created sensors/switches

binary_sensor.nest_protect_hallway_battery_health (this is critical life saving, if battery dies not good!)
binary_sensor.nest_protect_hallway_occupancy
binary_sensor.nest_protect_hallway_temperature
binary_sensor.nest_protect_hallway_humidity

sensor.nest_protect_hallway_led (attributes below):
  - brightness
  - color

Auto create switches:

switch.nest_protect_hallway_heads_up
switch.nest_protect_hallway_nightly_promise
switch.nest_protect_hallway_pathlight
switch.nest_protect_hallway_steam_check

AND THE REST OF THESE as the attributes on the main device/entity:

binary_sensor.nest_protect_hallway_buzzer_test
binary_sensor.nest_protect_hallway_co_status
binary_sensor.nest_protect_hallway_co_test
binary_sensor.nest_protect_hallway_heat_status
binary_sensor.nest_protect_hallway_heat_test
binary_sensor.nest_protect_hallway_humidity_test
binary_sensor.nest_protect_hallway_led_test
binary_sensor.nest_protect_hallway_line_power
binary_sensor.nest_protect_hallway_occupancy
binary_sensor.nest_protect_hallway_pir_test
binary_sensor.nest_protect_hallway_smoke_test
binary_sensor.nest_protect_hallway_speaker_test
binary_sensor.nest_protect_hallway_wifi_test
sensor.nest_protect_hallway_replace_by

@rsnodgrass
Copy link
Author

This allows leaving all the diagnostic "sensors" since they just become attributes and you don't need to have a flag to specific include/exclude diagnostic.

@operinko
Copy link

I would prefer to have the diagnostic sensors as sensors, just disabled by default.

@rsnodgrass
Copy link
Author

@operinko I think that is a good idea as well, easy just to have one config to enable all the diagnostic sensors.

@Daneish
Copy link

Daneish commented Mar 15, 2022

2. All sensors enabled by default and better document how to disable default entities or how to disable tracking for the recorder. This is a one-time task.

I like option 2 better. Having all sensors exposed by default provides users more information on the safety of their device that they may never see if they are disabled by default. This allows users to more easily create relevant automations to act upon any safety issues.

I agree that data should be exposed as individual entities rather than attributes of a master entity, unless it fits into a entity type that uses attributes, such as the light entity rsnodgrass mentioned. However with the current controls I’m not sure they do fit into the light type, as the light on my Protect can’t be turned on and off with a service (at least not on my battery version).

@operinko
Copy link

Maybe have an "Enable all diagnostic sensors" checkbox during initial setup steps?

@iMicknl
Copy link
Owner

iMicknl commented Mar 15, 2022

This allows leaving all the diagnostic "sensors" since they just become attributes and you don't need to have a flag to specific include/exclude diagnostic.

I will keep them as sensors since to align to the Home Assistant best practices. This is why they added the configuration and diagnostic categories to core, so they don't show up in your lovelace dashboards by default.

Thanks everyone for sharing opinions on what would be a good approach! Let's see what we can do the coming time to make this a better experience for new users. I will see if someone from core can chime in as well, since I eventually plan to bring this to core.

@mdegat01
Copy link

mdegat01 commented Mar 25, 2022 •
edited
Loading

Thanks so much for this integration! Was really missing my Nest protects in HA. I was a bit surprised by the number of sensors, I think it might be a good idea to have the diagnostic entities disabled by default. I think more users will want them disabled then enabled personally. Although a setting in the config flow does sound like the best of both worlds.

This requires disabling and/or adding recorder YAML config options to filter out sensors to not impact performance on HA tracking and recording so many sensors (especially on a RPi 3).

Just as an aside, making these attributes would make this problem worse not better. HA records a state change in the database anytime any aspect of an entity changes, state or attributes. And that state change event includes the state and all the attributes of the entity.

So if these became attributes then you would be recording much more data. Because any time the sensor with these attributes changed (one of its attributes or the state value) it would record a state changed event with a copy of all the current diagnostic values. And now there would be no way to filter anything out since there's no option in recorder to filter out the value of an attribute of an entity.

Making these individual sensors is definitely the right approach, there's a reason that's the current best practice. But I do agree it would be good to have a way to start with all the diagnostic entities disabled.

@mdegat01
Copy link

mdegat01 commented Mar 25, 2022 •
edited
Loading

Another possible option here is to publish one more sensor which is "Test Status". It goes on when any test failed and off otherwise. And then have all the sensors for the individual tests disabled by default.

I know you can make this sensor easily enough as a binary sensor group but then you have to leave all the other ones enabled. If there was s test roll up sensor then you could just leave that one enabled and send a notification to yourself like "hey something is wrong with this protect, go open the app and check it out".

Like looking closely at this list I don't really mind any of the others personally.

  1. Primary entities - obviously useful, they're the reason we're here
  2. Config entities - handy. Could disable them if you prefer to manage that via the nest app for some reason, I'm happy for one less reason to open it.
  3. Battery and expiration date - Great, want both of those. Battery is critical and I'm never going to remember that date without help.
  4. Test results - If something broke I want a notification. It doesn't need to tell me specifically what happened, I'll open the app and figure it out. Or I'll re-enable the individual test sensors temporarily.

@mdegat01
Copy link

Small update - might want to also disable the config entities by default. And possibly put a warning about them in the readme. I tried to change one of them and nothing seemed to really happen. So I went in the nest app and changed the same and got a pop up warning like "this may take up to 24 hours" to take effect.

A 24 hour lag time kind of rules out any automation use case for them. You could show them in a dashboard or something I guess but really I don't think anyone is going to change them much given that. They aren't quite as useful as I originally thought and likely to create confusion for users when they change a value and nothing seems to happen.

@jeffdeal
Copy link

jeffdeal commented Apr 18, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@iMicknl @operinko @mdegat01 @rsnodgrass @Daneish @jeffdeal