-
-
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
[kermi] Initial contribution #16329
base: main
Are you sure you want to change the base?
[kermi] Initial contribution #16329
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/kermi-modbus-binding/153385/1 |
What's the difference with #15687 ? |
@lolodomo This is a modbus binding and works without any authentication, the other type of binding is different. it uses an api endpoint, which i don't know that it may exist. But from my talks with kermi (manufacturer) the only way to get all the specific data is a modbus interface. By the way i contacted col-panic for exchange our knowledge, but currently i think both bindings are useful, maybe they work on different models. |
As described in my contribution, I reverse-enginered the binding by analysing the web-interface of the kermi heatpump's x-center. It is not as elegant as this solution, which by the way seems to be supported by the producer kermi - who never reacted to my mails - but it does not require a modbus binding. (I don't know modbus enough - that is, i only know the 2-wire modbus connections where you have to have special hardware, with my binding you don't need this). Still, as this seems to be in some way supported by the producer, I'd be in favor of this binding! |
@col-panic Hi, thanks for your participation to explain the difference. So #15687 is a binding, which connects to the WebBackend in the cloud, which is normally used by the kermi-(mobile)-app. This binding is for non-cloud / local access to the current values, it works with modbus TCP, so only local network access is requiered. For other (older) versions of the heatpump, additional hardware (converter from modbus RTU to modbus TCP) may be needed, which uses to connect by 2-wire-connection. |
No, not in the cloud! I use it to directly connect to the x-center in my local network! |
We should prevent two bindings that have a very high overlap. Could we somehow intetgrate both binding proposals into one? What would be the best way of moving this forward @KaaNee and @col-panic ? |
This binding seems to be definitely the better one, as there seems to be at least some support by the manufacturer! I don't think that the code I have written can contribute anything to the functionality implemented in this PR. It seems like @KaaNee did directly mail Kermi about opening the ModbusTCP - I will have to find out, whether this approach is feasible with my franchised version of Heizbösch. But It would be nice, if you could elaborate more on the mail exchange with Kermi. Did they enable this feature remotely? Don't consider #15687 anymore - consider it the be virtually closed. |
@col-panic can you perform some kind of review ? check if all the channels and use-cases that you had covered are available here. Maybe if there is a (small) gap, this can be implemented right away by @KaaNee |
Hi @lsiepel. yes, you're right. both are nearly the same, having the difference in their connection. My binding requires manual remote-settings from Kermi, which maybe can be a blocker for some users, if kermi (or similar manufacturer) isn't that supportive, maybe difficult to "force" them to add these settings in every users heating-central. The other binding from @col-panic is just working out of the box, maybe for more devices-types, we don't know. Long Story short: @col-panic Maybe you can publish your addon in the addon-store that your work won't be lost and used by every user which does not succeed or don't want the changes by kermi.
@col-panic If you disagree, than i could publish this on the addon-marketplace - the other way round. I'm fine with both solutions. |
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.
First review pass. Looked at the first 20 files mainly focused on the things, channels and structure. The comments i made apply to all channels and channel types, so please also look at other occurences in the other xml files.
- Keep labels short, < 23 chars, 2-3 words and capitalized.
- Make good use of UoM
- Use system types if possible: https://www.openhab.org/docs/developer/bindings/thing-xml.html#system-state-channel-types
...hab.binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-workhours-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-pvmodulation-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-pvmodulation-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-pvmodulation-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-pvmodulation-channel-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.modbus.kermi/src/main/resources/OH-INF/i18n/kermi_de.properties
Outdated
Show resolved
Hide resolved
1a3eb28
to
476777d
Compare
@lsiepel Thanks for your review. I corrected / improved the code. Please re-review. |
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.
I still need to look at the handler, bu don't want to hold back these comments. Most of the code looks solid. Also had a quick peek at the readme and noticed the channels need to be aligned with the thing xml files.
...nding.modbus.kermi/src/main/java/org/openhab/binding/modbus/kermi/internal/dto/AlarmDTO.java
Outdated
Show resolved
Hide resolved
...nding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-chargingcircuit-channel-types.xml
Outdated
Show resolved
Hide resolved
...nding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-chargingcircuit-channel-types.xml
Outdated
Show resolved
Hide resolved
...nding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-chargingcircuit-channel-types.xml
Outdated
Show resolved
Hide resolved
...nding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-chargingcircuit-channel-types.xml
Outdated
Show resolved
Hide resolved
...nding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-chargingcircuit-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-energysource-channel-types.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-energysource-channel-types.xml
Outdated
Show resolved
Hide resolved
...penhab.binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-power-channel-groups.xml
Outdated
Show resolved
Hide resolved
....binding.modbus.kermi/src/main/resources/OH-INF/thing/xcenter-pvmodulation-channel-types.xml
Outdated
Show resolved
Hide resolved
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.
Ran out of time, don;t want to hold back the comments so far. The last two files we have to look at, so getting closer.
...rc/main/java/org/openhab/binding/modbus/kermi/internal/handler/KermiXcenterThingHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/modbus/kermi/internal/handler/KermiXcenterThingHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/modbus/kermi/internal/handler/KermiXcenterThingHandler.java
Outdated
Show resolved
Hide resolved
Common mistake where the openHAB remote repository is directly merged into the feature branch. To prevent this i always merge openhab/main => my fork/main => my feature branch. Not sure if that was also the cases here, but it seems fixed 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.
Last few comments, otherwise LGTM. Do you need some time to test the binding after all these changes?
bundles/org.openhab.binding.modbus.kermi/src/main/resources/OH-INF/i18n/kermi.properties
Outdated
Show resolved
Hide resolved
Signed-off-by: Kai Neuhaus <code@kaineuhaus.com>
Signed-off-by: Kai Neuhaus <code@kaineuhaus.com>
Yes, i will do some final testings on a fresh system, will report back when finished.
BTW: I created a script for generate "Items"-Files, this is useful for other devs, too. Is there a good place to drop it ? |
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.
Thanks again for this contribution. LGTM. Ping me when you are finished with the tests.
Next step would be to add the binding logo to the openHAB website. See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website
New Binding for Kermi Heat Pumps
Similar implementation like modbus e3dc binding, this is a simple modbus binding for kermi heat pumps with modbus-tcp connection activated.
It's an easier integration for a kermi heat pump, preventing creating modbus-things and items manually, which is some really annoying click-work and confusing with bridges, pollers, data-things and items for each single value.
Just create a Modbus TCP-Bridge, create a kermi x-center thing attached to this modbus-bridge and you're fine. Finally adding items to the desired channels.
OH community-thread: https://community.openhab.org/t/kermi-modbus-binding/153385?u=kane