-
-
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
[verisure] Verisure Binding initial contribution #4789
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/testers-for-verisure-openhab-2-binding/7499/150 |
Ok just to make sure everybody is in agreement. @jarlebh can you give feedback on if your oke with this updated implementation replacing your pr? Or @jannegpriv if you have more information from @jarlebh can you provide that here? |
I was in email-contact with Jarle during the autumn when I fixed an issue #3849 with the Tellstick-binding, since Jarle was the author of that aswell. Jarle then told me that he had moved to HA, I've not been in contact with Jarle since that. |
Yes, I don't use OpenHab anymore so please replace my pull request.
Den man. 4. feb. 2019 kl. 16:30 skrev Hilbrand Bouwkamp <
notifications@github.com>:
… Ok just to make sure everybody is in agreement. @jarlebh
<https://github.com/jarlebh> can you give feedback on if your oke with
this updated implementation replacing your pr? Or @jannegpriv
<https://github.com/jannegpriv> if you have more information from @jarlebh
<https://github.com/jarlebh> can you provide that here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/openhab/openhab2-addons/pull/4789#issuecomment-460291319>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZhJvO_qQmwhT7nUgir22gE2seiIxPbks5vKFIVgaJpZM4agKis>
.
|
@jannegpriv and @jarlebh thanks for your responses. I'll close pr #3455 and we'll continue with this pr. |
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.
Thank you for picking up this binding. I've done a first review. I've looked at small style issues that make the code more readable and in style with openHAB. I understand some parts of the code were already there and you just build on it. So some comments might be on older code and something you also might have seen that could have been better.
I've also looked at some more structural issues. In general the code looks good. But there is a lot of duplicated code. This makes it harder to detect/review bugs. If something has been fixed at one place, it might be needed to fixed at other places. But due to the amount of code that might be difficult to detect.
Regarding the structural part. There is a lot of code related to sending commands that does look very similar. The code would greatly benefit if you can extract some generic component out of it and pass variables to the method. I think this can greatly reduce the code.
Another change could be to create separate handler classes for each device type (thing). Then in each handler you can restrict the code to what is needed for that device type. You could create a base class to handle the general part of the code.
I understand these changes are not small. But I think if you are willing to do are worth the effect and would make it easier to add new devices or make changes in case the api of the device changes.
The structural changes are more suggestions. So feel free to comment or discuss my review comments or ask for advice.
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
addons/binding/org.openhab.binding.verisure/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
I've now gone through the review comments and I have made some refactoring and restructioning of the binding. I must say that the review comments were very accurate and extensive and I've learnt a lot from them, kudos for that :-) I've now run into problems with my branch, since after some days I could no longer compile it with maven ( Should I create a new PR with a clean branch or how should I continue? I could of course push my changes to my branch for further review, and then after fixing those comments create a new branch. |
I pushed up my latest updates for review, I will then need help on how to proceed. |
Do you still have problems with your branch? It should not be necessary to create a new pr. It's always possible to fix the branch. If you still have problems let me know. I see you made some changes, but now the sign off is missing. Can you re-add them (or squash the commits and add it the one commit)? If you still have problems with the building, you might need to rebase again from master. There have been a number of changes lately due to changes to openhab-core, which resulted in problems with openhab2 addons. It could be you just merged at such point. Updating again should fix that. In general updating source to the latest from the remote master can be done with |
After syncing my branch with master a while ago I managed to get my branch to compile again. A git log in my branch reveals 6 commits that I've made, including 2 merges.
I then tried to squash my 6 commits via `git rebase -i HEAD~6, I then get a long list of commits in the editor:
Here I'm lost since my commits are not in a chronological order due to the merge. If I get help with solving this this sqaush there is only need or one sign-off, isn't it? I should be able to edit the commit message as a part of the squash procedure if I'n not mistaking.
I've always used the following schema for updating my local master and local feature branch with latest upstream master changes:
I then push my changes to my local branch to origin via My remotes:
The command sequence you listed, how is the use-case flow I described in step 1)-5) for updating local master and local feature branch and then pushing local feature branch done using those commands? I'm really glad for the all the help and I'm eager to get this resolved! :-) |
Yes you can move them to the top. It would look as if you had rebased on master. When done you must push force to github otherwise it will not work.
Yes that's correct.
Assuming you don't commit on master, it's all the same except for step 5: |
ac23666
to
5b1034c
Compare
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Travis tests were successfulHey @jannegpriv, |
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Travis tests were successfulHey @jannegpriv, |
…e review remark. Added more info to README. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Travis tests were successfulHey @jannegpriv, |
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, but 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: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Travis tests were successfulHey @jannegpriv, |
Just waiting for Jenkins to build before merging. @Hilbrand do you know why the DCO check fails? |
The DCO check sometimes seems to be magic. Because while it looks like it's correctly signed of it still fails. Sometimes it's because a commit is made with a different e-mail as used in the sign-off, but other times I have no idea. In general when there is a sign off in the commit (or at lease a sign off in a single commit) we consider it signed off. And when we merge it's squashed anyway so the merged commit message can updated with the signed off as set in the original commit. |
|
||
@Override | ||
public void run() { | ||
logger.debug("Trigger Event {} on {} at time {}", event, vth, DateTime.now()); |
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.
Please replace by ZonedDateTime
.
logger.debug("Trigger Event {} on {} at time {}", event, vth, ZonedDateTime.now());
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.
Fixed!
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.
You need to create a new pull request with this commit as this pull request is merged already.
import org.eclipse.smarthome.core.thing.binding.BridgeHandler; | ||
import org.eclipse.smarthome.core.types.Command; | ||
import org.eclipse.smarthome.core.types.RefreshType; | ||
import org.joda.time.DateTime; |
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.
The usage of org.joda.time
is deprecated and already removed from OHC (see openhab/openhab-core#1342).
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.
Fixed!
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv) Signed-off-by: CSchlipp <christian@schlipp.de>
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv) Signed-off-by: MPH80 <michael@hazelden.me>
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv) Signed-off-by: Daan Meijer <daan@studioseptember.nl>
Also-by: Jarle Hjortland <jarlebh@gmail.com> (github: jarlebh) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> (github: jannegpriv)
[verisure] Initial contribution of Verisure binding.
This has already appeared as a pull request before #3455, but @jarlebh is no longer active in OH community.
I’ve made a re-design of the Verisure binding to support multiple installations/sites since I also have a Verisure installation at my summer house.
I’ve based the re-design on the work made by @Andreas_L, @jarlebh and @MSV12 but updated it to handle multiple sites, the changes are not backward compatible so all discovered things will get new internal IDs.
I’ve also added support for some new things:
I’ve also compiled a jar-file based on OH2.5 but it also works for OH2.4.
I've checked and corrected output from static code analysis.
Commit has been signed off.
URL to the community thread.
Fixes #4788