Summary:	ASTERISK-22429: [patch] - chan_dahdi allows for updating both hw and sw gains, but dahdi show channel doesn't reflect changes
Reporter:	Jaco Kroon (jkroon)	Labels:
Date Opened:	2013-08-29 14:37:40	Date Closed:	2013-10-14 17:58:01
Priority:	Minor	Regression?
Status:	Closed/Complete	Components:	Channels/chan_dahdi
Versions:	11.5.0	Frequency of Occurrence	Constant
Related Issues:
Environment:		Attachments:	( 0) asterisk-11.5.0-dahdi_gain_display.diff ( 1) jira_asterisk_22429_hwgain_trunk.patch ( 2) jira_asterisk_22429_hwgain_v1.8.patch ( 3) jira_asterisk_22429_v1.8_v2.patch ( 4) jira_asterisk_22429_v1.8.patch ( 5) jira_asterisk_22429_v11.patch
Description:	I think some output explains it best: # asterisk -Rx "dahdi show channel 3" | grep Gains Gains (RX/TX): 8.00/6.00 # asterisk -rx "dahdi set swgain rx 3 4.0" software rx gain set to 4.0 on channel 3 # asterisk -Rx "dahdi show channel 3" | grep Gains Gains (RX/TX): 8.00/6.00 So in spite of the gain having being modified the change is not reflected.  There is also no indication whether the displayed gains is the hw or sw gains. I've written a small patch to rectify the situation and improve it somewhat: # asterisk -Rx "dahdi show channel 3" | grep Gains SW Gains (RX/TX): 8.00/6.00 HW Gains (RX/TX): 0.00/0.00 # asterisk -rx "dahdi set swgain rx 3 4.0" software rx gain set to 4.0 on channel 3 # asterisk -Rx "dahdi show channel 3" | grep Gains SW Gains (RX/TX): 4.00/6.00 HW Gains (RX/TX): 0.00/0.00 There is a secondary (and actually more significant) problem here: The hw gains are not initialized during startup/reload at all (at least, not as far as I can tell).  And if they're already set during reload/restart the only way that I can see to get them back to some pre-configured value is to explicitly reset them using "dahdi set hwgain".  This patch assumes that they are zeroed during startup/reload (which can be false).
Comments:	By: Matt Jordan (mjordan) 2013-09-12 19:04:37.477-0500 Thanks for the patch Jaco! By: Richard Mudgett (rmudgett) 2013-10-08 18:22:32.680-0500 After looking at the gains code in chan_dahdi, the hwgains should never be used. 1) chan_dahdi only deals with hwgains in the CLI "dahdi set hwgain" command. 2) The DAHDI modules only support setting hardware gain values. 3) Not all hardware supports hardware gains.  I only see one DAHDI driver module supporting it. By: Jaco Kroon (jkroon) 2013-10-09 06:43:16.252-0500 Richard, I can appreciate that, points 2 and 3 seems conflicting however (possibly grammer in 2 that's tripping me up).  I assume you're referring to these parameters in the wctdm24xxp module: parm:           fxotxgain:int parm:           fxorxgain:int parm:           fxstxgain:int parm:           fxsrxgain:int This is not a granular per channel setting. Either which way - either the control of hwgains should be removed entirely, or at least the patch needs to be applied, but preferably more needs to be done in order to allow setting hwgain for those cards that support it from the config file too. The ability to set the hw gains on a per channel basis is definitely useful in my personal opinion and I'd rather see the functionality *added* to asterisk than removing the hwgain functionality entirely.  But that's just me. By: Richard Mudgett (rmudgett) 2013-10-09 11:43:43.055-0500 {quote} 2) The DAHDI modules only support setting hardware gain values. {quote} Rephrasing and expanding point 2: For DAHDI modules that support hardware gains, only setting hardware gain values are supported.  Reading the current value back is not supported.  Asterisk therefor cannot determine what is set as the current value for reporting purposes or relative gain adjustments. {quote} parm: fxotxgain:int parm: fxorxgain:int parm: fxstxgain:int parm: fxsrxgain:int {quote} The gain parameters you are pointing out are configured on DAHDI itself using modprobe.  e.g, To set the FXO port tx gain to -3.5 dB: {noformat} modprobe wctdm24xxp fxotxgain=-35 {noformat} The modprobe setting is global to the driver. Asterisk uses ioctl(DAHDI_SET_HWGAIN) to set the hardware gains in chan_dahdi.c:dahdi_set_hwgain().  The DAHDI_SET_HWGAIN method seems to have per channel control.  However, the DAHDI driver still needs hardware to support it.  Only the analog port cards seem to have the support.  T1/E1 cards do not support this feature even though their channels can be setup as analog ports. By: Richard Mudgett (rmudgett) 2013-10-09 11:52:20.504-0500 [^jira_asterisk_22429_v1.8.patch] and [^jira_asterisk_22429_v11.patch] - These patches mostly update the hwgain and swgain documentation.  I did keep the code saving the swgain changes so the gain will reflect in the CLI "dahdi show channel" output. By: Jaco Kroon (jkroon) 2013-10-10 02:38:15.318-0500 Personally I don't agree with the patch but I can see the merit. Since it isn't possible to query the hwgain values - isn't it better to rip out the hwgain stuff completely rather than to just try and document the problem away?  Or alternatively, to finish it (which shouldn't be hard - other than querying the initial hwgain values in the case where no hwgain is set) properly? Is there *any* advantage (even theoretically) to use hwgain over swgain (eg, clarity of signal, improved sampling etc ...), because if so, then I really think we should try and fix the feature rather than document it away.  If there truly is no advantage then let's rather remove the code completely.  I'd be willing to attempt patches on the 11.X branch if you're interesting in fixing - but I'll need some help on querying the values from the driver if that's even possible.  There are a few possible workarounds (read: sub-optimal solutions) here: 1.  If no value is set in the config - read the values from /sys/ somewhere (probably not portable) of what the module was loaded with and force-set it to those values so that we know for sure. 2.  If we can't determine module load values - force set it to 0.0dB 3.  Keep an internal "unknown" flag and just in the CLI output say that we don't know what the hwgain is set to. We need some way of determining whether hwgain is even supported by the card, and I suspect trying to set it is the only way to do that, so ideally option 1 or 2 would be better.  If we can query it, surely if we can't set it then trying to query should also return some appropriate errno that we can use to determine "hwgain not supported" on the channel (and set an appropriate flag on the channel structure). By: Richard Mudgett (rmudgett) 2013-10-10 11:22:48.923-0500 {quote} Since it isn't possible to query the hwgain values - isn't it better to rip out the hwgain stuff completely rather than to just try and document the problem away? Or alternatively, to finish it (which shouldn't be hard - other than querying the initial hwgain values in the case where no hwgain is set) properly? {quote} I can only see ripping it out on trunk and not on any released branches.  However, ripping it out will not save much since it is only the CLI command that would be removed and it does no harm leaving it in.  Someone may actually have a use for it that I cannot see.  "Finishing" the feature will actually *cause* harm because it would then always override any modprobe setting. {quote} Is there any advantage (even theoretically) to use hwgain over swgain (eg, clarity of signal, improved sampling etc ...), {quote} I don't think there is any advantage other than some small savings in CPU time.  Otherwise, it is a hit or miss feature depending upon hardware support. By: Jaco Kroon (jkroon) 2013-10-11 07:08:02.096-0500 Richard, I'll have to abide by what you decide, but I reckon your patch is subset of my original.  I also think it should be possible to complete the feature without causing harm (if by default it just doesn't touch the setting and output HW Gain: unknown it's still an improvement over current).  If however the value is set in the config file then obviously we WANT to override the module default don't you think? I don't think setting or NOT setting SWGAIN makes any difference in CPU, from what I can tell there's a linear matrix transform that's being applied to the incoming stream which (again, from what I can tell) defaults to the identity matrix that just results in no changes (but this is a hunch based on what little I've read of chan_dahdi).  If setting hwgain instead of swgain makes a quality difference (which I personally think it does) then I'd really prefer to set hwgain over swgain.  Keep in mind that I only ship Digium cards to my clients (had too many issues with other brands).  On a digital channel setting hwgain makes no sense as the signal is already companded (either ulaw or alaw, doesn't really matter), so even that would be a digital gain, not an analog gain setting (which based on my experience often results in a better digitized signal than without - you want to use the full digital range on input, not only the bottom half, eg, pushing +8dB on input already means we're more than doubling the signal strength which implies that we're getting at most 127 steps from the hardware on input, whereas if the same is applied at the pre-ADC point we get a full 256-step digitization - and that HAS to count for something). By: Richard Mudgett (rmudgett) 2013-10-14 17:47:56.897-0500 The points you make about hwgain make sense.  I hand not thought about needing to center the analog signal on the A/D conversion range.  I have created a patch to add hwtxgain and hwrxgain to chan_dahdi.conf.  However, the changes are more inline with a new feature/enhancement than a bug fix so I am only going to commit it to trunk. [^jira_asterisk_22429_hwgain_trunk.patch] - Adds hwtxgain and hwrxgain to chan_dahdi.conf. [^jira_asterisk_22429_v1.8_v2.patch] - Version for convenience.