Skip to content
This repository has been archived by the owner on Apr 15, 2023. It is now read-only.

[pvr.nextpvr] Added support for ATSC subchannel numbers, fixing 'no tuner available' m... #384

Merged
merged 1 commit into from
Jan 13, 2015

Conversation

sub3
Copy link
Contributor

@sub3 sub3 commented Nov 16, 2014

Added support for ATSC subchannel numbers, fixing 'no tuner available' message users were getting with this type of channel
Fixes a problem with playback of radio stations
Added recording artwork
Added fanart

@Jalle19
Copy link
Collaborator

Jalle19 commented Nov 17, 2014

Is the intention to get this included for Helix?

@Jalle19
Copy link
Collaborator

Jalle19 commented Nov 17, 2014

Needs a rebase as well.

@sub3
Copy link
Contributor Author

sub3 commented Nov 17, 2014

Yes, including in Helix was my intention.

@Jalle19
Copy link
Collaborator

Jalle19 commented Nov 17, 2014

Okay, if you rebase I can pull it in.

@sub3
Copy link
Contributor Author

sub3 commented Nov 18, 2014

I've rebased this. Please go ahead an pull it in. Thanks!

@Jalle19 Jalle19 changed the title Added support for ATSC subchannel numbers, fixing 'no tuner available' m... [pvr.nextpvr] Added support for ATSC subchannel numbers, fixing 'no tuner available' m... Nov 19, 2014
@sub3
Copy link
Contributor Author

sub3 commented Nov 28, 2014

Can you tell me when this will be merged? I really want these bug fixes included.

@Jalle19
Copy link
Collaborator

Jalle19 commented Nov 29, 2014

It'll have to wait until the first Helix point release, we need to focua on getting Helix out the door.

@sub3
Copy link
Contributor Author

sub3 commented Dec 13, 2014

What do I need to do to get this included in Helix now, instead of waiting for the point release? The first two changes are bug fixes, and causing significant issues for users of ATSC tuners and those using DVB radio. The other two changes are very minor. I can rebase this again to resolve the conflicts with more recent changes. I think it's unfair on NextPVR users to not include this.

@Jalle19
Copy link
Collaborator

Jalle19 commented Dec 13, 2014

@opdenkamp you decide

@sub3
Copy link
Contributor Author

sub3 commented Dec 23, 2014

I've rebased this again, so that it includes later changes from the upstream opdenkamp repository.

@opdenkamp
Copy link
Owner

meh, something broke this PR again. needs another rebase, sorry.

…' message users were getting with this type of channel

Fixes a problem with playback of radio stations
Added recording artwork
Added fanart
@sub3
Copy link
Contributor Author

sub3 commented Dec 31, 2014

OK, I've rebased this again.

@opdenkamp
Copy link
Owner

will merge this in asap, but will do #372 first. will probably merge these open PRs locally and push the result so we don't need a load of rebases again

@sub3
Copy link
Contributor Author

sub3 commented Jan 3, 2015

Ok thanks. Let me know if you need me to do anything.

@opdenkamp opdenkamp merged commit 53c48b2 into opdenkamp:master Jan 13, 2015
opdenkamp added a commit that referenced this pull request Jan 13, 2015
@sub3
Copy link
Contributor Author

sub3 commented Jan 14, 2015

Hi Lars. Just to be clear - this is going to show up in 14.1 right?

On 13/01/2015 1:09 p.m., Lars Op den Kamp wrote:

Merged #384 #384.


Reply to this email directly or view it on GitHub
#384 (comment).

@Jalle19
Copy link
Collaborator

Jalle19 commented Jan 14, 2015

@sub3 you'll need to create a corresponding PR to the "helix" branch (which got added today).

@sub3
Copy link
Contributor Author

sub3 commented Jan 14, 2015

You've got be f*cking kidding me. Are you guys intentionally trying to make life difficult for me? I've been trying to get this small code change, which fixes problems for NextPVR ATSC users, merged for like 2 months now.

@sub3
Copy link
Contributor Author

sub3 commented Jan 14, 2015

Sorry about above - just frustrated. I'll create a new helix branch and pull request later today.

@opdenkamp
Copy link
Owner

"master" is always for the upcoming release

@sub3
Copy link
Contributor Author

sub3 commented Jan 15, 2015

"helix" was the upcoming release way back when I created the pull request...

@opdenkamp
Copy link
Owner

and unfortunately we were already in beta by that time.

@sub3
Copy link
Contributor Author

sub3 commented Jan 15, 2015

and a serious bug effecting all ATSC users can't be fixed in beta?

@opdenkamp
Copy link
Owner

the "serious bug" must be on the backend's side then, expecting
subchannel numbers in the url that it didn't expect before.
also, using the channel number for tuning is not a good idea. you should
be using the channel id instead. your add-on will break if users don't
have "use backend channel numbers" enabled.

On 15-01-15 02:44, sub3 wrote:

and a serious bug effecting all ATSC users can't be fixed in beta?


Reply to this email directly or view it on GitHub
#384 (comment).

@sub3
Copy link
Contributor Author

sub3 commented Jan 15, 2015

That's your implementation recommendation, but either way, whether I'd chose to change the addon to use the channel id, or the major.minor number, I would still have needed to submit a change to the addon. I opted for the major.minor number, because this Kodi addon is just reusing streaming interface that is supposed to be human usable from apps like VLC etc, where the user wont know channel ids, but will know channel numbers.

I'll submit the pull request for helix today.

@opdenkamp
Copy link
Owner

I've never recommended to use channel numbers for anything like this. I
still don't, as it's something that breaks when people change the order
on one frontend, or use different channel numbers on that frontend, or
whatever.

There were no minor channel numbers until a couple of months ago when I
added it, and your add-on didn't change in the meantime. So the reason
for suddenly requiring those minor channel numbers must have come from
an update on the backend's side. Which probably also means that the
backend no longer supports Gotham or older.

On 15-01-15 21:12, sub3 wrote:

That's your implementation recommendation, but either way, whether I'd
chose to change the addon to use the channel id, or the major.minor
number, I would still have needed to submit a change to the addon. I
opted for the major.minor number, because this Kodi addon is just
reusing streaming interface that is supposed to be human usable from
apps like VLC etc, where the user wont know channel ids, but will know
channel numbers.

I'll submit the pull request for helix today.


Reply to this email directly or view it on GitHub
#384 (comment).

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

Successfully merging this pull request may close these issues.

3 participants