-
-
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
[enigma2] Initial Contribution #7514
Conversation
Travis tests have failedHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
5 similar comments
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
Travis tests were successfulHey @gdolfen, |
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.
Hello Guido,
thank you VERY much for porting the enigma2 binding to openHAB 2!
First of all, thank you for writing test cases! This is always handy if others start to edit your code later on so they notice when stuff breaks. :)
I left some minor comments in the code, but in general the quality of the binding looks pretty well already!
I do have a VUplus Uno4k and can do some tests with it if you like, after you have included my small comments from above.
Regards,
Stefan
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/enigma2/internal/discovery/Enigma2DiscoveryParticipant.java
Outdated
Show resolved
Hide resolved
Travis tests were successfulHey @gdolfen, |
Hello Stefan, thank you for your feedback. I tried to fix all of your findings, but two of them I don't understand (see conversion below). Can you please give me some more hints? It would be nice of you, if you can make some tests with your VUplus Uno4k. I'm just developing an enhancement for supporting sending questions and resolving answers. Should I commit this into this PR, or in a second one, if it is ready? Best 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.
Thanks, looks much better already.
I edited my 2 comments from yesterday that were messed up by github because I forgot to add the code ticks.
And I added 3 more comments. Please have a look.
I will give the binding a try later today probably.
Regarding your question:
I'm just developing an enhancement for supporting sending questions and resolving answers. Should I commit this into this PR, or in a second one, if it is ready?
I am not sure what you mean by questions and resolving answers? Is that a feature of the receiver? What does it do?
If you have the code ready for review, feel free to put it in now. if it takes time to develop and debug I would suggest to leave it out and get this PR in a shape so it can be merged.
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
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.
I just tested it with my VUuno4k receiver and its basically working.
As I had the binding running in my IDE i guess i was missing the mdsn bundles so auto discovery didn't work for me. However, this pointed me to the missing defaults in the configuration of the thing that I have just commented.
I only tested the controls via PaperUI though. But I think if they are working, the thingactions should also work if they work with your receiver.
One thing that would be nice for a version 2: Some kind of subscription mechanism to the receiver so we do not have to poll and get the information delayed, I don't know if this is possible though.
bundles/org.openhab.binding.enigma2/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.enigma2/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Spoken too early... I though you had the "strange characters filtered out already, but I get:
You should definitely catch these exceptions. For now it looks to me as if the Thing is going into "OFFLINE with configuration error". And if the Thing goes OFFLINE with configuration error, we should ALWAYS provide a message to the user so he knows how to fix it without digging into debug logs (in my case i dont see a message so a user would not know what to do). |
I now catch the IllegalArgumentException in the trasmit-Method. And I think the reason of this exception is because I dindn't URL-encode the id of the current channel. |
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.
These should be the final changes
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Outdated
Show resolved
Hide resolved
@cpmeister: |
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 just noticed that you didn't include an update to the CODEOWNERS file in the PR. Please add yourself to it.
Also you need to update the bom/openhab-addons/pom.xml
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/handler/Enigma2Handler.java
Outdated
Show resolved
Hide resolved
@cpmeister: |
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.
Everything looks good now, thanks for all of your work! But before this can be merged, another addons maintainer needs to take a look at it so please sit tight until they come around.
@cpmeister: Thank you for your approval. |
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 your contribution and your patience I added some remarks, let me know when you have any questions.
Martin
bundles/org.openhab.binding.enigma2/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.enigma2/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.enigma2/src/main/resources/ESH-INF/i18n/enigma2.properties
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/enigma2/internal/discovery/Enigma2DiscoveryParticipant.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/enigma2/internal/discovery/Enigma2DiscoveryParticipant.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Outdated
Show resolved
Hide resolved
...penhab.binding.enigma2/src/main/java/org/openhab/binding/enigma2/internal/Enigma2Client.java
Show resolved
Hide resolved
@martinvw |
Signed-off-by: gdolfen <[email protected]>
- Fixed review finding Signed-off-by: gdolfen <[email protected]>
You should at least change the version in your pom.xml, but rebasing would be the best |
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 for you rapid feedback!
If you can rebase and update the version number and the build is done we should be ready for a merge, thanks for your hard work!
@martinvw There are many pending builds because of an error receiving files from the apache repository: |
Travis tests were successfulHey @gdolfen, |
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]> Signed-off-by: CSchlipp <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
* [enigma2] Initial contribution openhab#7514 - Fixed review finding Signed-off-by: gdolfen <[email protected]>
This is a PR for adding support of enigma2 set top boxes like a dreambox or a vu+.
It is tested on a vu+ solo2 device. For more details see the readme: https://github.com/gdolfen/openhab-addons/blob/enigma2/bundles/org.openhab.binding.enigma2/README.md
There ist a snapshot build available here for testing: https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.enigma2/2.5.5-SNAPSHOT/
Because this ist my first PR to openhab please be so kind and let me know if there is something missing.
I opened a discussion here: https://community.openhab.org/t/new-enigma2-binding/97887