Opened 19 years ago

Closed 19 years ago

Last modified 18 years ago

#1945 closed enhancement (fixed)

DVB-S/diseqc patch

Reported by: yeasah@… Owned by: danielk
Priority: minor Milestone: 0.20
Component: dvb Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Includes various improvements, including:

  • Full diseqc tree code and GUI -- allows arbitrary diseqc configurations. New diseqc code.
  • LNB configuration is done by common presets (or optional custom mode that lets you directly set LOFs/etc.)
  • Retune on HAS_LOCK timeout for cards that lack the FE_CAN_RECOVER capability.
  • Rotor progress monitoring in signalmonitor, retune on rotation complete.
  • Raises voltage temporarily for duration of rotation to increase rotor speed.
  • Fix Hz/KHz tuning bug.
  • Always tune with FEC_AUTO if card can do it.
  • Move DVB options in the input configuration wizard that affect scanning above the scan button (previously on second page)
  • Some DVB-S fixes in siscan.cpp (but there's a long way to go there)
  • Change DVB-S+diseqc fixed-number-of-phantom-inputs scheme to instead allow the ad-hoc creation of new inputs (since there is no longer a bounded number of possible input configurations, the old scheme will not work anymore)
  • Coincidentally fixed bug in ListBoxSetting that prevented multiple strings from being present in the list with the same displayed name but different values.

There's some database update code that needs to be integrated; right now it's just called every time and the code works out whether the upgrade is needed.

I've been running with this code for some time, and it works well for me, but of course there is a wide variety of devices out there and it's likely it will need adjustment in some cases. Generally the new diseqc code attempts to be as efficient as possible and includes a minimum of device-workaround-hacks that make the command latency longer, but some of those may need to be added back in at some point.

Attachments (16)

diseqc.patch5 (198.5 KB ) - added by yeasah@… 19 years ago.
dvbdevtree.patch (71.7 KB ) - added by yeasah@… 19 years ago.
New code for device tree (no integration with existing code)
dvbdevtree_gui.patch (34.6 KB ) - added by yeasah@… 19 years ago.
New code for device tree GUI (no integration with existing code)
dvbdevtree_integration.patch (58.4 KB ) - added by yeasah@… 19 years ago.
Patch containing integration of device tree
dvbdevtree_gui_integration.patch (27.9 KB ) - added by yeasah@… 19 years ago.
Patch containing integration of device tree GUI
dvbdevtree_signalmonitor.patch (6.0 KB ) - added by yeasah@… 19 years ago.
Signal monitor changes -- retuning and monitoring
dvbdevtree.2.patch (71.8 KB ) - added by yeasah@… 19 years ago.
Updated dvbdevtree.cpp that fixes some voltage and rotor related inefficiencies
rotor-osd.patch (2.3 KB ) - added by yeasah@… 19 years ago.
Rotor OSD Patch
bandstacked-lnb.patch (4.3 KB ) - added by yeasah@… 19 years ago.
initial bandstacked patch
diseqc-switch-sql.patch (1.0 KB ) - added by yeasah@… 19 years ago.
Fixes broken switch code -- a rename from subtype -> type was only partially completed.
diseqc-dvbs2.patch (1.8 KB ) - added by yeasah@… 19 years ago.
conditional FE_GET_EXTENDED_INFO code was not updated for new diseqc
diseqc-raise-timeout.patch (546 bytes ) - added by yeasah@… 19 years ago.
raise ioctl retry delay and max retry count for some truly horrible DVB cards
diseqc-cleanup.patch (10.7 KB ) - added by yeasah@… 19 years ago.
Cleans up some bad indents, the large integer constant and static function warnings, and factors out a couple bit of common code. (the last part coincidentally fixes a problem with the rotor time estimate)
diseqc-repeat.patch (408 bytes ) - added by yeasah@… 19 years ago.
Raise the default diseqc command repeat to 1 repeat
diseqc-sigmon-rotor.patch (2.1 KB ) - added by yeasah@… 19 years ago.
I like the idea of having the alterable rotor target position (so it can just use the WaitForPos flag like a normal signal monitor value), but the rotor monitor code wasn't quite right. This should fix it, and hopefully improve the clarity of the code.
retune.patch (1.3 KB ) - added by yeasah@… 19 years ago.
Fixes a couple problems with DVBChannel::Retune

Download all attachments as: .zip

Change History (63)

by yeasah@…, 19 years ago

Attachment: diseqc.patch5 added

comment:1 by bander, 19 years ago

I have tried the patch against revision 10190. when I try to configure the disecq in mythtv-setup, I lose the changes when I exit. why the database does not save the changes. am I doing something wrong

comment:2 by yeasah@…, 19 years ago

I was wondering if this was going to prove confusing. What you're probably doing is exiting out of the capture card configuration page by hitting ESC (or otherwise doing a cancel at that level), which cancels the changes you've made.

It might not be so confusing if it wasn't the case that the only way to get out of the tree editor is via ESC, but that's the way all the other full-page list views work in mythtv-setup (since you want ACCEPT to function as an alternative edit node function)

Right now to successfully apply the change you have to ESC from the tree editor, ACCEPT the card, and then ESC from the card list (since that's another one of those lists where ESC is the only way out)

It could easily changed to always save on exit from the tree dialog, but then there's no way to cancel your changes. I'm certainly open to suggestions here.

comment:3 by anonymous, 19 years ago

I have tried all combination, still could not make it save changes. probably It will work if I understand what you meant by "ACCEPT the card, and then ESC from the card list".

these are the steps I'm following: 1- enter the tree editor, once I finish I hit ESC 2- I just Hit Enter to exit the capture card screen.

this patch is so important to me. because, you are implementing the retune function again, after it was removed from the revision 10132 on wards.

for some reason after the retune hack was removed from the mentioned revision my cards started to loop on a Partial Lock errors (L_s) no (LMS) any more.

so I need to get the tree editor to save and then I can test your patch.

in reply to:  3 comment:4 by anonymous, 19 years ago

Replying to anonymous:

I have tried all combination, still could not make it save changes. probably It will work if I understand what you meant by "ACCEPT the card, and then ESC from the card list".

these are the steps I'm following: 1- enter the tree editor, once I finish I hit ESC 2- I just Hit Enter to exit the capture card screen.

this patch is so important to me. because, you are implementing the retune function again, after it was removed from the revision 10132 on wards.

for some reason after the retune hack was removed from the mentioned revision my cards started to loop on a Partial Lock errors (L_s) no (LMS) any more.

so I need to get the tree editor to save and then I can test your patch.

Hmm, those steps should work, so it sounds like something is amiss that I haven't seen before, perhaps a problem with the new database tables being added. Feel free to contact me directly ( yeasah@… ), it might be quicker to debug that way.

Do you get any interesting messages from the mythtv-setup console log when editing the tree (especially when trying to save it, which happens at the time you accept the capture card editor screen)?

It would also be interesting to know what DVB-S card you have, since I've only enabled retuning for cards that lack the FE_CAN_RECOVER capability (though all cards get a retune upon completion of any rotor movement)

Thanks!

comment:5 by danielk, 19 years ago

Milestone: 0.20
Version: head

Yesah, can you break this up into smaller pieces.

Preferably, the size of each patch should be something that I can read through an understand in about 10 minutes. Also, each change in MythTV behaviour should have it's own patch.

Some parts need to be discussed, for instance:

  • Always tune with FEC_AUTO if card can do it.

This is much slower than explicit tuning parameters for some cards, if we really need this feature, it needs to be optional.

comment:6 by bander.ajba@…, 19 years ago

hello,

my card is twinhan vp 1030 w/CI, I have also SkyStarII which also behaves the same. I will try to test it again tomorrow to see what mythtv-setup spits out and I will email it to you if I see something not right. but right now I'm watching my team playing in the WC.

regards,

in reply to:  5 comment:7 by anonymous, 19 years ago

Replying to danielk:

Yesah, can you break this up into smaller pieces.

Preferably, the size of each patch should be something that I can read through an understand in about 10 minutes. Also, each change in MythTV behaviour should have it's own patch.

Yeah, I know it's kind of annoyingly monolithic at this point. There's definitely stuff that can be split off -- basically anything that isn't actually new code, including the potentially controversial stuff like the retuning or FEC_AUTO, so I can split that off for sure. But most of the complexity is in 2 totally new files (dvbdevtree.cpp/h, which contains all the new diseqc tree code, and dvbdevtree_cfg.cpp/h, which contains the GUI for the same), and I don't know how I would split up code that is all new -- it's a cohesive whole.

Some parts need to be discussed, for instance:

  • Always tune with FEC_AUTO if card can do it.

This is much slower than explicit tuning parameters for some cards, if we really need this feature, it needs to be optional.

On both of my cards FEC_AUTO is just as fast, so I thought it might be nice, but if it's going to reduce tuning performance I agree it should probably not be included, certainly not by default.

The reason I thought it might be nice is because there's at least one instance of FEC being wrong in transmitted NITs. Another solution would be to attempt to tune at scan time with FEC_AUTO, and take note of the actual FEC, but some cards that are FEC_AUTO capable just report back FEC_AUTO as the fec type after tuning, so that isn't 100% either.

I'll split it up as much as I can and post the patches -- it will likely take the form of the new dvbdevtree files as a single patch (without any changes to the existing code), and then a series of other patches that contain the various other DVB-S fixes and also the patch that integrates the new dvbdevtree stuff with everything else.

Sound good?

by yeasah@…, 19 years ago

Attachment: dvbdevtree.patch added

New code for device tree (no integration with existing code)

by yeasah@…, 19 years ago

Attachment: dvbdevtree_gui.patch added

New code for device tree GUI (no integration with existing code)

by yeasah@…, 19 years ago

Patch containing integration of device tree

by yeasah@…, 19 years ago

Patch containing integration of device tree GUI

by yeasah@…, 19 years ago

Signal monitor changes -- retuning and monitoring

comment:8 by bander.ajba@…, 19 years ago

I looked at the mythtv-setup verbose and it seems that the database table dtv_device_tree does not exists. I also checked mythconverg if it has this table but the table is not there. can you post the sql statement to create this table.

regards,

bander

in reply to:  8 comment:9 by anonymous, 19 years ago

Replying to bander.ajba@gmail.com:

I looked at the mythtv-setup verbose and it seems that the database table dtv_device_tree does not exists. I also checked mythconverg if it has this table but the table is not there. can you post the sql statement to create this table.

regards,

bander

The table creation statements are in a function called DatabaseDiseqcUpgrade() in either dvbdevtree_cfg.cpp or dvbdevtree.cpp (I moved it in the most recent split patch to facilitate the split, it didn't really belong in the GUI file) -- it should be getting called automatically in dbcheck.cpp every time you run the app right now, but it obviously isn't working. If you determine why it's failing, please let me know and I'll update the patch.

Thanks!

by yeasah@…, 19 years ago

Attachment: dvbdevtree.2.patch added

Updated dvbdevtree.cpp that fixes some voltage and rotor related inefficiencies

comment:10 by danielk, 19 years ago

(In [10305]) Refs #1945. Creating a branch to merge in Yeasah's DiSEqC patches for testing.

comment:11 by danielk, 19 years ago

(In [10322]) Refs #1945. Initial commit of new DiSEqC code to diseqc branch.

Yeasah, can you have a look over this and make sure it still works? I removed some code that looked like unrelated fixes. If they are related please generate a patch against this (w/explanation), if not generate a patch and ticket for each one against SVN head.

in reply to:  11 comment:12 by yeasah@…, 19 years ago

Replying to danielk:

(In [10322]) Refs #1945. Initial commit of new DiSEqC code to diseqc branch.

Yeasah, can you have a look over this and make sure it still works? I removed some code that looked like unrelated fixes. If they are related please generate a patch against this (w/explanation), if not generate a patch and ticket for each one against SVN head.

I'll give it a test when I can, but I did notice a few things just going over the changeset, I think at the very least the last two will have to be corrected in one way or another before the stuff will work properly:

  • The always-use-auto-FEC thing is still in there, I never did take that out (sorry about that) -- line 749 of dvbchannel.cpp
  • The couple changes that you left out from siscan.cpp are indeed not related to the diseqc patch (DVB-S scanning fixes that leaked into the patch), so that's fine.
  • The changes to libmyth/settings.cpp+h are needed for the device tree editor to work properly. The change allows the myth listbox to have multiple strings with the same displayed value, which is needed for the GUI since the tree display is actually just an indented listbox -- and tree nodes can easily have the same name.
  • The code added to dvbchannel.cpp to delete the diseqc_tree object will cause crashes with the current implementation -- the lifetimes of tree objects is currently managed by the diseqcdevtrees class, so deleting the object in dvbchannel will leave an outstanding reference to a deleted object, which will either be reused or re-deleted at some future point.

On an unrelated note, if it's easy to take the new rotor position status signal monitor value and stick it in the OSD, that would be a really nice addition to this patch. I have a pretty rough understanding of the signal monitor code, so I didn't want to do much more to that stuff than was necessary in the patch.

comment:13 by yeasah@…, 19 years ago

I had a chance to try out the branch (with the two fixes I mentioned before to the changes that were made on import), and there are other problems that have been added to the diseqc code itself. It doesn't see and doesn't let you add switch children, for example.

It probably would have been pretty easy to find the problems if the code hadn't had so many syntax and other non-functional changes along with some real functionality changes -- it makes it really hard to see the few places you changed the code materially when so much of the code has been touched. It would have been much better to have the initial import be applied without any functionality changes (just renames and simple transformations like that), so that we had baseline working functionality, and then functional changes applied on top (in another changeset) so they could be easily recognized and considered.

Or just follow another common approach to open source patch contribution -- ask the original author to make any changes. That way you avoid breaking stuff because you might not fully understand the code, and the amount of time you spend handling externally sourced code goes down.

Anyway -- I wouldn't mind tracking down the new problems, but I would need to see the functional changes as a separate changeset from the non-functional changes, so that I can actually see something useful in terms of what is different from my original code.

comment:14 by danielk, 19 years ago

(In [10362]) Refs #1945. Fix from Yeasah for a bug in ListBoxSetting causing problems when different values have the same description which can unfortunately happen with automatically filled lists.

This changes the binary revision so plugins must be relinked.

comment:15 by danielk, 19 years ago

(In [10364]) Refs #1945. Don't delete diseqc_tree in DVBChannel, it is a pointer to a global variable.

comment:16 by danielk, 19 years ago

(In [10367]) Refs #1945. Cleans up db tables in DiSEqC patch.

comment:17 by danielk, 19 years ago

(In [10368]) Refs #1945. Fixes DiSEqC settings code problems.

  • This restores the behaviour of SetChild() so that new nodes can be inserted.
  • This Changes "\t" to " " for indentation of the tree.
  • This adds a IsRealDeviceID() call to DiSEqCDevDevice, so that fake deviceid fields can be detected. At the moment this is a hack dependent on a 32 bit integer size, but I'll commit a better fix later.

comment:18 by yeasah@…, 19 years ago

It's looking good now, it seems to be working again by my testing. Here's an additional small patch that adds OSD display for the rotor position.

by yeasah@…, 19 years ago

Attachment: rotor-osd.patch added

Rotor OSD Patch

comment:19 by danielk, 19 years ago

(In [10394]) Refs #1945. Applies rotor position UI patch with some modifications.

comment:20 by danielk, 19 years ago

(In [10400]) Refs #1945. Merges r10362:10399 from svn head to diseqc branch.

comment:21 by danielk, 19 years ago

(In [10401]) Refs #1945. Some DVBChannel cleanups.

comment:22 by danielk, 19 years ago

(In [10403]) Refs #1945. Renames in diseqcsettings.{h,cpp} and some changes in videosource to minimize the size of the patch with respect to SVN head.

by yeasah@…, 19 years ago

Attachment: bandstacked-lnb.patch added

initial bandstacked patch

comment:23 by yeasah@…, 19 years ago

Added bandstacking support based on Mark Buechler's description and patch -- I was still waiting for some final clarification from Mark, but as I haven't gotten a reply yet and I see that Daniel's working on the settings stuff, I figured I'd get this into the mix sooner rather than later.

by yeasah@…, 19 years ago

Attachment: diseqc-switch-sql.patch added

Fixes broken switch code -- a rename from subtype -> type was only partially completed.

comment:24 by Janne <janne-mythtv@…>, 19 years ago

Component: mythtvdvb

by yeasah@…, 19 years ago

Attachment: diseqc-dvbs2.patch added

conditional FE_GET_EXTENDED_INFO code was not updated for new diseqc

by yeasah@…, 19 years ago

Attachment: diseqc-raise-timeout.patch added

raise ioctl retry delay and max retry count for some truly horrible DVB cards

by yeasah@…, 19 years ago

Attachment: diseqc-cleanup.patch added

Cleans up some bad indents, the large integer constant and static function warnings, and factors out a couple bit of common code. (the last part coincidentally fixes a problem with the rotor time estimate)

by yeasah@…, 19 years ago

Attachment: diseqc-repeat.patch added

Raise the default diseqc command repeat to 1 repeat

by yeasah@…, 19 years ago

Attachment: diseqc-sigmon-rotor.patch added

I like the idea of having the alterable rotor target position (so it can just use the WaitForPos flag like a normal signal monitor value), but the rotor monitor code wasn't quite right. This should fix it, and hopefully improve the clarity of the code.

comment:25 by yeasah@…, 19 years ago

6 new patches added to the diseqc ticket, mostly bugfixes, but also some tweaks on the diseqc command parameters and a little code cleanup as well.

comment:26 by danielk, 19 years ago

(In [10448]) Refs #1945. Applies diseqc-dvbs2.patch, updates DVB-S2 code to work with new DiSEqC code. (Adds modulation param).

comment:27 by danielk, 19 years ago

(In [10449]) Refs #1945. Applies diseqc-raise-timeout.patch, increases the timeouts and retry counts to cope with some "truly horrible DVB cards".

comment:28 by danielk, 19 years ago

(In [10450]) Refs #1945. Applies diseqc-repeat.patch, this increases the default DiSEqC command repeat from 0 to 1. (i.e. this is after the repeats on individual ioctls increased in changeset [10449].)

comment:29 by danielk, 19 years ago

(In [10451]) Fixes #1990. Refs #1945. Adds initial support for Bandstacked LNBs.

comment:30 by danielk, 19 years ago

(In [10452]) Refs #1945. Applies diseqc-cleanup.patch, miscellaneous code cleanups.

comment:31 by danielk, 19 years ago

(In [10453]) Refs #1945. Applies (most of) diseqc-sigmon-rotor.patch, this fixes some problems with the rotor position update code.

comment:32 by danielk, 19 years ago

Yeasah, I believe I applied all the patches.

There was a small compile problem with the cleanup patch, just different scoping with gcc 3.4.6 (older?)

I also didn't apply the WaitForSDT removal in the rotor patch. This is a good idea as an optimization, but it needs to be optional. With some of the more broken DVB drivers/devices you need to wait for the SDT even without a rotor.

comment:33 by danielk, 19 years ago

(In [10466]) Refs #1945. Merges r10399:10465 from svn head to diseqc branch.

comment:34 by Mark.Buechler@…, 19 years ago

Looking at the bandstack patch, I don't see anywhere where the voltage is being overridden to 18 volts.

in reply to:  34 comment:35 by yeasah@…, 19 years ago

Replying to Mark.Buechler@gmail.com:

Looking at the bandstack patch, I don't see anywhere where the voltage is being overridden to 18 volts.

Take a look at DiSEqCDevLNB::GetVoltage() ...

by yeasah@…, 19 years ago

Attachment: retune.patch added

Fixes a couple problems with DVBChannel::Retune

comment:36 by yeasah@…, 19 years ago

Added patch that fixes issues with DVBChannel::Retune() as reported by Mark Buechler. Mark reports that the patch seems to fix the symptoms. Explanation of patch below.

Yeasah Pell wrote:

I think this is a problem with DVBChannel::Retune(). There's two problems, really.

#1 DVBChannel::prev_tuning isn't fully assigned after a successful tune (only the params part), so the polarity isn't retained.

#2 prev_tuning is improperly being used as "the last desired tuning state", but it's actually "the last successful tuning state". cur_tuning is more properly the last desired state, but it isn't updated when Tune() is called directly from outside of DVBChannel. So an assignment added to the Tune() method takes care of that, and Retune() can then use cur_tuning as it should.

comment:37 by danielk, 19 years ago

Yeasah, can you do this without the "cur_tuning = tuning" in Tune()? It is a bit confusing since a lot of the tuning functions actually call Tune() with cur_tuning as the first paramater. It is really intended as a temporary variable used in calculating the tuning parameters so it often in an inconsistent state.

in reply to:  37 comment:38 by anonymous, 19 years ago

Replying to danielk:

Yeasah, can you do this without the "cur_tuning = tuning" in Tune()? It is a bit confusing since a lot of the tuning functions actually call Tune() with cur_tuning as the first paramater. It is really intended as a temporary variable used in calculating the tuning parameters so it often in an inconsistent state.

If cur_tuning isn't appropriate as the desired tuning state either, the most obvious solution would be to add yet another DVBTuning member variable (called desired_tuning?) which is assigned in the same place as the "cur_tuning = tuning" assignment you're talking about, and have Retune() use that instead of cur_tuning.

I think it would be good to eliminate cur_tuning if possible if it's just used to ferry temporary state around -- it looks like it's just used to propagate tuning info back from ParseTuningParams via GetTransportOptions, but I think those could just as easily modify a supplied tuning value argument and avoid the need to store them in class state. Maybe I missed somewhere that the cur_tuning state is needed across public method calls, but I didn't see anything like that.

comment:39 by danielk, 19 years ago

(In [10491]) Refs #1945. A little cleanup of fake diseqcid handling.

comment:40 by danielk, 19 years ago

(In [10492]) Refs #1945. DVBChannel cleanup, gets rid of cur_tuning from class state.

comment:41 by danielk, 19 years ago

(In [10493]) Refs #1945. Applies modified retune patch.

The Retune() function was using prev_tuning for retuning, but this only gets set when the hardware tells us the tuning has been successful. This adds another variable, desired_tuning, which gets set whether or not the tuning has been successful the first time around, and uses this for retuning.

comment:42 by danielk, 19 years ago

Resolution: fixed
Status: newclosed

(In [10585]) Fixes #1945. Merges DiSEqC branch to SVN head.

  • Implements a tree based DiSEqC configuration method.
  • Properly compares DVB-S frequencies (which are stored in the DB in kHz not Hz).
  • Reimplements Retune() code in a more correct fashion.
  • Removes temporary cur_tuning from class state (for more dependable re-tuning).

comment:43 by danielk, 19 years ago

(In [10588]) Refs #1945. Fixes compilation of diseqc.cpp on systems without DVB headers.

comment:44 by danielk, 19 years ago

(In [10591]) Refs #1945. Adds inttypes include to diseqc.h ...

comment:45 by danielk, 19 years ago

(In [10611]) Refs #1945. Fixes #2074. Allows adding non DVB-S DVB recorders again...

comment:46 by Janne Grunau, 18 years ago

(In [16457]) Fix horizontal transponders on dishnet SW21 legacy switches

Before the diseqc branch merge [10585] we did this on all legacy switches but since nobody complained since 0.20 for the other types I'll fix it only for SW21.

Refs #1945, #4493

comment:47 by Janne Grunau, 18 years ago

(In [16458]) Merges revision [16457] from trunk: Fix horizontal transponders on dishnet SW21 legacy switches

Before the diseqc branch merge [10585] we did this on all legacy switches but since nobody complained since 0.20 for the other types I'll fix it only for SW21.

Refs #1945, #4493

Note: See TracTickets for help on using tickets.