> I want some feedback as to whether what I am doing is correct. I am
> using a DS80C320 with a Phillips SC28L198 8 channel UART. 2
> interrupts are triggered by the internal UARTS, 1 by the phillips chip
> and another by timer 0. I have found that for reliable operation i
> must turn all interrupts off when servicing the internal uart
> interrupts, and when putting a character into the ring buffer for
> transmission. Can anyone see any problem with this? Does anyone with
> a similar setup do it differently? Do I have to disable all
> interrupts, or only a few?
With the code you presented, the reason you need to disable interrupts
is to maintain a consistent state of tx_inptr when the associated
interrupt occurs. Only the UART interrupts should need to be disabled.
The minimal amount of code that needs to be protected is
As written, if an interrupt occurs between the two statements,
tx_inptr[chan] has an incorrect value, which could cause
isempty_tx[chan] in the interrupt routine to incorrectly return false.
What happens if you rewrite as
tx_inptr[chan] = (tx_inptr[chan]+1) & TXBUF_SIZE;
Assuming that tx_inptr is an unsigned char and that byte accesses are
separate and atomic (almost certainly the case for your processor), I
think you can leave interrupts enabled. What would happen is that an
interrupt would not see the new character until tx_inptr[chan] is
updated. Technically, this is not guaranteed by C unless tx_inptr[] is
of type volatile sig_atomic_t.
What happens if the interrupt occurs between the pointer update and
testing sending_flg? The byte gets written. If the following
completion interrupt occurs before uputch resumes (possible with
multiple interrupts), sending_flg could be set to 0, then uputch resumes
and sets it to 1 and restarts interrupts. The interrupt routine will
then see that there are no new characters and again reset sending_flg.
This shouldn't be a problem.
You didn't provide any protection against buffer overrun. What that
means is that if you buffer more than TXBUF_SIZE-1 characters, you will
probably get a false indication of an empty buffer, assuming that is
isempty_tx compares the two pointers for equality.
I hesitate to mention this, but the code for the internal UARTs can
usually be made more efficient in uputch() by changing the subscripts
"[chan]" within the cases to the actual channel used: [INTUART_1], etc.
The same would be true of the external channels if you wrote them as
separate cases, rather than combined. Also, isempty_tx would be more
efficient as a macro than as a non-inline function, since it has a
constant argument. This issue would affect interrupt service time. If
throughput is not a problem for your code, KISS and don't bother.
Thad
Reply by Rick Merrill●July 1, 20042004-07-01
Ross Marchant wrote:
> Hi All
>
> I want some feedback as to whether what I am doing is correct. I am
> using a DS80C320 with a Phillips SC28L198 8 channel UART. 2
> interrupts are triggered by the internal UARTS, 1 by the phillips chip
> and another by timer 0. I have found that for reliable operation i
> must turn all interrupts off when servicing the internal uart
> interrupts, and when putting a character into the ring buffer for
> transmission. Can anyone see any problem with this?
That is required to synchronize the interupts and the buffering.
(If you didn't do it, you would get random errors!)
Just make sure (and document) that TXBUF_SIZE is 2^x -1 .
- RM
Reply by Ross Marchant●June 30, 20042004-06-30
Hi All
I want some feedback as to whether what I am doing is correct. I am
using a DS80C320 with a Phillips SC28L198 8 channel UART. 2
interrupts are triggered by the internal UARTS, 1 by the phillips chip
and another by timer 0. I have found that for reliable operation i
must turn all interrupts off when servicing the internal uart
interrupts, and when putting a character into the ring buffer for
transmission. Can anyone see any problem with this? Does anyone with
a similar setup do it differently? Do I have to disable all
interrupts, or only a few?Below is two snippits of code as an example:
.................
//
// uputch -
//
// Puts a character out of a serial port
//
void uputch(unsigned char chan,unsigned char data)
{
unsigned char oldie;
oldie = IE;
IE = 0;
wdog();
switch(chan)
{
//Internal UARTS
case INTUART_1:
tx_buff[chan][tx_inptr[chan]] = data;
tx_inptr[chan]++;
tx_inptr[chan] &= TXBUF_SIZE;
if (sending_flg[1] == 0) {
sending_flg[1] = 1;
TI_1 = 1;
}
break;
case INTUART_0:
tx_buff[chan][tx_inptr[chan]] = data;
tx_inptr[chan]++;
tx_inptr[chan] &= TXBUF_SIZE;
if (sending_flg[0] == 0) {
sending_flg[0] = 1;
TI_0 = 1;
}
break;
//Philips UARTS
case EXTUART_7:
case EXTUART_6:
case EXTUART_5:
case EXTUART_4:
case EXTUART_3:
case EXTUART_2:
case EXTUART_1:
case EXTUART_0:
tx_buff[chan][tx_inptr[chan]] = data;
tx_inptr[chan]++;
tx_inptr[chan] &= TXBUF_SIZE;
//Enable TX FIFO empty interrupt
*IMR_WR_PTR[chan] = 0x03;
break;
}
IE = oldie;
}
...........
//
// uart1_isr -
//
// Interrupt routine for internal UART 1
//
interrupt void uart1_isr(void)
{
unsigned char oldie;
oldie = IE;
IE = 0;
if (RI_1) {
RI_1 = 0;
rx_buff[INTUART_1][rx_inptr[INTUART_1]] = SBUF1;
rx_inptr[INTUART_1]++;
rx_inptr[INTUART_1] &= RXBUF_SIZE;
}
if (TI_1) {
TI_1 = 0;
if (!isempty_tx(INTUART_1)) {
SBUF1 = tx_buff[INTUART_1][tx_outptr[INTUART_1]];
tx_outptr[INTUART_1]++;
tx_outptr[INTUART_1] &= TXBUF_SIZE;
}
else {
sending_flg[1] = 0;
}
}
IE = oldie;
}
...........
tia
Ross