[mythtv] [mythtv-commits] Ticket #1945: DVB-S/diseqc patch
yeasah at schwide.com
Sun Jul 2 23:48:05 UTC 2006
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
>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
>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?
More information about the mythtv-dev