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

Fix entity category of Home Assistant exposed enum sensors #19474

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

frenck
Copy link
Contributor

@frenck frenck commented Oct 29, 2023

Similar #19434, but in this case, for the enums that are exposed as sensors.

Related to #19430

@hmmbob
Copy link
Contributor

hmmbob commented Oct 29, 2023

My errors all have power_outage_memory as suffix, those aren't touched in the pr. Is that expected?

@frenck
Copy link
Contributor Author

frenck commented Oct 29, 2023

@hmmbob
Copy link
Contributor

hmmbob commented Oct 29, 2023

Right, I see. Thanks, I'll test as soon as it's in dev


// If it has an entity category of config, but exposed as sensor, then change
// it to diagnostic. Sensors have no input, so can't be configured.
if (discoveryEntry.discovery_payload.entity_category === 'config') {
Copy link
Owner

@Koenkk Koenkk Oct 29, 2023

Choose a reason for hiding this comment

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

@frenck if I understand correctly the logic is: only if type == sensor and entity_category == config, then entity_category should be set to diagnostic. If true, then I propose to remove this and the other similar if and instead change the end of the function to:

        discoveryEntries.forEach((d) => {
            // If a sensor has entity category `config`, then change
            // it to `diagnostic`. Sensors have no input, so can't be configured.
            // https://github.com/Koenkk/zigbee2mqtt/pull/19474
            if (d.type === 'sensor' && d.discovery_payload.entity_category === 'config') {
                d.discovery_payload.entity_category === 'diagnostic';
            }
        });

        return discoveryEntries;

In this way, any future added sensors to other types are also covered.

@frenck
Copy link
Contributor Author

frenck commented Oct 30, 2023

Adjusted as suggested 👍

@Koenkk Koenkk merged commit 69f6891 into Koenkk:dev Oct 31, 2023
11 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Oct 31, 2023

Thanks as always 😄

// If a sensor has entity category `config`, then change
// it to `diagnostic`. Sensors have no input, so can't be configured.
// https://github.com/Koenkk/zigbee2mqtt/pull/19474
if (d.type === 'sensor' && d.discovery_payload.entity_category === 'config') {
Copy link

Choose a reason for hiding this comment

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

This seem to have missed binary sensors, which now also are validated.

Copy link
Owner

Choose a reason for hiding this comment

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

Pushed a fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seem to have missed binary sensors, which now also are validated.

This statement is not correct. This has not been implemented for binary sensors (but likely would in the future).

Copy link

Choose a reason for hiding this comment

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

It was implemented for mqtt integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh? Why would we implement such rules on an integration level? That makes no sense...

Will investigate that one.

Copy link
Contributor Author

@frenck frenck Nov 6, 2023

Choose a reason for hiding this comment

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

Ok, so this sneaked in for 2023.11.1 unintentionally, specifically on MQTT.
We are going to revert that part and ship Home Assistant 2023.11.2.

The change by Koen is still good, as that is expected behavior for 2023.12 (which got merged today as well).

../Frenck

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.

4 participants