Poor coding practises (in TDM driver)

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. :smile:

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.

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

  2. I want (my company) to contribute to this project.

  3. The code is probably of a perfectly adequate standard.

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

I agree on almost all points… but what exactly is the outcome are you hoping for here?

  1. You can update the code and postback to the community, or
  2. You can update the code and not postback, forever dooming you to additional source-check when upgrading to future versions, or
  3. Um, live with it.

Not to put too sharp of a point on it, but If you can’t be bothered to update the code then why should the (non-Digium) development community? (Especially considering that not all * users even have TDM cards.) It seems to me that what you’re really requesting is for Digium to re-write the code to your specifications, “or else”.

Thanks. I’m glad I’m not alone. :wink:

[quote=“hungerbrother”] but what exactly is the outcome are you hoping for here?

  1. You can update the code and postback to the community, or
  2. You can update the code and not postback, forever dooming you to additional source-check when upgrading to future versions, or
  3. Um, live with it.
    [/quote]
    I agree with you that these are our options. You’ll be pleased to hear that I’m not an arse so when our modifications to the driver are completed I’ll post it back to the community. :wink:

Seems you are making two points here. I agree with your first that it isn’t the job of the non-Digium community to maintain the Digium driver. I would say that is clearly the job of the Digium developers - they do sponsor Asterisk in an attempt to sell more hardware after all.

The second point, that I would like Digium to re-write to code, I would have to agree with also. I don’t particularly care whose specification they write it to as long as it looks like there was one. :wink: IMHO they probably should as it would show that they have a quality development process. Driver issues do directly reflect on their hardware after all.

If it was up to me I would avoid Digium hardware (if they did write the driver, and the copyright notice makes it look like they did) and use a non-Digium solution (a channel bank for example) with Asterisk. Unfortunately, at the very low end, and ironically as at the high-end Digium hardware would work fine, Digium hardware is probably the best available. We’d like to integrate it with some of our existing products and our customers like the price-point.

I would never say “or else” to the non-Digium community who develop for free out of the love of what they do or the desire to make a better product. They should be praised. If that was what I implied it was directed squarely at the Digium developers. From what I have seen so far the Asterisk code is generaly of a far superior standard to the code in that driver too so the community should be proud.

[quote=“M2”]I agree with you that these are our options. You’ll be pleased to hear that I’m not an arse so when our modifications to the driver are completed I’ll post it back to the community. :wink:
[/quote]

Glad to hear it. Re-reading my message, I’m not sure if my tone was exactly what I intended.

/grin

In the “grand scheme of things”, Digium, Inc. is a tiny organization that has the same problems as we all do… namely, too few resources. I’m not trying to be a Digium apologist, but they do have some good results with what few developers they have.

Personally, I’m hoping that the Digium T3 interface ships before a TDM driver re-write, with ‘well developed’ drivers or course. :smile: (We do some interesting things in order to get enough channel density to support our environment.)

If I may make a tiny tongue-in-cheek point, this is an example of the problems with the free software concept. If the product is actually that valuable to people, people will gladly pay for it, and then you could put more developers to work on it. :smile:

But then you wouldn’t see the source code anyways… :stuck_out_tongue:

Dan