Opened 14 years ago

Closed 13 years ago

Last modified 12 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@… 14 years ago.
dvbdevtree.patch (71.7 KB) - added by yeasah@… 14 years ago.
New code for device tree (no integration with existing code)
dvbdevtree_gui.patch (34.6 KB) - added by yeasah@… 14 years ago.
New code for device tree GUI (no integration with existing code)
dvbdevtree_integration.patch (58.4 KB) - added by yeasah@… 14 years ago.
Patch containing integration of device tree
dvbdevtree_gui_integration.patch (27.9 KB) - added by yeasah@… 14 years ago.
Patch containing integration of device tree GUI
dvbdevtree_signalmonitor.patch (6.0 KB) - added by yeasah@… 14 years ago.
Signal monitor changes -- retuning and monitoring
dvbdevtree.2.patch (71.8 KB) - added by yeasah@… 13 years ago.
Updated dvbdevtree.cpp that fixes some voltage and rotor related inefficiencies
rotor-osd.patch (2.3 KB) - added by yeasah@… 13 years ago.
Rotor OSD Patch
bandstacked-lnb.patch (4.3 KB) - added by yeasah@… 13 years ago.
initial bandstacked patch
diseqc-switch-sql.patch (1.0 KB) - added by yeasah@… 13 years ago.
Fixes broken switch code -- a rename from subtype -> type was only partially completed.
diseqc-dvbs2.patch (1.8 KB) - added by yeasah@… 13 years ago.
conditional FE_GET_EXTENDED_INFO code was not updated for new diseqc
diseqc-raise-timeout.patch (546 bytes) - added by yeasah@… 13 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@… 13 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@… 13 years ago.
Raise the default diseqc command repeat to 1 repeat
diseqc-sigmon-rotor.patch (2.1 KB) - added by yeasah@… 13 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@… 13 years ago.
Fixes a couple problems with DVBChannel::Retune

Download all attachments as: .zip

Change History (63)

Changed 14 years ago by yeasah@…

Attachment: diseqc.patch5 added

comment:1 Changed 14 years ago by bander

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 Changed 14 years ago by yeasah@…

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 Changed 14 years ago by 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.

comment:4 in reply to:  3 Changed 14 years ago by anonymous

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 Changed 14 years ago by danielk

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 Changed 14 years ago by bander.ajba@…

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,

comment:7 in reply to:  5 Changed 14 years ago by anonymous

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?

Changed 14 years ago by yeasah@…

Attachment: dvbdevtree.patch added

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

Changed 14 years ago by yeasah@…

Attachment: dvbdevtree_gui.patch added

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

Changed 14 years ago by yeasah@…

Patch containing integration of device tree

Changed 14 years ago by yeasah@…

Patch containing integration of device tree GUI

Changed 14 years ago by yeasah@…

Signal monitor changes -- retuning and monitoring

comment:8 Changed 13 years ago by bander.ajba@…

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

comment:9 in reply to:  8 Changed 13 years ago by anonymous

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!

Changed 13 years ago by yeasah@…

Attachment: dvbdevtree.2.patch added

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

comment:10 Changed 13 years ago by danielk

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

comment:11 Changed 13 years ago by 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.

comment:12 in reply to:  11 Changed 13 years ago by yeasah@…

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 Changed 13 years ago by yeasah@…

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 Changed 13 years ago by danielk

(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 Changed 13 years ago by danielk

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

comment:16 Changed 13 years ago by danielk

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

comment:17 Changed 13 years ago by danielk

(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 Changed 13 years ago by yeasah@…

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.

Changed 13 years ago by yeasah@…

Attachment: rotor-osd.patch added

Rotor OSD Patch

comment:19 Changed 13 years ago by danielk

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

comment:20 Changed 13 years ago by danielk

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

comment:21 Changed 13 years ago by danielk

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

comment:22 Changed 13 years ago by danielk

(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.

Changed 13 years ago by yeasah@…

Attachment: bandstacked-lnb.patch added

initial bandstacked patch

comment:23 Changed 13 years ago by yeasah@…

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.

Changed 13 years ago by yeasah@…

Attachment: diseqc-switch-sql.patch added

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

comment:24 Changed 13 years ago by Janne <janne-mythtv@…>

Component: mythtvdvb

Changed 13 years ago by yeasah@…

Attachment: diseqc-dvbs2.patch added

conditional FE_GET_EXTENDED_INFO code was not updated for new diseqc

Changed 13 years ago by yeasah@…

Attachment: diseqc-raise-timeout.patch added

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

Changed 13 years ago by yeasah@…

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)

Changed 13 years ago by yeasah@…

Attachment: diseqc-repeat.patch added

Raise the default diseqc command repeat to 1 repeat

Changed 13 years ago by yeasah@…

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 Changed 13 years ago by yeasah@…

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 Changed 13 years ago by danielk

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

comment:27 Changed 13 years ago by danielk

(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 Changed 13 years ago by danielk

(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 Changed 13 years ago by danielk

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

comment:30 Changed 13 years ago by danielk

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

comment:31 Changed 13 years ago by danielk

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

comment:32 Changed 13 years ago by danielk

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 Changed 13 years ago by danielk

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

comment:34 Changed 13 years ago by Mark.Buechler@…

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

comment:35 in reply to:  34 Changed 13 years ago by yeasah@…

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?() ...

Changed 13 years ago by yeasah@…

Attachment: retune.patch added

Fixes a couple problems with DVBChannel::Retune

comment:36 Changed 13 years ago by yeasah@…

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 Changed 13 years ago by 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.

comment:38 in reply to:  37 Changed 13 years ago by anonymous

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 Changed 13 years ago by danielk

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

comment:40 Changed 13 years ago by danielk

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

comment:41 Changed 13 years ago by danielk

(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 Changed 13 years ago by danielk

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 Changed 13 years ago by danielk

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

comment:44 Changed 13 years ago by danielk

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

comment:45 Changed 13 years ago by danielk

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

comment:46 Changed 12 years ago by Janne Grunau

(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 Changed 12 years ago by Janne Grunau

(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.