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

[draytonwiser] Drayton Wiser Binding initial contribution #3168

Merged
merged 122 commits into from
Aug 9, 2020

Conversation

andrew-schofield
Copy link
Contributor

@andrew-schofield andrew-schofield commented Jan 24, 2018

Add a new binding that supports the Drayton Wiser smart heating system.

Currently the binding supports auto-discovery, all current devices and most channels.
Some features are still in development, but the binding is usable currently, and has been tested on 4 different heating setups.

There's a discussion thread in the community here.

The development of this binding was mentioned in #3126.

jar file for testing:
https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.draytonwiser/2.5.0-SNAPSHOT/org.openhab.binding.draytonwiser-2.5.0-SNAPSHOT.jar

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/drayton-wiser-thermostat-binding/35640/72

@martinvw martinvw added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels Jan 24, 2018
@andrew-schofield
Copy link
Contributor Author

Not sure why the fail state on Jenkins after my most recent commit. The log suggests that the maven JVM died at the end even though the compile completed successfully.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/drayton-wiser-thermostat-binding/35640/114

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/drayton-wiser-thermostat-binding/35640/1

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/drayton-wiser-thermostat-binding/35640/170

@andrew-schofield
Copy link
Contributor Author

I really don't understand the random build system failures I'm seeing. Why are there errors in other plugins, when all I've changed is code in my plugin, and they passed last time?

@andrew-schofield andrew-schofield changed the title [draytonwiser][WIP] Initial contribution [draytonwiser] Initial contribution Apr 25, 2018
@andrew-schofield
Copy link
Contributor Author

This PR is no longer WIP and is ready for review. I am aware there are multiple commits that are missing the signed-off-by line, but given you're going to request that I squash all the commits before merging anyway, I don't see this as a blocker to review.

@martinvw martinvw removed the work in progress A PR that is not yet ready to be merged label Apr 25, 2018
@andrew-schofield andrew-schofield force-pushed the draytonwiser branch 3 times, most recently from cdfd4a2 to 33c47b1 Compare June 14, 2018 21:42
@andrew-schofield
Copy link
Contributor Author

I've gone through and signed-off all the historic commits that were causing review errors.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/drayton-wiser-thermostat-binding/35640/199

Copy link

@triller-telekom triller-telekom left a comment

Choose a reason for hiding this comment

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

Thank you very much for writing this binding!

I made a first review where I just looked at the modeling of your things and channels. Once we find an agreement, I will continue by looking at the sources.

Please have a look at my comments inside the code and do not hesitate to ask if you have questions.

EDIT: if you make changes, please make them in additional commits. And if you like to integrate the newest changes from the master branch, please rebase your branch onto it, do not merge it.

@triller-telekom
Copy link

I have answered some of you questions, please have a look.

Also please do not add further features to this PR like the smart plug you just added, because this makes the review more difficult. Otherwise if you plan to add more things before someoen reviews them, please change the name of this PR to "WIP" (work in progress).

@andrew-schofield
Copy link
Contributor Author

andrew-schofield commented Jul 29, 2018

I've sorted out most of the channel names and types etc. I haven't added UoM to any channels yet, nor have I removed the json channels.

Question: I tried to add humidity as a system channel, however when I tried to use the binding on my instance of 2.3.0 I got an error message about it not being a supported channel-type. Is system.atmospheric-humidity only in >=2.4.0?

@triller-telekom
Copy link

Thank you for sorting out the channel names so far.

To answer your question: Yes the system.atmospheric-humidity is very new and only available in 2.4.0-SNAPSHOT of openHAB so far.

@andrew-schofield
Copy link
Contributor Author

I've just tried looking at the UoM documentation and some of the bindings that use it, and I'm afraid I'm thoroughly confused.
When I receive some data from the binding, like a temperature update, do I need to return a QuantityType(value, UnitOfReceivedValue)?
When I'm handling a command (like updating a setpoint), how do I convert the command value to the input unit I actually want?

@triller-telekom
Copy link

When I receive some data from the binding, like a temperature update, do I need to return a QuantityType(value, UnitOfReceivedValue)?

Yes, exactly.

When I'm handling a command (like updating a setpoint), how do I convert the command value to the input unit I actually want?

That depends on what type the command is and what kind of data your device expects... If you define the channel to only accept QuantityTypes then you know what value and unit it is. If you also allow DecimalTypes (Number instead of Number:UOM_Type) then you have to define what you do with that value.

@andrew-schofield andrew-schofield force-pushed the draytonwiser branch 2 times, most recently from 98363f0 to b33849f Compare August 12, 2018 20:22
@andrew-schofield
Copy link
Contributor Author

OK, I've added UoM to all the channels I think I reasonably can, with the exception of humidity as I haven't set up a test 2.4.0 instance of openhab to make sure it works. I haven't disabled the schedule channel yet, as I'd like to be able to keep a way of updating a schedule, but I need to think about a better way of doing it.
The Jenkins failure appears to be because of a broken modbus binding on master :(

Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
…utes

Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
…ng quantity type

Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
… duration

Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
* Changed time back to minutes

It makes much more cleaner code.
It's ok to not just specify %unit% in the pattern. The pattern is a template anyway.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>

* Updated snapshot version to 2.5.8-SNAPSHOT

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>

Co-authored-by: Andrew Schofield <the.uncle.fungus@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @andrew-schofield,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand added this to the 2.5.8 milestone Jul 28, 2020
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Here is my review feedback.

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @andrew-schofield,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

- Added missing representation property to thing xml file
- Changed representation property of bridge to network address because serialnumber is not know at discovery time.
- Added id as representation property to controller. To find only 1 controller if the user uses a  different thinguid as the discovery.
- Fixed hot water thing discovery. It doesn't have a serial number and also no related to a device.
- Improved the readme formatting.
- And some other review comments.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
@TravisBuddy
Copy link

Travis tests were successful

Hey @andrew-schofield,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

There are two approvals from maintainers, now. But @Hilbrand contributed to this PR. How do we handle this?

@andrew-schofield
Copy link
Contributor Author

LGTM

There are two approvals from maintainers, now. But @Hilbrand contributed to this PR. How do we handle this?

Yeah, I would be happier if @cpmeister also approved it if we need more than one approval.

@Hilbrand
Copy link
Member

Hilbrand commented Aug 9, 2020

I agree that my approval is not completely independent 😉 It's just @cpmeister did approve and then left 2 small comments, which have been addressed imoh. And because @cpmeister doesn't seem to be available at the moment (his last activity was july 15th) I don't think that should be blocking this being merged (given he already approved it albeit with 2 additional comments. So if you agree with me @fwolter I would suggest we merge this binding.

@kaikreuzer
Copy link
Member

I agree with @Hilbrand and this PR has anyhow a looong history with a lot of reviews, so that it is really time to have it merged. Big thanks to everyone involved here!

@kaikreuzer kaikreuzer merged commit 6e32b2f into openhab:2.5.x Aug 9, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Andrew Schofield <the.uncle.fungus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.