-
Notifications
You must be signed in to change notification settings - Fork 24
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
multiple tuner support #64
Conversation
008954f
to
5c53493
Compare
Had another look at the logic of this, and its not going to work as intended. Ill close this and re-evaluate. Was attempting to do it with as little code change as possible to other areas, but i think its going to require a larger core change to the logic to accomodate multiple tuners with the same channel lineups. |
Found some time to look at this briefly. I think the logic works this time. It checks for the uniqueId on the channel as a primary search, but if that doesnt work, it will continue looking for a match of the channel, subchannel, channelname all matching the PVR_CHANNEL object. This should mean it will find the first tuner that tests as available regardless of whether its the specified uniqueId it was searching for. May have to create an option to have a preference for a specific tuner/tuners and/or ability to blacklist tuners. |
New commit to make hide duplicate channel setting work. |
Thanks for the comment Matthew. I had wondered why you implemented a blacklist setting. This is all theoretical because i dont have multiple boxes to test with to actually see if it works as im expecting it to. In the long run, i think additional settings as you have done (preffered, and now i understand why the blacklist) would be desired, but this approach gets the basics up and running. |
111599d
to
3d18d79
Compare
0bb5076
to
95a4cdc
Compare
dbaf630
to
134aedf
Compare
Just a reminder for myself. peak3d has now made it possible to retrieve the status code >400. Should be able to drop the checktuneravailable function, set the "failonerror" protocol option (CURLAddOption(filehandle, ADDON_CURL_OPTION_PROTOCOL, "failonerror", "false")) and then retrieve using .GetFilePropertyValue(filehandle, FILE_PROPERTY_RESPONSE_PROTOCOL, "") This would remove the downside to this solution, without the requirement to implement locking using libhdhomerun. |
11b575b
to
1a61b5d
Compare
I've acquired a second hdhomerun device so ive now been able to test multiple tuner devices. I've updated the commit to now handle acquiring a channel from any available tuner. Any chance of a review on this @Rechi @ksooo . I know you will both be busy, but i believe it would be a nice addition to the addon that i can now confirm first hand functions as expected. |
addon version bump in addon.xml.in is missing |
CheckTunerAvailable() does a simple CURLOpen, and will fail if a http status code of >= 400 is returned when connecting. This will then allow _GetChannelStreamURL() to iterate through other tuners. This should allow the case with multiple tuner devices that support the same channel As the uniqueid is created with the url of the tuner, this wont match on a second tuner, therefore check for a match of the ChannelNumber, SubChannel Number and ChannelName are all equal to the requested channel
specifically check and deal with error response codes after xbmc/xbmc#14114
Fixed, sorry about that. |
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, code-wise. Cannot say anything about functionality, though.
How:
CheckTunerAvailable() does a simple CURLOpen, and will fail if a http status code
of >= 400 is returned when connecting. HDHomerun returns a 503 status code when no tuners available. This will then allow _GetChannelStreamURL() to iterate through other tuners to return another url from a different tuner.
Dev Details:
We are not able to find the exact status code to check for a 503 using kodi's CURLOpen provided to addons, as if anything >= 400 is detected, the call fails, and the new GetFilePropertyValue() isnt able to be run with this state. Ive gone down the route of using curl/curl.h to manage status in another branch (https://github.com/fuzzard/pvr.hdhomerun/tree/openlivestream), but build issues for appveyor (building curl on win automatically, without building for all platforms (linux/mac os not needing generally) have been stumping me for a couple of days, and im going to leave that as is for now. I hate cmake, but i digress.
Issues: Using the current codebase, the most glaring issue with this is that between the time of the CheckTunerAvailable method call and the return to GetChannelStreamProperties(), there is potential for change of status (tuner no longer available, failed tuner now available). For now, i think its a viable option for users with multiple tuners.
Discussion:
To implement this in the best case scenario that i can see, we would need to return to implementing our own Open/Read/CloseLivestream functions, and use the libhdhomerun hdhomerun_device_tuner_lockkey_request and release to handle our own locking. I've been fiddling with a branch that goes down this path, but havent fleshed it all out yet.
Interested in comments about traveling down this path to support multiple tuners better. My own personal thoughts are that it would be interesting from my dev point of view, but i dont have multiple tuners, so the testing would be tricky, and i dont know of any users who would be willing to guinea pig for me.
Is a close enough is good enough approach (This PR) good enough for most users, and therefore everything else is pointless?