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

Initial commit of the kodi binding #1262

Merged
merged 25 commits into from
Dec 27, 2016
Merged

Initial commit of the kodi binding #1262

merged 25 commits into from
Dec 27, 2016

Conversation

pail23
Copy link
Contributor

@pail23 pail23 commented Sep 26, 2016

This is the initial commit for a kodi (https://kodi.tv/) binding.

@kaikreuzer kaikreuzer added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 27, 2016
@MHerbst
Copy link
Contributor

MHerbst commented Nov 19, 2016

Thanks to @Pail for implementing this binding. I have tried it with my Kodi installation and the first tests looks very good.

I have still some comments/questions:

  • The binding seems to concentrate on Kodi's music functions (would be enough for me). The "old" XBMC binding has some additional functionality for videos and PVRs. Are you thinking about adding this functionality to the binding?
  • The binding uses its own libraries for http and websocket communication. As most of the bindings are now using jetty for http I think it would be a good idea to switch the implementation to jetty to reduce the number of different third party libraries. As far as I can see only the websocket protocol is used, so it should be possible to implement the communication with the Jetty websocket client API.
  • Jetty also contains a websocket implementation (https://www.eclipse.org/jetty/documentation/9.2.10.v20150310/jetty-websocket-client-api.html) and I think it would be good idea to use this library. Unfortunately this library is not currently included in the openHab Target. @kaikreuzer Do you think it would be possible to add it to the openHAB target? Then other bindings using websockets could use it too.

As far as I can see only one class in the binding needs to be modified to introduce jetty. I can try it in the next days.

@kaikreuzer
Copy link
Member

@kaikreuzer Do you think it would be possible to add it to the openHAB target?

Definitely yes!
Especially as Jetty is (as you correctly point out) the library to use for HTTP client code, we should include websocket clients here as well. I'll try to update the target platform accordingly soon!

@pail23
Copy link
Contributor Author

pail23 commented Nov 20, 2016

@MHerbst the binding also works for videos. PVRs are not supported at the moment since I don't use kodi as a PVR. Maybe somebody who uses the PVR functionality could extend this binding once this PR is merged.
I can try to migrate the code to Jetty once the websocket part is available in the target platform. As you already mentioned, the changes should be limited to once class.

@kaikreuzer
Copy link
Member

FYI: openhab/openhab-distro#329
If you pull the latest master from openhab-distro, your targetplatform should now include the Jetty websocket libs!

@MHerbst
Copy link
Contributor

MHerbst commented Nov 21, 2016

Thanks @kaikreuzer. That was really fast.

@pail23 I just tested it with a movie and video and everything works fine. Don't know why I thought that there was missing something ... Maybe you can add an additional channel to display the media type. The old XMBC binding has got a property "Player.Type".

If you have problems with the migration you can leave me a message and then I can try to help.

- Added media type channel
- Added the AudioSink support


Signed-off-by: Paul Frank <[email protected]>
@pail23
Copy link
Contributor Author

pail23 commented Nov 23, 2016

@MHerbst I have tried to migrate the binding to websocket. Preliminary tests have been successful. It would be great if you could test as well and provide me feedback. I have also added the mediatype channel.

@MHerbst
Copy link
Contributor

MHerbst commented Nov 24, 2016

Great @pail23 ! Will test it this weekend.

<option value="Select">Select</option>
<option value="Back">Back</option>
<option value="Home">Home</option>
<option value="ContextMenu">Conext Menu</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a typo. Should be "Context menu"

} else if (method.startsWith("GUI.OnScreensaver")) {
processScreensaverStateChanged(method, params);
} else {
logger.warn("Received unknown method: {}", method);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better so reduce the warning level to debug. When Kode updates its library e.g. there are a log of messages like this:
2016-11-26 11:33:36.524 [WARN ] [b.kodi.protocol.KodiConnection:341 ] - Received unknown method: AudioLibrary.OnUpdate

I have also seen several other methods that are not handled (and don't need to be handled). So I think level "debug" would be sufficient.

@MHerbst
Copy link
Contributor

MHerbst commented Nov 26, 2016

Hi Paul,
I have tested it with Kodi 16 and 17 and it works with both versions without real problems. Really good work!
I have added two comments directly into the commit diffs regarding a typo and a log message.

Here two additional remarks:

  • the media type does not work correctly for all types. For music it is OK, but when I play a youtube video I get "unknown". Here is the debug message with the JSON message : {"jsonrpc":"2.0","method":"Player.OnPlay","params":{"data":{"item":{"title":"Amy Macdonald - Down By The Water (New single) - LIVE BBC Radio Scotland","type":"movie","year":2016},"player":{"playerid":1,"speed":1}},"sender":"xbmc"}}.
    To me it looks that there is valid type of "movie".
  • Your commit contains only a language properties file with xx_xx. I can create a file with german texts and post it here.

@MHerbst
Copy link
Contributor

MHerbst commented Nov 26, 2016

I forgot one remark: you have add a new channel to the binding ("mediatype"). After I started openHAB with an existing configuration the new channel was not visible within Paper UI in the Kodi thing. I had to remove and add the thing again. I am not sure whether this is a problem in the (Smarthome) runtime or whether the binding has to implement something to refresh the channel.
Maybe @kaikreuzer can help to clarify this.

@ThomDietrich
Copy link
Member

Hello Paul and thanks for implementing that binding. As an intensive user of Kodi I really appreciate it. Is this a new development or is it based on the xbmc binding?

I had a quick look at the README and I think it should be extended by more details before the merge of this PR @kaikreuzer.

Signed-off-by: Paul Frank <[email protected]>
@pail23
Copy link
Contributor Author

pail23 commented Nov 28, 2016

Hi @MHerbst

Thanks for testing and reviewing. I have update the PR based on your comments

@ThomDietrich This binding was orignially based on the OH1 xbmc binding, but in the meanwhile it's quite different. As an example: The new binding uses only web socket communication, whereas the the OH1 binding also uses http communication. I can try to work on the README. Nevertheless I appreciate also contributions from others ;-)

@MHerbst
Copy link
Contributor

MHerbst commented Nov 28, 2016

Hi @pail23

thanks :-). Will try it in the next days. I hope I'll be able to send you a properties file with german translations until end of this week.

Maybe you have already seen the issue regarding #1496 regarding national characters.

In the meantime I was able to add a PVR add-on to my Kodi installation. So I can try to add PVR support and the functionality would be similar to the old XMBC binding.

@kaikreuzer
Copy link
Member

I had to remove and add the thing again. I am not sure whether this is a problem in the (Smarthome) runtime or whether the binding has to implement something to refresh the channel.
Maybe @kaikreuzer can help to clarify this.

This is indeed a known issue. I have just created eclipse-archived/smarthome#2555, so it can be addressed in future.

Copy link
Member

@kaikreuzer kaikreuzer 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 this contribution!
I managed to reduce the LOCs by half by removing superfluous empty lines ;-)

Love to see it merged soon! Please also add it to the binding/pom.xml and the feature.xml.

When testing, my Kodi stayed in "INITIALIZING" status, i.e. it never went ONLINE or OFFLINE. Does this work for you? Maybe check if this status update mechanism can be somehow improved. Thanks!

http://www.eclipse.org/legal/epl-v10.html

-->
<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" immediate="true" name="org.openhab.onkyo" activate="activate">
Copy link
Member

Choose a reason for hiding this comment

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

org.openhab.onkyo -> org.openhab.kodi


## Binding Configuration

The binding can auto-discover the Kodi players present on your local network. The auto-discovery is enabled by default. To disable it, you can create a file in the services directory called Kodi.cfg with the following content:
Copy link
Member

Choose a reason for hiding this comment

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

This line should actually be in the "Discovery" section. And no need to create a Kodi.cfg file; instead, the line should be added to conf/services/runtime.cfg.

The binding can auto-discover the Kodi players present on your local network. The auto-discovery is enabled by default. To disable it, you can create a file in the services directory called Kodi.cfg with the following content:

```
org.openhab.Kodi:enableAutoDiscovery=false
Copy link
Member

Choose a reason for hiding this comment

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

org.openhab.Kodi -> org.openhab.kodi


The Kodi player thing requires the ip address and the port to access it on.
In the thing file, this looks e.g. like
```
Copy link
Member

Choose a reason for hiding this comment

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

please add an empty line above code sections as otherwise Jekyll breaks the layout.

In the thing file, this looks e.g. like
```
Kodi:Kodi:myKodi [ipAddress="192.168.1.100", port="9090"]

Copy link
Member

Choose a reason for hiding this comment

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

You are missing the closing ``` here.

logger.error("exception during connect to " + wsUri.toString(), t);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line


public void input(String key) {
socket.callMethod("Input." + key);

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

JsonObject params = new JsonObject();
params.addProperty("text", text);
socket.callMethod("Input.SendText", params);

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line


public void playNotificationSoundURI(String uri) {
playURI(uri);

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

playURI(uri);

}

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

@ThomDietrich
Copy link
Member

ThomDietrich commented Dec 15, 2016

Hey guys, this might be too early in the development process: I want to point out, that the README.md in this PR lacks some information and clarity. It would be amazing if this would get sorted out before the binding is merged.

@kaikreuzer
Copy link
Member

Could you check if this the remote control option is enabled in your installation?

I definitely have this enabled, yes. Still, it stays in INITIALIZING.
But in any case, if no connection can be established, the status should be switched to OFFLINE. @pail23 Could you improve the code in such a way?

@ThomDietrich What information and clarity do you exactly miss? Imho, the readme is ok, at least for people that already know what Kodi is.

@ThomDietrich
Copy link
Member

ThomDietrich commented Dec 21, 2016

@kaikreuzer my remark was more aimed at little details like capitalization or clarity of certain terms.
@pail23 If you are interested in my contribution, please take as much as you like from this revised version: https://gist.github.com/ThomDietrich/b39938de6d1e053fe912a2c4b0bb3298
In comparison to other Binding readme's one part that's still missing: "Full Example"

I've got one question. The former xbmc binding had a lot more properties/channels (e.g. the show title). Are you going to add these as well?

- improved the online/offline state handling


Signed-off-by: Paul Frank <[email protected]>
@pail23
Copy link
Contributor Author

pail23 commented Dec 21, 2016

@kaikreuzer The status goes no to OFFLINE after startup if there is no connection to kodi
@ThomDietrich Thanks for your proposal. If have basicaly copied it with 2 minor additions. The main part which is missing in the binding is the PVR handling. Since I don't use this, I propose to merge the binding once the currently implemented code is OK. Maybe somebody else can then submit a PR to add the PVR handling.

@kaikreuzer
Copy link
Member

@kaikreuzer The status goes no to OFFLINE after startup if there is no connection to kodi

Thanks @pail23! Now I only need to figure out, why it isn't connecting in my case...

@MHerbst
Copy link
Contributor

MHerbst commented Dec 21, 2016

@pail23 @ThomDietrich I have a now PVR support (for T-Entertain) running in my Kodi installation and can work on it. I just wanted to wait until a first version of the binding was merged.

@ThomDietrich
Copy link
Member

@pail23 What about data like the show title? I think there is more available than the PVR related stuff...

@pail23
Copy link
Contributor Author

pail23 commented Dec 23, 2016

@ThomDietrich I have added code to read the show title from kodi and add it to the title channel. I was not able to play something in kodi which has a non empty showtitle. It would be great if you could test this.

@ThomDietrich
Copy link
Member

Hey pail, I'm not in my home for 1-2 weeks. That will make it difficult to test with my Kodi instance.

In my current setup I've got the following two items:

String Kodi_Title ... {xbmc="<[#livingRoom|Player.Title]"}
String Kodi_TitleShow ... {xbmc="<[#livingRoom|Player.ShowTitle]"}

These items normally behave in the following way:
Movie: Kodi_Title="Matrix", Kodi_TitleShow=""
TV Show: Kodi_Title="Everyone dies", Kodi_TitleShow="Game of Thrones"

Only a show has a ShowTitle. Pretty easy to identify.

Thanks for adding the show title but please do not just add it to the title channel with a " - " in between. That's not how I want to use it and others might also have their own ideas... Would you consider provide it as its own channel?

- Implemented a systemcommand channel which allows to hibernat/shutdown/...


Signed-off-by: Paul Frank <[email protected]>
@pail23
Copy link
Contributor Author

pail23 commented Dec 27, 2016

@ThomDietrich I have seperated the show title now. My first intend with the merged title was to save real estate on the screen when displaying these values. But this can also easily be achieved with a simple rule concatinating the 2 values.
In addition I have added a system command channel which allows to reboot/shoutdown/hibernate/suspend kodi.

@ThomDietrich
Copy link
Member

Wonderful. Thank you. I will be happy to test this binding and report my findings after it got merged. Is anything still stopping this?

@kaikreuzer
Copy link
Member

on my to-do list for today!

@kaikreuzer
Copy link
Member

I finally figured out why I wasn't able to connect: I was using Kodi 17 alpha and this simply didn't allow remote applications... The beta now works nicely :-)

Only a show has a ShowTitle.

Shall we maybe define the channel as "advanced" then? I don't really like to see this empty field there by default... "mediatype" imho should also be marked advanced.

But apart from that, I don't have any further remarks, so let's merge - many thanks, it is a cool feature!

@kaikreuzer kaikreuzer merged commit 213f4d6 into openhab:master Dec 27, 2016
@MHerbst
Copy link
Contributor

MHerbst commented Dec 27, 2016

Great. This makes it easier to add some additional (still missing) features to have a full replacement of the older version.

@ThomDietrich
Copy link
Member

@kaikreuzer could you (to me as a non binding developer) explain which implications "advanced" would have?
thanks for merging!

@kaikreuzer
Copy link
Member

"advanced" means that it is only shown when you click "more" in the Paper UI and that in simple mode, it isn't linked by default.

@ThomDietrich
Copy link
Member

@pail23 I just saw that the merged readme.md does not contain a "Full Example" :( Would you care to add one?

@pail23 pail23 deleted the kodi branch January 9, 2017 20:11
@mtzro2003
Copy link

@kaikreuzer kaikreuzer modified the milestone: 2.0.0 Jan 17, 2017
@sehroriginell
Copy link

sehroriginell commented Jan 18, 2017

Dear @pail23,

thank you for your new binding. Everything works so far for me. But I have two questions:

  1. Is there any way to figure out weather or not Kodi is online right now? (Unfortunately it's not possible to check the online/offline-state of a thing in a rule yet.)

  2. Can you just end the Kodi application without shutting down or hibernate the machine it's running on?

Thank you!

fharni pushed a commit to fharni/openhab2-addons that referenced this pull request Feb 10, 2017
* Initial commit of the kodi binding

Signed-off-by: Paul Frank <[email protected]>
Flole998 pushed a commit to Flole998/openhab-addons that referenced this pull request Dec 30, 2021
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Aug 12, 2023
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.

7 participants