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

[warmup] Initial contribution #8562

Merged
merged 20 commits into from
May 13, 2021
Merged

[warmup] Initial contribution #8562

merged 20 commits into from
May 13, 2021

Conversation

jamesmelville
Copy link
Contributor

@jamesmelville jamesmelville commented Sep 23, 2020

This binding integrates the Warmup 4iE Thermostat https://www.warmup.co.uk/thermostats/smart/4ie-underfloor-heating, via the API at https://my.warmup.com/.

I logged an issue for this at #7996. This PR was initially logged against 2.5.x in this PR #7998. All the comments in the PR have been addressed in this new PR for version 3.0.

Per the readme, it currently supports a Bridge to the API, holding the API credentials, and Rooms, which report current temperature and setpoint, and allow setting of an override of the setpoint for a specified duration.

I have tested this successfully on my own installation.

Resolves #7996

@jamesmelville jamesmelville requested a review from a team as a code owner September 23, 2020 20:21
@TravisBuddy
Copy link

Travis tests were successful

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

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 24, 2020
@fwolter fwolter self-assigned this Oct 4, 2020
@fwolter fwolter added the awaiting feedback Awaiting feedback from the pull request author label Nov 1, 2020
@jamesmelville jamesmelville requested a review from wborn as a code owner December 1, 2020 20:03
@jamesmelville
Copy link
Contributor Author

Hi @fwolter thank you for your review - I've updated everything as requested except for the one conversation which remains open above. Alongside that I think I've simplified the error handling logic and removed some code which was stopping polling if an internet connection became unavailable, sorry if this adds to your work reviewing but I thought it was preferable. CI build hasn't completed but I'll keep an eye out for any issues raised. Also not sure how I added wborn as a code owner 🤷.

@jamesmelville jamesmelville requested a review from fwolter December 2, 2020 22:11
@fwolter fwolter removed the awaiting feedback Awaiting feedback from the pull request author label Dec 4, 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.

The file bundles/org.openhab.automation.groovyscripting/userdata/persist.mqtt shouldn't be part of your PR.

What open point do you refer to?

@jamesmelville
Copy link
Contributor Author

jamesmelville commented Dec 4, 2020

The file bundles/org.openhab.automation.groovyscripting/userdata/persist.mqtt shouldn't be part of your PR.

Sorry - I thought I'd already removed that but it's in my next push

What open point do you refer to?

The duplicated serialNumber, which is now removed, so the point is no longer open.

@jamesmelville jamesmelville requested a review from fwolter December 8, 2020 20:19
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.

You need to replace SmartHomeUnits by Units to make the code compile.

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

@fwolter fwolter added the cre Coordinated Review Effort label Dec 18, 2020
@fwolter fwolter removed their assignment Dec 18, 2020
@fwolter fwolter requested a review from a team December 26, 2020 09:34
@cpmeister
Copy link
Contributor

Other than those few changes it looks good to me.

@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 22, 2021
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

There are some last open review comments. And since we're already into th enew year, new release some basic must be updated. Are you able to finish the binding so it can be merged?

@fwolter fwolter added the awaiting feedback Awaiting feedback from the pull request author label Apr 10, 2021
@fwolter
Copy link
Member

fwolter commented May 1, 2021

@jamesmelville can you fix the minor code review findings? Then we could merge this before OH 3.1 is released.

Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: James Melville <jamesmelville@gmail.com>
@jamesmelville
Copy link
Contributor Author

Hi @cpmeister @fwolter @Hilbrand - sorry for all the delays, life's been happening I guess! Anyway I think I've resolved all the review comments and got everything updated, not sure if I've done it in time for 3.1 but let me know!

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 addressing the remaining review comments!

LGTM

Your binding will make it into OH 3.1!

@fwolter fwolter changed the title [warmup] Add Support for Warmup 4iE Device [warmup] Initial contribution May 13, 2021
@fwolter fwolter merged commit db05079 into openhab:main May 13, 2021
@fwolter fwolter added this to the 3.1 milestone May 13, 2021
@fwolter fwolter removed awaiting feedback Awaiting feedback from the pull request author cre Coordinated Review Effort labels May 13, 2021
@fwolter
Copy link
Member

fwolter commented May 13, 2021

computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
Signed-off-by: James Melville <jamesmelville@gmail.com>
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
Signed-off-by: James Melville <jamesmelville@gmail.com>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: James Melville <jamesmelville@gmail.com>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: James Melville <jamesmelville@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.

[warmup] Add Warmup Binding
6 participants