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

Rules engine does not cope pretty well with invisible properties #2467

Closed
bewee opened this issue Mar 25, 2020 · 11 comments
Closed

Rules engine does not cope pretty well with invisible properties #2467

bewee opened this issue Mar 25, 2020 · 11 comments
Labels

Comments

@bewee
Copy link
Member

bewee commented Mar 25, 2020

Currently, the rules engine does not cope pretty well with rules that use properties marked invisible.

But let's say I have a light which has a property "mode" (enum: white, colour) and two more properties "colour" and "brightness" which are shown or hidden according to the selected mode. I can now create two rules, one depending on "brightness" and the other one depending on "colour" (I need to change the "mode" property of the light between creation of the two rules). This will then lead to always one of them getting marked [INVALID] and deactivated. Still both will be executed (although deactivated, a little weird), but when entering edit mode the rule is changed so that the now invalid nodes are deleted irreversibly.

Proposal:
The rule engine should always allow all known properties of a device to be used regardless of their visibility state.

@mrstegeman
Copy link
Contributor

We've never had a case (until now) where properties were conditionally marked visible, based on another property.

For this specific state, there is a new feature coming with 0.12: ColorModeProperty

I'd suggest adding this to your devices, and then you can base your rules off of that property value plus the other relevant property value.

@bewee
Copy link
Member Author

bewee commented Mar 25, 2020

Great! I've already seen this one, but why is it marked read-only?

@bewee
Copy link
Member Author

bewee commented Mar 25, 2020

Anyway, I guess there may be other devices doing this as well. At least I did so with this adapter (Radio: frequency is only available in FM mode, whereas skipping songs can only be done during listening Spotify, ...)
How were hidden properties usually used so far?

@mrstegeman
Copy link
Contributor

Great! I've already seen this one, but why is it marked read-only?

None of the devices I've worked with have let me directly change mode. With that said, there's nothing that's forcing it to be read-only, and I can change the schema if necessary.

How were hidden properties usually used so far?

Until now, we've only ever used them to hide complexity. For instance, a device may internally have separate hue, saturation, and brightness properties, but we may mark those as invisible and create a "fake" ColorProperty, which will internally map back to the real properties.

@bewee
Copy link
Member Author

bewee commented Mar 25, 2020

None of the devices I've worked with have let me directly change mode. With that said, there's nothing that's forcing it to be read-only, and I can change the schema if necessary.

The tuya lightbulbs I use allow this, even more they require the mode to be changed manually. Would be really great if this could be changed.

Until now, we've only ever used them to hide complexity. For instance, a device may internally have separate hue, saturation, and brightness properties, but we may mark those as invisible and create a "fake" ColorProperty, which will internally map back to the real properties.

But does this really require additional properties? And besides this: Wouldn't it be nice to be able to change the hue, for example, on its own from the rules engine, even if it is hidden in the normal device view?

@mrstegeman
Copy link
Contributor

The tuya lightbulbs I use allow this, even more they require the mode to be changed manually. Would be really great if this could be changed.

Done: https://iot.mozilla.org/schemas/#ColorControl

But does this really require additional properties? And besides this: Wouldn't it be nice to be able to change the hue, for example, on its own from the rules engine, even if it is hidden in the normal device view?

This is done to match the ColorProperty schema, which takes an RGB hex color value. It was just an example. We do similar things with door locks, where the schemas use actions but the devices use a property.

@bewee
Copy link
Member Author

bewee commented Mar 25, 2020

Done: https://iot.mozilla.org/schemas/#ColorControl

Thanks

This is done to match the ColorProperty schema, which takes an RGB hex color value. It was just an example. We do similar things with door locks, where the schemas use actions but the devices use a property.

I don't quite see yet why it should be a problem to allow rules to make use of these hidden attributes.
But anyway, could maybe some sort of other visibility flag be offered that is intended to change? For instance it could be called 'shown', set to true if not defined and affect visibility just as the visible flag but be ignored by the rules engine.
Or another idea: For a property, one could specify whether its visibility might change, for instance with a flag called 'variableVisiblity'
Obviously these are all just ideas.

@mrstegeman
Copy link
Contributor

It seems weird to me to have properties that are visible to rules but not in the normal properties UI. Maybe, instead of changing the visibility, you could mark properties as read-only when they're not allowed to be changed?

@bewee
Copy link
Member Author

bewee commented Mar 25, 2020

I guess I see your point there. But I find it kind of unnecessary to show properties to the user which are completely irrelevant at the moment. Especially for devices with many modes, things may get messy and there may even be so many properties altogether they can no longer be rendered well (but still one may want to use them in rules). And therefore it seems somehow more reasonable to me to fill the available space with relevant properties.

@mrstegeman
Copy link
Contributor

Yeah, that's definitely a fair point. This probably deserves some additional thought about the right approach. This may even be another case where object properties would come in handy.

@mrstegeman
Copy link
Contributor

I'm going to close this. We had some issues with the 1.0.0 release where the visible property was not properly forwarded to the gateway, which led me to the conclusion that we really shouldn't have "invisible" properties. Instead, adapters should start filtering out their own invisible properties if they don't want the gateway to see them.

See also: #2670, #2672, WebThingsIO/gateway-addon-python#87, WebThingsIO/gateway-addon-node#82, WebThingsIO/gateway-addon-ipc-schema#15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants