-
-
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
[amazonechocontrol] Initial contribution for the Amazon Echo Control Binding (#3083) #3087
[amazonechocontrol] Initial contribution for the Amazon Echo Control Binding (#3083) #3087
Conversation
) Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Hi community, |
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Hi all, |
org.eclipse.smarthome.core.types, | ||
org.openhab.binding.amazonechocontrol, | ||
org.openhab.binding.amazonechocontrol.handler, | ||
org.openhab.core.library.types, |
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.
This dependency causes problems, please refactor to no be dependent on it anymore.
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.
Hi Martin, I do not what you mean with remove these dependencies, because the are used, but I have copied now the dependencies from the icloud binding (I only changed the names from icloud with amazonechocontrol). Now Jenkis is green again. But the build goes red again, but it has produced my binding: https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.amazonechocontrol/2.3.0-SNAPSHOT/
I do not know what's wrong. Thanks for your help!
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
If you would build (
So this is about the report of the Static code analysis. Please also check: http://docs.openhab.org/developers/development/bindings.html |
…alized exception for connection, correct comments Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
My problem is, that a local mvc clean install fails with a lot of errors in the first binding of the solution (which is not my binding):
But I found now the error log on the github server (Switch to RAW log, simple if you know it). So I will fix the errors. |
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Maybe take a look at: https://stackoverflow.com/questions/18075343/java-project-in-eclipse-the-type-java-lang-object-cannot-be-resolved-it-is-ind Or try running the mvn clean install from the command line, if you run windows I could advice running bash on windows to run maven there. |
Hi Martin, |
This sounds weird. Many people use a Mac for openHAB development, including myself - and Java8 works like a charm (rather, openHAB distro does not work with Java9 correctly). |
… code cleanup Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Build fails again, with a unit test which is not from my binding:
What should I do now? What happens with my pull request if another binding failing the build? |
Don't worry, this isn't a blocker. In order to improve the other tests, it might be nice though if you could create an issue for those failing tests, so that they become a known problem and can be dealt with. |
…g for lost internet connection Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
It's seems that the unit test depend on an external resource in the web, which was temporary unavailable. Because now the build is green without change in the test. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
…ider Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
@martinvw , @kaikreuzer My binding is now ready for a review. Please remove the "work in progress" tag. I' have implemented now all currently planned features. Only bugfixes will be done in the next time if they are reported here. |
I would remove it as a config option from the binding - others might feel tempted to do it likewise, which we should avoid.
This is imho a very administrative info, I would rather expect this to be a warning in the log and the admin should react on this. For getting notified about such things, the new log reader binding might be an appropriate means (again, this can be used by admins to define all kinds of notifications about problems base on log files - independently of a specific binding). |
Cleanup discovery. No more account discovery. Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
@mgeramb Let me know, once you are done - ideally by tonight, so that it can still make it into the 2.3 release! |
@kaikreuzer My current state:
I currently move the show ID away from the configuration and state provider to the servlet. This will be finished tonight. And hope I can finish to update the documentation for this change. But still open is |
Binding configuration for showIdinGUI removed, Ids shown in servlet Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
error handler added Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Update readme Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
@kaikreuzer I have finished my work now. Only the discussion for the image channel type and the flashbriefing is open. |
@kaikreuzer I have two technical questions:
|
Thanks for the updates, @mgeramb! I am ok with leaving 4+5 open and as is.
Good question, I haven't thought about that - you actually can't. You can only check the options for an item, so my idea with the console command for items makes even more sense with that.
Not yet, see https://github.com/eclipse/smarthome/projects/3#card-8298857. |
@Nullable | ||
HttpService httpService; | ||
@Nullable | ||
ServiceRegistration<?> discoverServiceRegistration; |
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 guess this can be removed 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.
Just one very small last comment to avoid resource leaks - then I am fine with merging it as is (although we had to agree on some quirks that won't make me refer to this binding as an example for best practices, no offense ;-))
} | ||
|
||
private void saveProperties() { | ||
try { |
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 should definitely use a try-with-resources here to avoid resource leaks!
if (result == null) { | ||
result = new Properties(); | ||
if (propertyFile.exists()) { | ||
try { |
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 should definitely use a try-with-resources here to avoid resource leaks!
* @author Michael Geramb - Initial Contribution | ||
*/ | ||
@NonNullByDefault | ||
public class StateStorage { |
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.
Would have been much nicer to make use of the ESH StorageService
, but let's leave it as it is now - I noticed that too late, sorry.
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.
@kaikreuzer: Is it so complicated to replace it? How can I get the StorageService? Is it injectable? Does it work if the thing is defined in a thing file?
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.
No, not really complicated, I just didn't want to put more work on you.
Feel free to refactor it tonight, if you are interested.
Here is an example of a binding that stores some data. The storage itself is created in the ThingFactory and obtained from the StorageService
.
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.
Does it work if the thing is defined in a thing file?
Yes!
Resource leak in state storage fixed, unused field removed in factory Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Fixed try syntax in StateStorage Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
@kaikreuzer I have checked in your requested changes |
Upgrade version for storage replacement Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Upgrade version for storage replacement with RC3 hint Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
StateStorage class removed Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
Jenkins null pointer access warning fixed Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
@kaikreuzer finished, StateStorage is removed and the StorageService used, it was really simple but it takes a immediate checkin and build to provide an upgrade possibility to the existing beta users. But one more question, what does this Jenkins warning mean:
Is there anything to do for me? |
html.append("<html><head><title>" + StringEscapeUtils.escapeHtml(BINDING_NAME) + "</title><head><body>"); | ||
html.append("<h1>" + StringEscapeUtils.escapeHtml(BINDING_NAME) + "</h1>"); | ||
html.append( | ||
"<html><head><title>" + StringEscapeUtils.escapeHtml(BINDING_NAME + " RC3") + "</title><head><body>"); |
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.
What is this about? There isn't any RC3...
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.
Ah, ok, I just saw that you did that for the intermediate release to the marketplace. All fine.
No, I have seen this in many places, it is nothing specific to your binding and afaik it does not harm (although it would be nice to have it addressed, but not in 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.
Thanks for all your patience with me and your prompt replies and updates!
I am happy that we managed to get it into the 2.3 release! 🎉
Thank you to all reviews! You have done a good job and I have learned a lot from you! |
…Binding (openhab#3083) (openhab#3087) Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
…Binding (openhab#3083) (openhab#3087) Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
…Binding (openhab#3083) (openhab#3087) Signed-off-by: Michael Geramb <[email protected]> (github: mgeramb)
This is my very first pull request. Please give me an hint if I did something wrong.
I marked the title with [WIP], I'am not sure if this is correct, this version is fully functionally and can be used. But of course a plan additional features in the future.
New Binding Amazon Echo Control
This binding let control openHAB Amazon Echo devices (Alexa).
It provide features to control and view the current state of:
Some ideas what you can do in your home by using rules and other openHAB controlled devices:
Detailed description:
https://community.openhab.org/t/first-public-beta-openhab2-amazonechocontrol-binding-controling-alexa-from-openhab2/37844/8
Build should be here:
https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.amazonechocontrol/