-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[boschshc] Add support for Light/Shutter Control II #16400
[boschshc] Add support for Light/Shutter Control II #16400
Conversation
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.
Code looks good, thanks!
Left some comments, mainly documentation.
...hshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java
Show resolved
Hide resolved
...shc/src/main/java/org/openhab/binding/boschshc/internal/discovery/ThingDiscoveryService.java
Show resolved
Hide resolved
Hi @GerdZanker, @jlaur and @lsiepel, I would like to discuss the thing/channel modeling approach for this device with you. We have the following situation: Bosch provides a device that can be configured either as a shutter control or a light switch with two light switch circuits. The shutter control configuration is straight-forward (one device, one thing type, corresponding channels). However, when configured as two light switches, we see a parent device (it provides basic services such as communication quality and power metering) and two child devices, that represent the light switch circuits. In my current implementation I have modeled it in the same way in openHAB. I introduced two different thing types, and the user sees three things. Here is an overview of the things and channels:
Currently the user sees three independent things in openHAB, but they belong to one single physical device. From a technical perspective, this is the most straight-forward mapping, but I'm aware that this might not be the most user-friendly one, since users might expect one thing per physical device. Indeed, @mike-bike who was so kind to test my code already mentioned that this might be counter-intuitive for users (see #14562 (comment)). However, my rationale for separating the things was that the channel names are always the same this way. If we combined all these channels into one thing then we would need to "number" certain channels like this:
The problem is that this does not scale well. For example, if devices are released in the future that allow more switch circuits or even more complex / mixed configurations (like shutter controls, dimmers, relays, light switches freely configurable) then the channels would have to be configured in a dynamic way. If we keep the thing separation, we can use "thing composition" to model the configuration. What would be your preferred way to model this device and possible future devices in terms of the openHAB model? Are dynamic channel configurations possible and would you prefer it in this case? Also: the Light Control II device has the bridge as parent device. Is it possible to model deeper parent-child relations, such as the Light Switch being a child of Light Control II (and therefore a grandchild of the bridge)? |
Yes, that is possible, and I would actually suggest to go with your first proposition of separate things, but make the parent Light Control II a bridge. The Netatmo binding is one example of having multiple levels:
|
I do not think, that Netatmo actually fits to the Bosch device. Netatmo (and Weather Service providers etc) support user defined multiple „devices“ e.g. weather channels or weather stations etc. There the "root" thing acting as bridge makes sense. Defined as Roller Shutter II it has:
Defined as Light Switch II it has:
Though, I do not have any idea about the implementation complexity to define dynamic channels, but to me it would make logical sense to have it as one thing. Anyway, current setup works for me as well. I am happy about the support! Thanks guys Take care, |
I must say that this is no simple choice. If you model it as the suggested parent/child, i must agree with @mike-bike it feels a bit overcomplicated. About the scalability: don't know if you expect future devices with 3+ circuits, but i would not make this (not yet existing) problem a big issue. And if a device with many more circuits is introduced, it can allways be modeled as a parent/child device. Anyway, not making it easier here, but i would sugggest a more 'flat' structure of the Thing with numbered channels. Similar to how the z-wave binding handles a two button switch. |
Thank you very much for your input @jlaur, @lsiepel and @mike-bike. After having experimented with the parent/child model with two bridges, I am not really satisfied with the user experience. The Light Shutter Control II seems much harder to configure than the other devices although it's not really such a different device. Bosch exposes it as parent device with two child devices on the technical level, but this does not mean that the user (or the user interface) has to be aware of this technical representation. Maybe we can clarify the following before we proceed:
After that we might have more context information to make a decision. Thank you all for your input and support 👍 |
That's a good question. Interestingly it is shown as two devices in the Bosch App, there is no parent. Both devices do have the power consumption and energy as attributes. They both show the same consolidated values, but if a device is switched off, it shows power consumption 0 even though the other may consume power. If it is on, it shows the one consolidated value. The signal strengths is not visible in any of the Light Switch devices. It will only become an option if the device is configured as Roller Shutter. Please note, that I only can describe the visualization in the Bosch App and not the underlying software model. Similar view could be achieved in OpenHAB UI (e.g. by grouping) independent of the number of things. (Edit: inline images removed. Happy to share via PM as needed) |
@lsiepel I just tried to find the two-button z-wave switch, but I got lost because there are tons of thing definitions 😆 Could you please direct me to a relevant thing definition and the corresponding handler implementation? |
I'm not familiair with the z-wave code. My suggestions was purely from functional perspective. It is one device, that has 2-buttons. They are modelled exactly as @mike-bike suggested, so i would suggest to create something like: How to construct those channels in code from the Bosch devices is beyond my knowledge about this device. But i don't think it would be that difficult for you. |
After having thought about the different possibilities for some time I'm now also in favor of this model (one thing type, numbered channels). I will provide an implementation as soon as possible 👍 @jlaur, I hope that you are also OK with this approach since it's different from what you suggested previously. |
Definitely, it sounds like the best solution now when I have seen all arguments. 👍 My proposal was made when not knowing all details, and was mostly to confirm that multiple layers of bridges is possible and sometimes useful. I agree on the more flat structure for this particular case confined by specific hardware with exactly two switches. |
I just finished an implementation with a single thing type and re-uploaded the JAR 👍 @mike-bike could you please download and test again? Note that it's important to delete all things and channels belonging to the old implementation, otherwise you might run into persistence / configuration issues. So the order should be
|
Thanks for testing again and for your feedback ❤️ That's really amazing considering the amount of code that was changed in the mean time 😉 I guess it does pay off to work with unit tests 😎 I'm working on code cleanups, some refactorings that are required now, maximizing unit test coverage and documentation. Will push some code soon. Stay tuned 👍 |
bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/config/configs.xml
Outdated
Show resolved
Hide resolved
I pushed my changes and the PR is open for code reviews again 👍 Note for the reviewers @lsiepel @jlaur and @GerdZanker: it might be easier to compare against @mike-bike I propose to wait until after the code reviews before the next (and hopefully last) test iteration. You need to test once more that all my refactorings worked and we might change the labels/identifiers (e.g. |
Hi @david-pace, I will wait until advised. Happy to test when you are ready. |
Signed-off-by: David Pace <dev@davidpace.de>
the i18n generator seems to behave incorrectly when entries are removed Signed-off-by: David Pace <dev@davidpace.de>
Signed-off-by: David Pace <dev@davidpace.de>
Signed-off-by: David Pace <dev@davidpace.de>
Signed-off-by: David Pace <dev@davidpace.de>
Signed-off-by: David Pace <dev@davidpace.de>
Signed-off-by: David Pace <dev@davidpace.de>
ae1833f
to
06dc7b2
Compare
Signed-off-by: David Pace <dev@davidpace.de>
5c1cda0
to
4ee1f23
Compare
I rebased everything on @mike-bike please test again with the new JAR and let me know how the |
I cannot see any such log entry with the new JAR any longer. ==> Fixed!
Deleted all items and things and loaded new JAR. Devices discovered and added to Inbox. I have added 1 device to the model and tested functionality:
For me it looks good! Not sure what you mean by If so, please see snipped below. However, I could not find any of the additional logs you have mentioned earlier. No "sorting" or "successfully"... I have loaded the following: Here the relevant extract from the log:
Please let me know if I can help any further. PS: How do I get the code snippes into the browser window without breaking the lines whenever I paste from the terminal window? |
@david-pace , correction! I did a restart after clearing the cache. After a while (the PI2 is soooo slooooow) I think I found what you were asking for (pls ignore the ugly formatting):
|
Hi @mike-bike, thank you so much for testing again 👍 You can paste code, log files etc. in so called fenced code blocks. Begin and end a block with triple backticks like this:
Thanks to your analysis we now know that the Smart Home Controller seems to list the child device IDs in a somewhat strange order. For both of your devices, we first get
It looks like It looks like we could even hardcode the child device IDs to In this case I would say the code is open for code reviews again 😎 Thank you very much for your testing support @mike-bike ❤️ |
Note to the reviewers @GerdZanker, @jlaur and @lsiepel: I created two additional PRs on top of this one:
The other two PRs are much easier to review once this PR is merged, because otherwise all the commits from this PR will "clutter" the other PRs. I will therefore wait until this PR is reviewed and merged, then rebase the other ones, and then add you as reviewers. It's not time-critical since we are not talking about bugfixes, but rather about functional enhancements and stability improvements through tests. I just wanted to let you know that this PR kind of "blocks" the other PRs and that I have prepared more code already 😎 |
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.
Nice, to see this growing support of more and more devices. Thank you @david-pace
@lsiepel - it looks like your review is almost finished according to #16400 (review). Just checking if you will be able to finish your review, perhaps it just got off your radar? 😉 I had a brief look myself, and it looks nice and clean to me (as usual). 👍 |
Hi Guys, I just opened another opportunity to grow even further: #16590 😀 Have a great weekend Regards, |
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.
LGTM, again very clean. Thanks!
* [boschshc] Add support for Shutter Control II (openhab#14562) * add new channel type for child protection Signed-off-by: David Pace <dev@davidpace.de>
* [boschshc] Add support for Shutter Control II (openhab#14562) * add new channel type for child protection Signed-off-by: David Pace <dev@davidpace.de> Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
* [boschshc] Add support for Shutter Control II (openhab#14562) * add new channel type for child protection Signed-off-by: David Pace <dev@davidpace.de> Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
* [boschshc] Add support for Shutter Control II (openhab#14562) * add new channel type for child protection Signed-off-by: David Pace <dev@davidpace.de>
* [boschshc] Add support for Shutter Control II (openhab#14562) * add new channel type for child protection Signed-off-by: David Pace <dev@davidpace.de>
Description
This PR adds support for another Bosch Smart Home device, namely Light/Shutter Control II. The device can either be configured as shutter control or as light control with two light switch circuits.
Testing
JAR Download for Testing