[mythtv] [mythtv-commits] Ticket #1945: DVB-S/diseqc patch
danielk at cuymedia.net
Sun Jul 2 20:48:30 UTC 2006
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.
> Things I agree with, or at least don't have a major problem with:
> 6) Changing from STL containers to Qt containers -- they are similar
> enough that it's not a big deal, and there's plenty of STL in the
> application already, so I figured it was ok. But I can understand the
> desire to not use two different implementations of the same thing.
I prefer STL containers myself, but try to use the Qt ones where
they are close enough to the real thing. We want the code to be
maintainable which means we try to keep down the number of libraries
someone looking at the code for the first time needs to know.
> 9) Changing the default source id on the tune methods to be zero instead
> of -1 -- fine, I figured it would be better to use a value that was
> entirely out of the range of the database field, but autoincrement won't
> ever cough up a zero normally so I suppose it's just as good (and then
> it can use a uint for the ID like it should) -- OTOH if somebody ever
> makes myth multi-database something like that could be a problem.
Multi-database is pretty much dead unless someone can find a linkable
one with decent performance.
> 10) Locking added to the signal monitor stuff. Great, I wasn't sure
> exactly what was required in the signal monitor for thread safety.
I wouldn't expect you to know this, this type of thing is what I'm
usually looking for when I review a patch.
> 11) In general checking if a pointer is NULL before deleting it. I
> dislike that, just because it perpetuates the common misconception that
> it is unsafe to delete a null pointer, and explicitly refuses to use a
> feature of the language that was put there for that exact purpose. But
> it's harmless, so I don't really have a problem 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.
> Things that I don't really agree with -- I'm not particularly happy with
> some of these changes, but I'm not about to argue them either, as it's
> not my project:
> 12) Changing the database identifier lookup functions to use arrays of
> QStrings instead of const ascii strings. That one baffles me -- it's
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.
> 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.
> 16) Moving two of the diseqc ioctl methods that previously resided in
> the main tree class into static module functions. I definitely don't
> agree with that. Currently it is certainly possible to do that, since
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.
> 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?
> 18) Changes to the device IDs to use unsigned ints. Ah, I see why some
Make 0 the sentinel instead of -1.
> 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.
> 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.
> Regarding thread safety, I think the most important thing is that the
> tree be locked for the lifetime of a retrieved rotor object (when the
> signal monitor requests the rotor from the channel class and uses it to
> update its values), which it currently isn't -- beyond that, the tree
> itself could easily be locked when it is in use -- I think that's not
> strictly necessary given the current usage patterns (if you have two
> dvbchannel objects trying to use the same device at the same time you've
> got other problems, and I assume that should never happen), but it would
> be a trivial addition so it's probably a good idea whether it's strictly
> needed or not -- to avoid failure cascades. As far as I remember, the
> other objects are either definitively single-threaded or already locked.
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.
More information about the mythtv-dev