[mythtv] [mythtv-commits] Ticket #1945: DVB-S/diseqc patch

[Yeasah Pell]

Daniel Kristjansson wrote:

>On Sun, 2006-07-02 at 13:19 -0400, Yeasah Pell wrote:
>  
>
>>I can't easily do what you're suggesting here, because the branch has so 
>>many changes all in one changeset, it would be more difficult to tease 
>>apart the individually coherent changes than it is worth. I'll enumerate 
>>the changes I know about here.
>>    
>>
>
>If you want to drop the DiSEqC changes that is up to you. I would
>prefer your code over the current code, but I can't do that without
>some cooperation. If you had created the small independent patches
>I asked for, this would not have had to be such a monstrously
>large changeset. But this is water under the bridge as far as
>I'm concerned. I've already gone through the code, I won't have
>time to do it again for 6-12 months, this is what you have to work
>with should you choose to.
>  
>
I don't want to drop the diseqc changes, and I don't know why you would 
say that. Granted, I don't have THAT much of a vested interest in the 
changes making it in -- it wouldn't be that big of a deal to maintain a 
separate patchset for my own use, and my needs are definitely currently 
fulfilled. It would be nice to not have to do that, and it would also be 
nice for other people to be able to benefit from the new functionality, 
but that's as far is it goes.

The file I'm concerned with could not be practically split up into small 
independent patches -- it's a new file, deriviative of another project 
of mine, ported to myth. The rest of the stuff could certainly have been 
split up more, but I'm not concerned with the rest of the files, just 
the changes to the diseqc tree implementation file -- as that's where 
the bugs have been introduced.

My problem is that you made so many different kinds of changes to that 
file in one single changeset, which has effectively blinded me. Saying 
it has something to do with the format of the original submission makes 
no sense. Even if I had submitted the patch as a single file only, it 
would have had no impact on whether you could commit first with a 
minimal set of syntax/style/etc. changes guaranteed to not break things 
first, and follow up with a changeset (or even better, multiple 
changesets) that gets it to the way you really like it. That way I could 
easily see what the important stuff was that was changed, we wouldn't be 
having these problems at all, and I would be perfectly happy even though 
the exact same changes had been made.

I'm not trying to be a jerk about this or anything, and I honestly don't 
mind the changes that have been made at all (I don't expect to agree 
with all or even any of them, and I'm fine with that) -- I was just 
really annoyed about the way the changes were applied to the branch, 
since it was done in such a way that is going to make getting everything 
working again that much harder.

I'm not saying that I shouldn't have split the original patch up more, 
but that has nothing to do with the way you went about committing the 
changes into the branch. But hey, if that's the way it is and there's no 
changing it, ok, we'll work with it.

>
>The problem is buggy tool-chains. We want to make it as easy possible
>to port MythTV to embedded platforms. These generally support a small
>subset of C++ features and proper delete behaviour is one I've been
>bit with personally on more than one embedded platform.
>  
>
Well, I'll take your word for that. I have quite a bit of experience 
working with various embedded C++ platforms, and while I've never 
encountered that particular problem, I've hit lots of other jaw-droppers 
-- generally speaking vendor-supplied compilers are quirky to the point 
of horribleness. Do you remember which toolchain this was specifically?

>
>Unless it is for some reason impossible we use QStrings, the performance
>overhead is nil in the real world. If you can show a profile that shows
>that tuning is 1% faster with C strings I'll reconsider.
>
>  
>
>>13) Generally changing things to favor compactness. I can't say I 
>>    
>>
>Readability is important. If the code had been in better shape to
>begin with, if it hadn't been using C strings for instance, I would
>have just left the code alone.
>
>  
>
>>14) Similarly, eliminating interim temporary values is IMO generally a 
>>    
>>
>Same answer as 13. 
>
>  
>
I honestly don't care about these changes per se -- I think both 
readability and maintainability has declined somewhat with the applied 
changes, but that's my personal opinion and I don't expect anybody in 
particular to agree with it, and have no desire to argue about it. 
Again, the only reason I care about the changes at all is because I 
can't see the changes I *do* care about (i.e. the ones that are breaking 
things) because they were all committed together.

>>15) A comment indicating that the QTime object that is used to determine 
>>    
>>
><snip>
>The comment is correct. I wouldn't expect you to know this because
>you probably looked at the QTime documentation like the rest of us
>and made the same mistake.
>  
>
Fair enough, I can only go by what the Qt docs say, and I don't have a 
lot of Qt experience. I hope somebody has filed a report with trolltech 
about that. In fact, I patterned the QTime usage off of another timer in 
another place in myth, so you know ... keep your eyes open.

>
>Depending on the type of state, it may or not be correct to put in
>in the DiSEqC tree. Without knowing what the future state is I
>would rather keep these stateless so bad future changes don't sneak
>up on me but stick out like sore thumbs.
>  
>
Sure, I fundamentally agree with that, but what I'm saying is that the 
code isn't generally finished -- we won't know what additional state, 
etc. the methods will need until people start using the new code with 
their various hardware and give reports about how this device needs 
such-and-such alternate behavior. Since it isn't finished, making 
changes like that is premature. Not bad necessarily, just premature. 
Again, it really doesn't matter to me (there's a good chance it should 
end up that way anyway) except that it's yet another change in the one 
big changeset that syntactically masks the few problematic changes that 
I want to be able to see.

>  
>
>>Things that are definitely bugs:
>>17) Deleting the diseqc tree object in dvbchannel. Not right, needs to 
>>be removed.
>>    
>>
>Already done. But this does indicate another problem.
>Why are you using global variables?
>Why are you passing around pointers to global variables?
>  
>
Various reasons. There were cases when I was developing where the 
dvbchannel object went out of scope and thus destroyed the tree object 
with it, losing state entirely and causing the diseqc devices to be 
reinitialized from scratch -- I didn't look into why that was: remember, 
I developed this on a few hours of weekend time over a few weeks. Also, 
tree creation is pretty expensive in terms of database access, so I 
thought having a synchronized cache of tree state objects would be 
helpful to have anyway. Also, if it will ever be desired to have 
multiple recorders/channel objects operating on a smaller set of 
devices, something like that would be required, or else each channel 
would have its own copy of the tree, with its own internal state, and 
the internal state will become unsynchronized with the external state.

There aren't any proper "global variables" involved, though of course 
the static member variable which manages the list of tree objects is 
globally scoped. I guess the short answer is that the state is global 
because the actual state of diseqc devices is global to the entire system.

>  
>
>>18) Changes to the device IDs to use unsigned ints. Ah, I see why some 
>>    
>>
>Make 0 the sentinel instead of -1.
>  
>
You misunderstood. The negative IDs are used to supply the GUI with 
unique node IDs for nodes which haven't yet been added to the database, 
so that it has something to call them. They are negative to avoid 
collisions with actual node IDs. It has nothing to do with sentinel 
values. There's nothing great about the use of negative IDs for that 
purpose, it's pretty hackish, but I didn't forsee the need for that at 
design time and had to add something to fix that quickly after testing 
revealed the problem. Feel free to change it to use another solution (I 
suggested a couple, and there's an infinite variety of others), but just 
changing it all to use unsigned ids is simply changing it back to the 
way it was when I originally designed it, i.e. reverting a bugfix.

>  
>
>>19) The switch SetChild() method no longer checks the supplied ordinal 
>>to see if it's in the range of the array, but instead tries to retrieve 
>>a child at that ordinal and fails if there is no child object. Of course 
>>this breaks the switch, since it means that you can't add child nodes 
>>unless there are already child nodes present. I imagine this is the 
>>source of the no-switch-children problem I noticed.
>>    
>>
>Sounds like a bug.
>  
>
Yeah, that one's easy, it just needs to change back to checking against 
the container size instead of checking for null. No biggie.

>>changeset as a whole right now -- since some of the functional changes 
>>are problematic in a way that indicates a failure to fully understand 
>>the new code, coupled with the fact that it's not straightforward at all 
>>    
>>
>Document better. The code is not going in to head until I understand it.
>  
>
Fair enough, but again I don't have a lot of time to apply to a free 
contribution, and if I was to apply a normal level of documentation to 
the code I would simply never be able to contribute anything. I was 
expecting that if you had questions you would ask me, or at least 
involve me in some way.

>  
>
>Have you considered what happens DVB recording gets extended to
>allow multiple recordings on the same multiplex to go on? While
>only one of the recorders will issue DiSEqC commands, the other
>one will want to query the state of DiSEqC. Especially if the 
>two recordings are being watched as LiveTV on separate frontends.
>  
>
I think that depends on how that feature will be implemented. There's 
not much state that can currently be queried from the diseqc objects 
anyway -- there's the rotor position (if any), and then there's the 
implicit state of whether the last diseqc setup sequence was successful 
or not. The particulars of a diseqc configuration is implicit in which 
source/input is currently in use, which is known at the channel level. 
Just knowing that the diseqc configuration was successful isn't enough 
anyway -- the second recorder would want to know that the tuning was 
completed successfully too, which is done separately from the diseqc 
setup. Logically, the diseqc setup is just another part of tuning.

Maybe if multi-card/same LNB/same rotor type recording is ever done the 
system will have to know more about the particulars of individual diseqc 
setups, but I didn't hear much interest in support for that (and it 
sounds like it could be potentially quite complex) -- for multiple 
recordings on the same multiplex there's no need I can see for any 
special diseqc state queries. Assuming there are two dvbchannel objects 
to support that, the one that doesn't have control of the frontend will 
have to be able to determine whether the one that does have control has 
successfully tuned yet (which isn't done by the diseqc stuff at all, but 
does imply a successful diseqc setup) -- so I would guess that it might 
be better to do that stuff at a higher level. Perhaps the signal monitor 
could be used for this purpose?