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

Add setting to use HTTP-based tuner discovery #133

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

djp952
Copy link
Contributor

@djp952 djp952 commented Mar 2, 2022

Refers to Issue #116 - "Use ip address instead of device discovery"

This Pull Request adds a new setting to the addon that allows users to opt into using the SiliconDust API "HTTP Discovery" (at least that's what I've always it) to find their tuner devices. The normal UDP broadcast discovery doesn't work when the tuner(s) are on a different subnet than the client.

The HTTP discovery data can become "stale", as any device the user had turned on and connected will stay in this list for up to 24 hours after it's been turned off or disconnected. As such, the only data points used from the API are "DeviceID", which indicates the device has tuners and "LocalIP". The LocalIP address is used to then contact the device and gather the discovery information normally via libhdhomerun. This allows stale devices to be ignored.

If no devices are found via HTTP, there is a fallback to try UDP discovery as well. I felt this important since this API might disappear in the future.

Please let me know what you think or any edits that need to be made, happy to oblige.

I only added the setting string to en_gb, this is correct and it will be translated automatically at some point, yes?

Copy link
Contributor

@fuzzard fuzzard left a comment

Choose a reason for hiding this comment

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

extremely minor nits, otherwise looks good.

pvr.hdhomerun/addon.xml.in Outdated Show resolved Hide resolved
src/HDHomeRunTuners.cpp Outdated Show resolved Hide resolved
src/HDHomeRunTuners.cpp Show resolved Hide resolved
@djp952
Copy link
Contributor Author

djp952 commented Mar 2, 2022

Will address all this evening and re-push, thank you for the review!

@djp952
Copy link
Contributor Author

djp952 commented Mar 3, 2022

All requested changes made; sorry to Jenkins for the double force-push, I noted a blank line between variable declarations that shouldn't have been there but didn't see it until I looked at the green/red diff here

@fuzzard
Copy link
Contributor

fuzzard commented Mar 3, 2022

Looks ok to me. I'll have to ping @ksooo or @phunkyfish for merge rights.

One question i will ask, are you aware of whats returned in the event of an ipv6 network?
Im not silly enough to run that for my internal gear but im sure someone will chime in one day with WHATABOUTMYIPV6.
The query only comes about due to the use of ntohl.

Im ok to pushing it back until someone complains, so was just more a query whether in your experience with your other DVR addon if anyone had ever mentioned usage on an ipv6 network

@djp952
Copy link
Contributor Author

djp952 commented Mar 3, 2022

One question i will ask, are you aware of whats returned in the event of an ipv6 network? Im not silly enough to run that for my internal gear but im sure someone will chime in one day with WHATABOUTMYIPV6. The query only comes about due to the use of ntohl.

The current HDHomeRun ecosystem is exclusively IPv4, their APIs actually redirect to "ipv4-api.hdhomerun.com" if you dig into them. There is no support for IPv6 I can see in their libraries and I've never seen any discussion about that from SiliconDust, everything they talk about is always in IPv4 speak.

I think it's a safe bet for now, if they ever add IPv6 to their devices we'll see changes in libhdhomerun, the ntohl version of the IP is all that would be accepted today by the library. Basically, we'd have no way to add it right now even if we wanted to :)

Copy link
Member

@phunkyfish phunkyfish left a comment

Choose a reason for hiding this comment

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

LGTM

@phunkyfish phunkyfish merged commit 22ac7d8 into kodi-pvr:Nexus Mar 3, 2022
@phunkyfish
Copy link
Member

phunkyfish commented Mar 3, 2022

Add-on builds appear to be broken, once they fixed I'll get this released. Thanks for the contribution @djp952!

@fuzzard
Copy link
Contributor

fuzzard commented Mar 4, 2022

If you have some spare time @djp952 may be worth a backport to v19. I'm not sure how add-ons usually handle features, but I imagine it's not as strict as core Kodi.

@phunkyfish
Copy link
Member

Add-on builds appear to be broken, once they fixed I'll get this released. Thanks for the contribution @djp952!

Builds working and it’s released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants