-
Notifications
You must be signed in to change notification settings - Fork 482
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
base: master
Are you sure you want to change the base?
Conversation
mattreim
commented
Feb 15, 2024
•
edited
edited
The name "valve_set" fits better to "windowopen_set". |
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 Further it changes the API for clients which would need to support with old vs. new deCONZ version. 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. |
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. |
There was a problem hiding this 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.
I've changed it again now. |