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

[Bug] Excluding keys by default doesn't work #867

Open
1 task done
burmistrzak opened this issue May 21, 2024 · 12 comments
Open
1 task done

[Bug] Excluding keys by default doesn't work #867

burmistrzak opened this issue May 21, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@burmistrzak
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Tried to exclude a few properties/keys by default (occupied_cooling_setpoint to be specific) so I don't have to manually add a new configuration entry for every single thermostat discovered via MQTT to actually make them appear in Home.

Unfortunately, it seems like this doesn't work as expected, and excluded keys are still present on the newly discovered accessories.
However, excluding keys on a per-device basis works fine.

Related devices

No response

Related Devices

No response

Steps To Reproduce

No response

Expected behavior

No response

Device entry

No response

Status update

No response

Messages from this plugin

No response

This plugin

1.11.0-beta.5

Homebridge

1.7.0

Zigbee2MQTT

1.37.1-dev

Homebridge Config UI X (if applicable)

4.56.2

@burmistrzak burmistrzak added the bug Something isn't working label May 21, 2024
@itavero
Copy link
Owner

itavero commented May 22, 2024

Can you share the plugin configuration you tried initially?

Did it not work for any device, or only for specific devices?
Did those devices also have their own configuration?

@burmistrzak
Copy link
Contributor Author

Can you share the plugin configuration you tried initially?

@itavero Uhh, sure. I can share an excerpt, it's rather large configuration. 😅

[…]
	"defaults": {
		"exclude": false,
		"ignore_availability": false,
		"excluded_keys": [
			"linkquality",
			"occupied_cooling_setpoint"
		],
		"values": [
			{
				"property": "light",
				"exclude": [
					"color_xy"
				]
			},
			{
				"property": "system_mode",
				"include": [
					"off",
					"heat",
					"auto"
				],
				"exclude": [
					"cool"
				]
			}
		],
		"converters": {
			"switch": {
				"type": "outlet"
			},
			"occupancy": {
				"type": "motion"
			},
			"light": {
				"adaptive_lighting": true
			}
		}
	},
[…]

Did it not work for any device, or only for specific devices? Did those devices also have their own configuration?

I only tried excluding occupied_cooling_setpoint at this point (so to not break other stuff).
The affected devices are all Bosch thermostats (RBSH-RTH0-ZB-EU), and non of them had a config entry during testing.

To make the thermostats actually work, i.e. appear in Home, I had to add individual configs for each device to exclude occupied_cooling_setpoint, because cooling mode isn't supported, yet.

public static hasRequiredFeatures(accessory: BasicAccessory, e: ExposesEntryWithFeatures): boolean {
if (e.features.findIndex((f) => f.name === 'occupied_cooling_setpoint') >= 0) {
// For now ignore devices that have a cooling setpoint as I haven't figured our how to handle this correctly in HomeKit.
return false;
}
return exposesHasAllRequiredFeatures(e, [ThermostatHandler.PREDICATE_SETPOINT, ThermostatHandler.PREDICATE_LOCAL_TEMPERATURE]);
}

@burmistrzak
Copy link
Contributor Author

burmistrzak commented May 25, 2024

@itavero I just noticed that my exclude for color_xy is completely wrong?! Not sure what I was thinking back then... 😑

Anyhow, to ignore color_xy for every light, I'll have to add it to the default excluded_keys, correct?
Do these default settings also apply to already existing lights, or do I have to remove and re-added them?

Also, I see some strange behavior in Home where color mode is not retained and always goes back to color temperature mode... Seems to happen mostly when color/mode is changed outside of HomeKit. 🤔

@itavero
Copy link
Owner

itavero commented May 25, 2024

Anyhow, to ignore color_xy for every light, I'll have to add it to the default excluded_keys, correct?

That is indeed the way it's should be.

Do these default settings also apply to already existing lights, or do I have to remove and re-added them?

If I recall correctly it should apply to all devices after restarting the Homebridge, unless they have their own excluded_keys defined.
I basically filter the exposes information from Zigbee2MQTT based on the excluded_keys configuration.

Any reason you are excluding color_xy?
I believe color_hs has priority over it.

Also, I see some strange behavior in Home where color mode is not retained and always goes back to color temperature mode... Seems to happen mostly when color/mode is changed outside of HomeKit. 🤔

Do you have the experimental COLOR_MODE flag set? If I'm not mistaking it was an attempt at improving this behavior.

@burmistrzak
Copy link
Contributor Author

burmistrzak commented May 25, 2024

Any reason you are excluding color_xy? I believe color_hs has priority over it.

Just making sure. 😉

Do you have the experimental COLOR_MODE flag set? If I'm not mistaking it was an attempt at improving this behavior.

Yep, I have. Do I need to re-add the lights for the flag to take effect?
Unfortunately, when a color is set and I go back to Home app, a color temperature is suddenly selected instead... 🤨

Edit: Interesting, when I choose a color from the color wheel in Home, the color mode seems to stick. However, selecting a preset from the color swatches isn't as reliable.

Can you by chance try this yourself?
You'll have to tap the rightmost color blob to reveal the color wheel & swatches.

@burmistrzak
Copy link
Contributor Author

@itavero Does default excluded_keys even work with nested (i.e. features) properties? 👀

@itavero
Copy link
Owner

itavero commented May 28, 2024

@itavero Does default excluded_keys even work with nested (i.e. features) properties? 👀

It should. But that's also the potential bug I'm thinking of

@itavero
Copy link
Owner

itavero commented May 28, 2024

Had quick look. The code that filters the exposes based on this setting is here:

// Filter/sanitize exposes information
if (info?.definition?.exposes !== undefined) {
info.definition.exposes = sanitizeAndFilterExposesEntries(
info.definition.exposes,
(e) => {
return !this.isExposesEntryExcluded(e);
},
this.filterValuesForExposesEntry.bind(this)
);
}

Current code uses sanitizeAndFilterExposesEntries. As far as I can tell that also filters all the features, but perhaps you can spot a mistake.

export function sanitizeAndFilterExposesEntries(
input: ExposesEntry[],
filter?: (entry: ExposesEntry) => boolean,
valueFilter?: (entry: ExposesEntry) => string[],
parentEndpoint?: string | undefined
): ExposesEntry[] {
return input
.filter((e) => filter === undefined || filter(e))
.map((e) => sanitizeAndFilterExposesEntry(e, filter, valueFilter, parentEndpoint));
}
function sanitizeAndFilterExposesEntry(
input: ExposesEntry,
filter?: (entry: ExposesEntry) => boolean,
valueFilter?: (entry: ExposesEntry) => string[],
parentEndpoint?: string | undefined
): ExposesEntry {
const output: ExposesEntry = {
...input,
};
if (output.endpoint === undefined && parentEndpoint !== undefined) {
// Make sure features inherit the endpoint from their parent, if it is not defined explicitly.
output.endpoint = parentEndpoint;
}
if (exposesHasFeatures(output)) {
output.features = sanitizeAndFilterExposesEntries(output.features, filter, valueFilter, output.endpoint);
}
if (Array.isArray(output.values) && valueFilter !== undefined) {
output.values = valueFilter(output);
}
return output;
}

@burmistrzak
Copy link
Contributor Author

burmistrzak commented May 28, 2024

@itavero Alright, just did some testing, with interesting results!
To confirm excluded_keys actually works, I excluded co2 by default, and included it only for one device.
And after a quick restart, sure enough, there's the single CO2 sensor!

So we now know for sure, that it's working for properties other than occupied_cooling_setpoint. I'll add another thermostat to see what happens.

There's a chance it was all just a funny coincidence, because I excluded occupied_cooling_setpoint right after adding the thermostats..? 😅

Edit: Yup, seems to work now. Not entirely sure what happened here... Maybe the plugin detected the new thermostat right before Homebridge was restarted? Anyhow, sorry for the noise!

@burmistrzak
Copy link
Contributor Author

@itavero Just had an interesting thing happen...
Some excluded keys (e.g. humidity on thermostats) suddenly reappear (but unresponsive) in the Home app after I did an update & restart of Zigbee2MQTT.
Restarting Homebridge seemed to have cleared things up, and the keys/services in question have nowdisappeared.. It's still kinda strange tho...

Do we do something special when Zigbee2MQTT reconnects (after an update)? And do we always filter out excluded keys?

Another possibility would be some sort of stale cache on one of the Home hubs... 🤔

@burmistrzak burmistrzak reopened this Jun 19, 2024
@itavero
Copy link
Owner

itavero commented Jun 19, 2024

The way the information is received and processed is always the same, so I have no explanation for this yet.

@burmistrzak
Copy link
Contributor Author

The way the information is received and processed is always the same, so I have no explanation for this yet.

Ok, thanks for confirming. ✌️
I'll keep an eye out, and capture a detailed sysdiagnose when something like this happens again.

There's a real chance that these transient, super weird issues are actually coming from HomeKit itself. The Home app has in part been quite buggy for months now. 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants