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
zwave_js: cover: Fibaro FGR-222 venetian blind tilt support #49371
Closed
+193
−5
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We probably don't want to add device specific code to the platforms. That should be solved some other way, preferably upstream in the zwave-js project.
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.
zwave_js
implements the Z-Wave protocol. AFAICS (please correct me if I'm wrong), theManufacturer Proprietary
command class of the Z-Wave protocol is explicitly designed to contain device-specializations. By providing access to theManufacturer Proprietary
values,zwave_js
is doing its part by implementing the transport layer. It mustn't know about semantics of the payload.It's up to the driver to make use of the payload, and the driver is home-assistant. We had the same approach when we implemented this for OpenZWave (#29701) and it was accepted back then.
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.
We'll only accept this PR if the device specific code can be abstracted into a problem case in discovery like we did here for dynamic climate temperature values: #49804
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.
Understand the ask, but I won't have time to re-design this. So feel free to close this if no-one wants to step in.
Two thoughts before you do that:
First, in the spirit of pragmatism, why introduce an abstraction for a problem now, before it is clear if there ever will be a second user of said abstraction (there might be, not denying that). The PR in its current shape will introduce less code than with the abstraction. Why not defer it to the time a second user emerges? The PR as it is right now will probably enable a bunch of users of the FGR222 right away (https://community.home-assistant.io/t/fibaro-fgrm222-venetian-blind-tilt-position/50710/17)
Second, each device that has some manufacturer-proprietary characteristics might have some unique quirks here or there. For the FGR222, for example, it is the tilt-position interfering with the reported cover position (https://github.com/home-assistant/core/pull/49371/files#diff-e6292e73b3e851acd39ec60f47355d032083c995521448faa206c8105c620fe6R252-R255). Of course, you can abstract over this specific problem as well, but doing that for each new device can bloat the abstraction over time as well. I still think that viewing Z-Wave as a transport / communication means and not as the driver itself makes sense as well.
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.
We don't want device specific code in the platforms. I don't think we'll accept the workaround for the tilt. That should be fixed by the device.