 |
 |
| View previous topic :: View next topic |
| Author |
Message |
adamp524
Joined: 06 May 2010
Posts: 22
|
| Interrupt driven serial |
 Posted: Thu Jul 29, 2010 9:42 am |
 |
|
Hardware: PIC18F4585 with its internal oscillator at 8Mhz.
I'm writing a program to read an XML message from the serial port (PIC hardware UART). The message start and end positions are denoted by an ASCII start line character (0x02) and an end line character (0x03).
My problem is that when I run the program, and send it 1 XML message every 100ms, the PIC locks up, and the watchdog timer kicks in and resets the device. This can happen any time between a couple of seconds to a few minutes from startup.
I've posted my serial interrupt and my main() function code below.
Any help would be appreciated.
Thanks
| Code: | //Global serial variables
#define SERIAL_BUFFER_SIZE 500
char SERIAL_BUFFER[SERIAL_BUFFER_SIZE] = "";
long SERIAL_BUFFER_NEXT = 0;
int1 SERIAL_PROCESS = FALSE;
#define START_LINE 0x02
#define END_LINE 0x03
#int_RDA
void RDA_isr(void)
{
char ch;
static int1 startFound = FALSE;
disable_interrupts(INT_RDA);
ch = getc();
//Start of line found
if(ch == START_LINE || startFound == TRUE)
{
startFound = TRUE;
SERIAL_BUFFER[SERIAL_BUFFER_NEXT++] = ch;
if(SERIAL_BUFFER_NEXT > SERIAL_BUFFER_SIZE)
{
//Buffer overflow - dump received data and start again
//disable serial receiver
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
startFound = FALSE;
enable_interrupts(INT_RDA);
//enable serial receivers
output_low(RECV_EN2);
}
if(ch == END_LINE) //End of line found
{
//disable serial receivers
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
SERIAL_PROCESS = TRUE;
startFound = FALSE;
}
}
if(SERIAL_PROCESS == FALSE)
{
enable_interrupts(INT_RDA);
}
} |
main():
| Code: | void main()
{
enable_interrupts(INT_RDA);
enable_interrupts(GLOBAL);
setup_wdt(WDT_ON);
while(TRUE)
{
process_serial();
restart_wdt();
}
} |
process_serial():
| Code: | void process_serial()
{
if(SERIAL_PROCESS == TRUE)
{
//Process serial buffer
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
SERIAL_PROCESS = FALSE;
enable_interrupts(INT_RDA);
//enable serial receivers
output_low(RECV_EN2);
}
} |
|
|
 |
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Thu Jul 29, 2010 8:36 pm |
|
|
Your code is a little bit difficult to read because you didn't follow the
normal C style convention of putting constants in upper case and
variables in lower case or mixed case. Also you didn't show your PIC
type or #fuses, etc. That's problably why there are no replies.
That said, I looked a little bit at your program and found a problem:
| Quote: |
#define SERIAL_BUFFER_SIZE 500
char SERIAL_BUFFER[SERIAL_BUFFER_SIZE] = "";
long SERIAL_BUFFER_NEXT = 0;
int1 SERIAL_PROCESS = FALSE;
SERIAL_BUFFER[SERIAL_BUFFER_NEXT++] = ch;
if(SERIAL_BUFFER_NEXT > SERIAL_BUFFER_SIZE)
{
SERIAL_BUFFER_NEXT = 0;
|
In the code above, you are checking for the end of the buffer by
comparing the index to the buffer size (500). In order for the if() test
to be true, the index must be 501. This means that the previous line
wrote 'ch' to index 500. Then the index was post-incremented to 501.
But there is no index of 500. The array only contains elements in
the range 0 to 499. Arrays start at 0.
Notice how CCS checks for buffer wrap in Ex_usb_serial2.c, in this
routine. They do something very similar to what you're doing, but
notice how they use '>=' to do the test:
| Code: |
char g_PutcBuffer[350];
int16 g_PutcNextIn=0, g_PutcNextOut=0;
void putc_tbe(char c)
{
g_PutcBuffer[g_PutcNextIn++] = c;
if (g_PutcNextIn >= sizeof(g_PutcBuffer))
g_PutcNextIn = 0;
}
|
| Quote: |
//disable serial receiver
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
startFound = FALSE;
enable_interrupts(INT_RDA);
|
Another point. There are no nested interrupts. You are not going to
get interrupted by another #int_rda interrupt in the middle of this
routine. So you don't have to disable interrupts and re-enable them
in the middle of the routine. Or at the start of the routine. |
|
|
adamp524
Joined: 06 May 2010
Posts: 22
|
|
| Posted: Mon Aug 09, 2010 7:37 am |
|
|
Thanks PCM Programmer
I tried changing the SERIAL_BUFFER_NEXT check to '>=' but this doesn't solve my problem - the WDT still catches the program somewhere and resets the PIC.
The reason for my disabling the serial interrupt was to ensure that the process_serial() function had finished before accepting any more serial data. If not, my program could had received more data and changed the buffer that would currently be processing.
Any other ideas?
Thanks |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Mon Aug 09, 2010 9:31 am |
|
|
You are making excessive use of enabling/disabling the RDA interrupt. This is not an error but makes your code a lot more difficult to read.
I understand why you want to disable the interrupt, but there is only one location where you have to do this, and that is after receiving the END marker.
In the fragment below I have marked the superfluous lines with a '//' in the first column. | Code: | #int_RDA
void RDA_isr(void)
{
char ch;
static int1 startFound = FALSE;
// disable_interrupts(INT_RDA);
ch = getc();
//Start of line found
if(ch == START_LINE || startFound == TRUE)
{
startFound = TRUE;
SERIAL_BUFFER[SERIAL_BUFFER_NEXT++] = ch;
if(SERIAL_BUFFER_NEXT > SERIAL_BUFFER_SIZE)
{
//Buffer overflow - dump received data and start again
//disable serial receiver
output_high(RECV_EN2);
// disable_interrupts(INT_RDA);
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
startFound = FALSE;
// enable_interrupts(INT_RDA);
//enable serial receivers
output_low(RECV_EN2);
}
if(ch == END_LINE) //End of line found
{
//disable serial receivers
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
SERIAL_PROCESS = TRUE;
startFound = FALSE;
}
}
// if(SERIAL_PROCESS == FALSE)
// {
// enable_interrupts(INT_RDA);
// }
} |
A real error is that in the small chance when receiving the END marker is also creating a buffer overflow, you are handling this combination wrong. The buffer is cleared but also the SERIAL_PROCESS flag is set.
There are no obvious errors in the code that could create a watchdog error. Without a complete program demonstrating your problem we can only guess. One thing that comes to mind is that you are missing the 'ERRORS' directive in the #use rs232 line. Without this directive the UART will stall after a receive buffer overflow (after 3 characters). |
|
|
bkamen
Joined: 07 Jan 2004
Posts: 1611
Location: Central Illinois, USA
|
|
| Posted: Mon Aug 09, 2010 9:49 am |
|
|
| ckielstra wrote: | You are making excessive use of enabling/disabling the RDA interrupt. This is not an error but makes your code a lot more difficult to read.
I understand why you want to disable the interrupt, but there is only one location where you have to do this, and that is after receiving the END marker.
|
I agree.
You NEVER want to really disable the interrupt because of the potential for overflows locking the unit (although ERRORs does clear)...
One way I do it (like with GPS's) is when gathering receive data in an interrupt, it gets stored to a buffer with a state machine that throws away successive received chars until the buffer is processed.
I make the process routine as fast as possible to clear that "toss" flag so that ISR can go back on its merry way.
I never disable the INT_RDA inside the ISR. (although I can see why someone else might want to, and not that it's wrong. I just don't do it.)
Yikes. Just toss chars away.
-Ben
_________________
Dazed and confused? I don't think so. Just "plain lost" will do. :D |
|
|
adamp524
Joined: 06 May 2010
Posts: 22
|
|
| Posted: Tue Aug 10, 2010 7:18 am |
|
|
Thanks everyone for your replies - sadly I'm still having problems with the serial!
ckielstra - I changed my serial buffer code from an 'if' to an 'else if' for the End of line found code - no change
I have also tried changing the #use directive for rs232 to include RESTART_WDT, ERRORS and DISABLE_INTS
e.g. #use rs232(stream=serial, baud=9600, UART1, bits=8, stop=1, RESTART_WDT, ERRORS, DISABLE_INTS)
bkamen - I've also tried dumping the data on the serial port when the buffer is being processed rather than disabling interrupts - no change.
I'll post my code in full below, see if that helps:
main.c: | Code: | #include "main.h"
//Serial defines
#define START_LINE 0x02
#define END_LINE 0x03
//Global serial variables
char SERIAL_BUFFER[SERIAL_BUFFER_SIZE] = "";
long SERIAL_BUFFER_NEXT = 0;
int1 SERIAL_PROCESS = FALSE;
//Initialise device address
int DEVICE_ADDR=255;
int TIMER1_OVERFLOW = 0;
//Serial RX ISR
#int_RDA
void RDA_isr(void)
{
char ch;
static int1 startFound = FALSE;
ch = getc();
//Start of line found
if(ch == START_LINE || startFound == TRUE)
{
startFound = TRUE;
SERIAL_BUFFER[SERIAL_BUFFER_NEXT++] = ch;
if(SERIAL_BUFFER_NEXT >= SERIAL_BUFFER_SIZE)
{
//Buffer overflow - dump received data and start again
fprintf(debug, "Buffer overflow\r\n");
//disable serial receiver
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
startFound = FALSE;
enable_interrupts(INT_RDA);
//enable serial receivers
output_low(RECV_EN2);
}
else if(ch == END_LINE) //End of line found
{
//disable serial receivers
output_high(RECV_EN2);
disable_interrupts(INT_RDA);
SERIAL_PROCESS = TRUE;
startFound = FALSE;
}
}
}
void startup()
{
//tri-state serial drivers and receivers
output_high(RECV_EN1);
output_high(RECV_EN2);
output_low(DRIV_EN1);
output_low(DRIV_EN1);
//Allow spare IO to float
output_float(SPARE_IO1);
output_float(SPARE_IO2);
//Initialise LCD pins
output_low(LCD_RS);
output_low(LCD_RW);
output_low(LCD_E);
output_low(LCD_DB4);
output_low(LCD_DB5);
output_low(LCD_DB6);
output_low(LCD_DB7);
//Enable and configure timers and interrupts
enable_interrupts(INT_EXT);
enable_interrupts(INT_TIMER1);
enable_interrupts(INT_RDA);
enable_interrupts(GLOBAL);
ext_int_edge(H_TO_L);
setup_timer_1(T1_INTERNAL|T1_DIV_BY_4);
//Enable serial drivers and receivers
output_low(RECV_EN1);
output_low(DRIV_EN1);
output_low(RECV_EN2);
output_low(DRIV_EN2);
//Print ready to debug serial port
fprintf(debug, "\n\rReady - DeviceAddr:%u\n\r",DEVICE_ADDR);
setup_wdt(WDT_ON);
}
void main()
{
startup();
while(TRUE)
{
if(TIMER1_OVERFLOW >= 3)
{
/* Flash CS LED1
if(flag == 0)
{
io_exp_output(GPIOA_ADDR, CS_LED1_ADDR, ON);
flag = 1;
}
else
{
io_exp_output(GPIOA_ADDR, CS_LED1_ADDR, OFF);
flag = 0;
}
*/
TIMER1_OVERFLOW = 0;
//fprintf(debug, "a"); //show alive
}
process_serial();
restart_wdt();
}
}
void process_serial()
{
if(SERIAL_PROCESS == TRUE)
{
//fprintf(debug, "Buf:%s", SERIAL_BUFFER);
//parse_xml(SERIAL_BUFFER);
delay_ms(50);
memset(SERIAL_BUFFER, '\0', SERIAL_BUFFER_SIZE);
SERIAL_BUFFER_NEXT = 0;
SERIAL_PROCESS = FALSE;
enable_interrupts(INT_RDA);
//enable serial receivers
output_low(RECV_EN2);
}
} |
main.h
| Code: | #include <18F4585.h>
//#FUSES NOWDT //No Watch Dog Timer
#FUSES WDT1024
#FUSES INTRC_IO //Internal RC Osc, no CLKOUT
#FUSES NOPROTECT //Code not protected from reading
#FUSES BROWNOUT //Reset when brownout detected
#FUSES MCLR //Master Clear pin enabled
#FUSES NOLVP //No Low Voltage Programming
#FUSES NOXINST // Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#device PASS_STRINGS=IN_RAM
#use delay(clock=8000000)
#use rs232(stream=serial, baud=9600, UART1, bits=8, stop=1, RESTART_WDT, ERRORS, DISABLE_INTS)
#use rs232(stream=debug, baud=19200, xmit=PIN_C2, rcv=PIN_C5, bits=8, stop=1)
#use i2c(MASTER, scl=PIN_C3, sda=PIN_C4, FAST, FORCE_HW)
#define FIRMWARE_VERSION "KpdDisp v1.0"
//LCD IO
#define LCD_DB4 PIN_A3
#define LCD_DB5 PIN_A4
#define LCD_DB6 PIN_A5
#define LCD_DB7 PIN_A6
#define LCD_RS PIN_A1
#define LCD_RW PIN_A2
#define LCD_E PIN_A0
#define LCD_BL PIN_A7
//Cardswipe IO
//Note: CS Clock set on interrupt pin
#define CS_DATA_PIN PIN_E2
//Keypad IO
//Note: pullup resistors must be placed on all rows
#define row0 PIN_D0
#define row1 PIN_D1
#define row2 PIN_D2
#define row3 PIN_D3
#define row4 PIN_D4
#define col0 PIN_D5
#define col1 PIN_D6
#define col2 PIN_D7
#define col3 PIN_E0
#define KPD_TMPR PIN_E1
//I2C
//IO Expander: Microchip MCP23017
#define IO_RST PIN_C0
#define IO_WRITE 0x40
#define IO_READ IO_WRITE+1
//Serial IO
#define RECV_EN1 PIN_B1
#define DRIV_EN1 PIN_B2
#define RECV_EN2 PIN_B3
#define DRIV_EN2 PIN_B4
//Spare IO
#define SPARE_IO1 PIN_B5
#define SPARE_IO2 PIN_C1
void process_serial(void);
void startup(void);
|
Thanks |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Aug 10, 2010 1:28 pm |
|
|
| Code: | enable_interrupts(INT_EXT);
enable_interrupts(INT_TIMER1); |
You enable the interrupts without an interrupt handler function being present. When the interrupt now occurs it is never cleared and the program forever hangs in the interrupt. That's why the watchdog kicks in. |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
