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

Add config/setvalve item to generic #7614

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mattreim
Copy link
Contributor

@mattreim mattreim commented Feb 15, 2024

valve_set

@mattreim
Copy link
Contributor Author

The name "valve_set" fits better to "windowopen_set".

@mattreim mattreim changed the title Re-Enable valve control for tuya trv Re-Enable valve control attribute for tuya trv Feb 16, 2024
@mattreim
Copy link
Contributor Author

With setvalve I somehow expect this object.

Setvalve

@manup
Copy link
Member

manup commented Feb 20, 2024

While the original name doesn't have the ideal consistency this PR would be a breaking change for little gain (I think).

There are a few existing legacy devices out there which were joined with the config/setvalve item.

image

Further it changes the API for clients which would need to support with old vs. new deCONZ version.
To be honest I wouldn't do that.

If such a change is to be made it should deprecate the old item name while adding a new one and support both for some time until the old one is removed.

@manup manup requested a review from SwoopX February 20, 2024 13:37
@mattreim
Copy link
Contributor Author

If the effort is too high, I can of course change it back to "setvalue". But it would still be good to use it again in DDF`s.

Copy link
Collaborator

@SwoopX SwoopX left a comment

Choose a reason for hiding this comment

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

Puh, difficult... I guess we should have paid more attention to this one back in the days. However, if we now modify this resource item, we'll probably have more pain than gain in the end.

Clearly, we also have a discrepancy on the capability standing behind that functionality. The given item is missing an item specific json file if I'm not mistaken and expecting bools whereas the changed version here is also of type bool, but intended/meant for integers? On a personal note, I wouldn't consider setting the valve open percentage manually a good idea, the thermostat should do that for me imho.

If it might be about either detecting if a window is open to close the valve or enable such a feature, we do already have a resource item for both cases. So it can be added to the device if missing.

To sum it up: I'd also consider the suggested modification as a breaking change, but adding functionality where we already have items for should be doable.

@mattreim
Copy link
Contributor Author

I've changed it again now.

@mattreim mattreim changed the title Re-Enable valve control attribute for tuya trv Add config/setvalve item to generic Mar 21, 2024
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.

None yet

3 participants