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

Remove NUMERIC_DISCOVERY_LOOKUP mapping for current and voltage categ… #25322

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

2long1
Copy link

@2long1 2long1 commented Dec 25, 2024

…ory and enabled_by_default

This is for issue number #25319 I am not sure how I to link issue to PR's in github. This is only my second contribution. Let me know how I should do things in the future.

This is an optional change since it will likely have backward changing effects and for my purposes, the PR in the zigbee-herdsman-converters repo solves my immediate issue.

@Koenkk
Copy link
Owner

Koenkk commented Dec 26, 2024

Current and voltage are disabled by default since they are not that much used, you can always enable them by default later.

@2long1
Copy link
Author

2long1 commented Dec 26, 2024

Sorry, the commit message was split across the issue title and the first category. Did you see that the diagnostic category was also removed in this PR? Your comment only mentioned the enabled nature for current and voltage. There is an overlap between the category values and what is in the presets definition in zigbee-herdsman-converters/lib/exposes.ts. I had to make these changes since they undo my herdsman-converters change that allowed me to remove the diagnostic category.

Should I close this PR?

@Koenkk
Copy link
Owner

Koenkk commented Dec 28, 2024

They should be in diagnostics, this change was made by on the of the HA maintainers, see #9293

@2long1
Copy link
Author

2long1 commented Dec 28, 2024

Ok, I get that but, the change is duplicated across zigbee-herdsman-converters and zigbee2mqtt. See:
https://github.com/Koenkk/zigbee-herdsman-converters/blob/e14940310dafad1c9d6d3d7b8f42bb108dac34af/src/lib/exposes.ts#L971

Which sets the entity_category for current to diagnostic in the presets.

This is done again in zigbee2mqtt at line:

entity_category: 'diagnostic',

My main issue with these decisions is that it removes control over what is considered diagnostic versus primary from the end user. The better answer is something in homeassistant that would allow you to override the category on a device basis, but I have been unable to locate such a capability.

Until then, I will just run with my forked repository. Feel free to close this (and the zigbee-herdsman-converters) PR.

@Nerivec
Copy link
Collaborator

Nerivec commented Dec 28, 2024

https://developers.home-assistant.io/docs/core/entity/#registry-properties

Set to EntityCategory.CONFIG for an entity that allows changing the configuration of a device, for example, a switch entity, making it possible to turn the background illumination of a switch on and off. Set to EntityCategory.DIAGNOSTIC for an entity exposing some configuration parameter or diagnostics of a device but does not allow changing it, for example, a sensor showing RSSI or MAC address.

@2long1
Copy link
Author

2long1 commented Dec 28, 2024

@Nerivec Thank you for that reply, I did see the you could force an entity_category, but what I couldn't figure out how to do was REMOVE a category that the device maintainer set. E.g. There doesn't seem to be a way to make a "diagnostic" entity into a primary sensor. I think HA needs another category for "primary" sensors, rather than relying on the "absense" of a category to imply primary. Did I miss something?

In my case, I purchased an energy monitor, but he only sensor not marked as diagnostic is energy (kWh). But I bought it to monitor realtime power (kW). I get that they wanted to keep the overview dashboard from being littered with every entity a device provides, I am just looking for a little more control.

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.

3 participants