Hi,
Let me start by apologising that my first post to this list is a criticism. I ordinarily would try to avoid being so negative however I have just finished reviewing the code for the TDM drivers and there are a few points which, as a professional software developer and CEO of a software development company, I feel need to be raised.
Don’t get me wrong - Asterisk is an amazing project and an extremely useful piece of software. These criticisms should not be taken as a criticism of Asterisk as a whole.
I shall use the following piece of code, taken from wctdm.c, to illustrate my points…
[code]#ifdef LINUX26
static irqreturn_t wctdm_interrupt(int irq, void *dev_id, struct pt_regs *regs)
#else
static void wctdm_interrupt(int irq, void *dev_id, struct pt_regs *regs)
#endif
{
struct wctdm *wc = dev_id;
unsigned char ints;
int x;
int mode;
ints = inb(wc->ioaddr + WC_INTSTAT);
outb(ints, wc->ioaddr + WC_INTSTAT);
if (!ints)
#ifdef LINUX26
return IRQ_NONE;
#else
return;
#endif
if (ints & 0x10) {
/* Stop DMA, wait for watchdog */
printk("TDM PCI Master abort\n");
wctdm_stop_dma(wc);
#ifdef LINUX26
return IRQ_RETVAL(1);
#else
return;
#endif
}
if (ints & 0x20) {
printk("PCI Target abort\n");
#ifdef LINUX26
return IRQ_RETVAL(1);
#else
return;
#endif
}
for (x=0;x<4;x++) {
if (wc->cardflag & (1 << x) &&
(wc->modtype[x] == MOD_TYPE_FXS)) {
if (wc->mod[x].fxs.lasttxhook == 0x4) {
/* RINGing, prepare for OHT */
wc->mod[x].fxs.ohttimer = OHT_TIMER << 3;
if (reversepolarity)
wc->mod[x].fxs.idletxhookstate = 0x6; /* OHT mode when idle */
else
wc->mod[x].fxs.idletxhookstate = 0x2;
} else {
if (wc->mod[x].fxs.ohttimer) {
wc->mod[x].fxs.ohttimer-= ZT_CHUNKSIZE;
if (!wc->mod[x].fxs.ohttimer) {
if (reversepolarity)
wc->mod[x].fxs.idletxhookstate = 0x5; /* Switch to active */
else
wc->mod[x].fxs.idletxhookstate = 0x1;
if ((wc->mod[x].fxs.lasttxhook == 0x2) || (wc->mod[x].fxs.lasttxhook = 0x6)) {
/* Apply the change if appropriate */
if (reversepolarity)
wc->mod[x].fxs.lasttxhook = 0x5;
else
wc->mod[x].fxs.lasttxhook = 0x1;
wctdm_setreg(wc, x, 64, wc->mod[x].fxs.lasttxhook);
}
}
}
}
}
}
if (ints & 0x0f) {
wc->intcount++;
x = wc->intcount & 0x3;
mode = wc->intcount & 0xc;
if (wc->cardflag & (1 << x)) {
switch(mode) {
case 0:
/* Rest */
break;
case 4:
/* Read first shadow reg */
if (wc->modtype[x] == MOD_TYPE_FXS)
wc->reg0shadow[x] = wctdm_getreg(wc, x, 68);
else if (wc->modtype[x] == MOD_TYPE_FXO)
wc->reg0shadow[x] = wctdm_getreg(wc, x, 5);
break;
case 8:
/* Read second shadow reg */
if (wc->modtype[x] == MOD_TYPE_FXS)
wc->reg1shadow[x] = wctdm_getreg(wc, x, 64);
else if (wc->modtype[x] == MOD_TYPE_FXO)
wc->reg1shadow[x] = wctdm_getreg(wc, x, 29);
break;
case 12:
/* Perform processing */
if (wc->modtype[x] == MOD_TYPE_FXS) {
wctdm_proslic_check_hook(wc, x);
if (!(wc->intcount & 0xf0))
wctdm_proslic_recheck_sanity(wc, x);
} else if (wc->modtype[x] == MOD_TYPE_FXO) {
wctdm_voicedaa_check_hook(wc, x);
}
break;
}
}
if (!(wc->intcount % 10000)) {
/* Accept an alarm once per 10 seconds */
for (x=0;x<4;x++)
if (wc->modtype[x] == MOD_TYPE_FXS) {
if (wc->mod[x].fxs.palarms)
wc->mod[x].fxs.palarms--;
}
}
wctdm_receiveprep(wc, ints);
wctdm_transmitprep(wc, ints);
}
#ifdef LINUX26
return IRQ_RETVAL(1);
#endif
}[/code]
Criticism Number 1 - Why use meaningless variable names?
In the driver code there are a great many variables with meaningless names such as x or y. While these are usually used as loop counters (strangely in contravention of the standard to use i) sometimes they are used to store other values.
Criticism Number 2 - Confusing renames of variables
Eg. struct wctdm *wc = dev_id;
Here we have a pointer to something (a void *) called dev_id, which a naive reader would guess probably meant device_id, becoming a pointer to a struct called wc of type wctdm. After searching for the definition of this struct it appears that it is very definitely not a device id.
More confusingly all the above line does is cast from a void * to a wctdm *. Given that this is the case then wouldn’t it have been appropriate to disable the warning from the compiler? Perhaps explain why we have to do this in a comment? Name the wc variable something meaningful?
Criticism Number 3 - Almost total lack of comments
Before I get any replies saying things like “If you aren’t good enough to read the code you shouldn’t be contributing!” I would like to reiterate that I am a professional software developer and have been coding for some twenty five years.
There is a reason that code should be extensively commented, regardless of how simple it is. I shall use a different piece of code (from the same file) as an example here as it illustrates my point so well I can’t resist.
/* the constants below control the 'debounce' periods enforced by the
check_hook routines; these routines are called once every 4 interrupts
(the interrupt cycles around the four modules), so the periods are
specified in _4 millisecond_ increments
*/
#define RING_DEBOUNCE 4 /* Ringer Debounce (64 ms) */
#define DEFAULT_BATT_DEBOUNCE 4 /* Battery debounce (64 ms) */
#define POLARITY_DEBOUNCE 4 /* Polarity debounce (64 ms) */
You see, even a first year Comp Sci student could tell you what these lines do. Of course if you now take the time to read the comment you’ll see that these lines in fact do something subtly different. Which is wrong? The comment? The code?
At least we know what was intended. This is the key point here. Good comments, even for trivial code, allow a developer, coming to the code afresh, to quickly see the intention of the previous developers. If these comments were (are) absent then we have no way of knowing what you intended - merely what you did.
Criticism Number 4 - Using side effects of non-idempotent functions in conditional statements
This is madness. Not only does it make code extremely hard to read but it makes it error prone too. Check out this beautiful example…
while((vbat = wctdm_getreg(wc, card, 82)) > 0x6) {
if ((jiffies - origjiffies) >= (HZ/2))
break;;
}
What on earth are you trying to achieve here? Why should I have to trawl through the code looking for the side effects of wctdm_getreg? If an employee of mine wrote a line like this with no comments I would have no choice but to fire them. My other employees would refuse to work with such an individual.
Why is this all this a problem?
The above issues, especially when combined into a single source file, make contributing to the project extremely costly.
I, in retrospect somewhat foolishly, asked one of our senior developers to have a quick look at the driver code to see if they could work out why very short loop-disconnects (~50ms) aren’t detected. I was hoping that this would take an hour or two tops. Luckily for me he is a quality employee in every way so he came back to me almost immediately saying that it was going to take a lot longer than a couple of hours to understand the code fully.
While the solution looks obvious if you read point three you’ll see that it now requires more thought, especially as the obvious changes didn’t work.
I would dearly like to fix this problem in-house and contribute the changes back to the community. Unfortunately the cost of developer time in my country (~$150/hour for someone of Mark’s quality) far outweighs the cost of the hardware in this case. If he needs more than four hours to fix this it is cheaper for us to ditch the TDM card we were using to test Asterisk and either buy one which works (and doesn’t use the TDM drivers) from a competitor or use a channel-bank.
So what?
You may well ask yourself that. If that is the case then I have failed in my explanation. I shall, however, reiterate the points below.
-
I like Asterisk! After only about a week of use (in spare moments) I think it is extremely useful and am seriously considering deployment on a wide scale.
-
I want (my company) to contribute to this project.
-
The code is probably of a perfectly adequate standard.
-
Poor coding practises, as outlined above, make verifying number three (directly above) and contributing to the project much more costly than they need to be.