 |
 |
| View previous topic :: View next topic |
| Author |
Message |
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
| is it "forgetting" the interrupt enable bit??? :o |
 Posted: Mon Apr 11, 2011 5:37 pm |
 |
|
Hi There,
I got a small program running on a 16f883 pic. it's supposed to respond to incoming rs232 data. However, I've realized that there is a seemingly random "sometimes" factor in my app and that sometimes my #int_rda wouldn't kick-in....  is it considered saf(er) to re-enable the interrupts in every iteration?
I don't see how my code would hang itself anywhere...
I pasted my code below - i have utilized the buffer code from the sisr.c example.
Clues, hints & opinions appreciated!
| Code: |
#include <16F883.h>
#device adc=16
#include <string.h>
#FUSES NOWDT //No Watch Dog Timer
#FUSES HS //High speed Osc (> 4mhz for PCM/PCH) (>10mhz for PCD)
#FUSES NOBROWNOUT //No brownout reset
#FUSES NOLVP //No low voltage prgming, B3(PIC16) or B5(PIC18) used for I/O
#use delay(clock=20000000)
#define MCU2_RX PIN_C7
#define MCU2_TX PIN_C6
#USE RS232(baud=9600,parity=N,xmit=MCU2_TX,rcv=MCU2_RX,bits=8,stream=MCU2, ERRORS)
#define ADDR 0x03
#define BUFFER_SIZE 32
#define bkbhit (next_in!=next_out)
typedef struct{
int16 phase,
width,
intens;
int8 mode,
Hwver,
FWver,
temp;
}IRStruct;
IRStruct LEDdata[2];
BYTE buffer[BUFFER_SIZE];
BYTE next_in = 0;
BYTE next_out = 0;
char datagram[9]={0};
char elcnt=0;
void ProcessData(int8 *data, IRStruct *Param);
BYTE bgetc(void);
void main()
{
enable_interrupts (int_RDA); // enable the rs232 interrupts
enable_interrupts (GLOBAL);
memset(&LEDdata, 0 sizeof LEDdata);
char cin=0;
char recv_dat[9]={0};
int8 incnt=0;
while (true){
while(bkbhit){
cin=bgetc();
if (cin == 0x86 || incnt!=0) {
recv_dat[incnt++]=cin;
if (incnt>=9) {
memcpy(datagram,recv_dat,9);
ProcessData(datagram, &LEDdata);
memset(recv_dat,0x00,sizeof(recv_dat));
incnt=0;
}
}
}
if (bit_test (RS232_ERRORS, 2) && !input (MCU2_RX)) {
// if FERR bit set and pin C7 low, reset to start bootloader mode !
reset_cpu ();
}
}// [MAIN LOOP]
}
//------------------------------------------------------------------------------
BYTE bgetc()
{
BYTE c;
while(!bkbhit) ;
c=buffer[next_out];
next_out=(next_out+1) % BUFFER_SIZE;
return(c);
}
//------------------------------------------------------------------------------
void ProcessData(int8 *data, IRStruct *Param)
{
int8 i=0;
int8 snd[9] = {0};
int8 cmd = data[2];
int8 header = data[0];
int8 address = data[1];
int8 cksum=data[1]+data[3]+data[4]+data[5]+data[6]+data[7];
// check validity of data (header, address & cksum)
if (header==0x86 &&
address == ADDR &&
cksum==data[8]) {
switch (cmd) {
//assembling response.
}
snd[8] = snd[1] + snd[3] + snd[4] + snd[5] + snd[6] + snd[7];
// wait a short while before sending the response.
//delay_ms(5);
if (cmd != 0x09 || cmd !=0x11) {
for(i=0;i<9;i++) {
fputc(snd[i],MCU2);
}
}
}
memset(data,0x00,sizeof(data));
}
//------------------------------------------------------------------------------
#int_rda
void serial_isr() {
int t;
buffer[next_in]=getc();
t=next_in;
next_in=(next_in+1) % BUFFER_SIZE;
if(next_in==next_out)
next_in=t; // Buffer full !!
}
|
Thanks,
roN |
|
 |
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Mon Apr 11, 2011 5:45 pm |
|
|
As a note on the side:
After I moved
| Code: | enable_interrupts (int_RDA); // enable the rs232 interrupts
enable_interrupts (GLOBAL); |
inside the while(true), it seems to be working all the time... that's doesn't seem to be right to me... |
|
|
Wayne_
Joined: 10 Oct 2007
Posts: 681
|
|
| Posted: Tue Apr 12, 2011 1:52 am |
|
|
This is always true
| Code: |
if (cmd != 0x09 || cmd !=0x11)
|
cmd cannot be 0x09 and 0x011 at the same time so at least one of these conditions will allways be true 
What are the symptoms of it not working?
How do you know? |
|
|
ezflyr
Joined: 25 Oct 2010
Posts: 1019
Location: Tewksbury, MA
|
|
| Posted: Tue Apr 12, 2011 5:31 am |
|
|
Hi,
I haven't studied your code in detail, but I have a couple of comments:
1. You should not need to 'enable interrupts' multiple times. Do it once just before your While (True) loop, and that should be sufficient. It may not matter, but you enable interrupts before your initialize a bunch of variables.
2. You define a stream as 'MCU2', but then don't use the stream designator in your INT_RDA ISR. Change 'getc' to 'fgetc(MCU2)'
3. Break the problem up into parts to determine if you have a 'reception issue' or a 'processing issue'. That is, start by getting rid of all the code not related to receiving the incoming serial data, and verify that you can receive the intended data 100% of the time before moving on to the data processing.
John |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Tue Apr 12, 2011 10:10 am |
|
|
| Wayne_ wrote: | This is always true
| Code: |
if (cmd != 0x09 || cmd !=0x11)
|
cmd cannot be 0x09 and 0x011 at the same time so at least one of these conditions will allways be true
What are the symptoms of it not working?
How do you know? |
Hoops, I guess that better be a && than a || - thanks for the hint!
The symptoms are that this code all of a sudden stopped responding to any kind of queries and a reset would resolve the problem. The queries were still coming yes, I could monitor this on my debugging terminal as well as on the scope i have connected to Rx and Tx. |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Tue Apr 12, 2011 10:16 am |
|
|
| ezflyr wrote: | Hi,
I haven't studied your code in detail, but I have a couple of comments:
1. You should not need to 'enable interrupts' multiple times. Do it once just before your While (True) loop, and that should be sufficient. It may not matter, but you enable interrupts before your initialize a bunch of variables.
2. You define a stream as 'MCU2', but then don't use the stream designator in your INT_RDA ISR. Change 'getc' to 'fgetc(MCU2)'
3. Break the problem up into parts to determine if you have a 'reception issue' or a 'processing issue'. That is, start by getting rid of all the code not related to receiving the incoming serial data, and verify that you can receive the intended data 100% of the time before moving on to the data processing.
John |
Alright
#1 yes I know that once only should be enough.... how come it matters when i enable the interrupts? :o However, moved it to the top of my main now....
#2 -getc()
+fgetc(MCU2)
- I however think this has nothing to sdo with my problem as the interrupt didn't even trigger anymore...
#3 Yep, I agree.... will keep digging further today...
Thanks for your hints! |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19518
|
|
| Posted: Tue Apr 12, 2011 2:34 pm |
|
|
I would move your variable declarations in front of the function calls.
Standard C, requires variables to always be declared at the top of a routine.
Later C allows them to also be declared at the start of a code block.
Neither allow them to be declared 'mid code'.
Only later languages like C++ do.
CCS, does not complain if variables are declared 'in code', _but_ I have seen a lot of odd behaviour appear when they are....
Best Wishes |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Tue Apr 12, 2011 3:02 pm |
|
|
| Ttelmah wrote: | I would move your variable declarations in front of the function calls.
Standard C, requires variables to always be declared at the top of a routine.
Later C allows them to also be declared at the start of a code block.
Neither allow them to be declared 'mid code'.
Only later languages like C++ do.
CCS, does not complain if variables are declared 'in code', _but_ I have seen a lot of odd behaviour appear when they are....
Best Wishes |
HI Ttelmah, yeah in fact, I have seen some weird behaviours too that "all of a sudden" disappeared just by moving the variable declarations to the top. Also I have resolved my problem, the problem actually was that I wasn't careful enough on how I handled a double pointer in my ProcessData function. CCS isn't just not complaining about variable declarations, it also doesn't make a difference between * and **. It sure requires you/us to be careful! Esp when you're used to errors and warnings from GNU compilers and such. |
|
|
ezflyr
Joined: 25 Oct 2010
Posts: 1019
Location: Tewksbury, MA
|
|
| Posted: Tue Apr 12, 2011 3:17 pm |
|
|
Hi Cerr,
Your experience is somewhat 'classic' in software debugging, that is problems in one area of the program appear as problems in another area. That is why I encouraged you to divide up the problem into one of data reception, and data processing.
Why don't you post a copy of your revised code so that anyone reading this thread will have the opportunity to learn what you did?
Thanks,
John |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Tue Apr 12, 2011 3:19 pm |
|
|
| ezflyr wrote: | Hi Cerr,
Your experience is somewhat 'classic' in software debugging, that is problems in one area of the program appear as problems in another area. That is why I encouraged you to divide up the problem into one of data reception, and data processing.
Why don't you post a copy of your revised code so that anyone reading this thread will have the opportunity to learn what you did?
Thanks,
John |
Yep, I sure can do:
| Code: |
#include <16F883.h>
#include <stdlib.h>
#FUSES NOWDT //No Watch Dog Timer
#FUSES HS //High speed Osc (> 4mhz for PCM/PCH) (>10mhz for PCD)
#FUSES NOBROWNOUT //No brownout reset
#FUSES NOLVP //No low voltage prgming, B3(PIC16) or B5(PIC18) used for I/O
#use delay(clock=20000000)
#define MCU2_RX PIN_C7
#define MCU2_TX PIN_C6
#USE RS232(baud=9600,parity=N,xmit=MCU2_TX,rcv=MCU2_RX,bits=8,stream=MCU2, ERRORS)
#define ADDR 0x03
#define BUFFER_SIZE 32
#define bkbhit (next_in!=next_out)
typedef struct{
int16 phase,
width,
intens,
brightness;
int8 mode,
Hwver,
FWver,
temp;
}IRStruct;
IRStruct LEDdata[2];
BYTE buffer[BUFFER_SIZE];
BYTE next_in = 0;
BYTE next_out = 0;
char datagram[9]={0};
char elcnt=0;
void ProcessData(int8 *data, IRStruct *Param);
BYTE bgetc(void);
void main()
{
char cin=0;
char recv_dat[9]={0};
int8 incnt=0;
int ch=0;
memset(&LEDdata, 0 sizeof LEDdata);
enable_interrupts (int_RDA); // enable the rs232 interrupts
enable_interrupts (GLOBAL);
srand(0xffff);
// loading some random data into structure
LEDdata[0].FWver = 0x01;
LEDdata[0].HWver= 0x01;
LEDdata[0].temp = 0xaa;
LEDdata[0].mode = 0xbb;
LEDdata[0].brightness = 0x5678;
LEDdata[1].brightness = 0x1234;
while (true){
ch^=1;
LEDdata[ch].brightness = rand();
while(bkbhit){
cin=bgetc();
if (cin == 0x86 || incnt!=0) {
recv_dat[incnt++]=cin;
if (incnt>=9) {
memcpy(datagram,recv_dat,9);
ProcessData(datagram, LEDdata);
memset(recv_dat,0x00,sizeof(recv_dat));
incnt=0;
}
}
}
/* if (bit_test (RS232_ERRORS, 2) && !input (MCU2_RX)) {
// if FERR bit set and pin C7 low, reset to start bootloader mode !
reset_cpu ();
} */
}// [MAIN LOOP]
}
//------------------------------------------------------------------------------
BYTE bgetc()
{
BYTE c;
while(!bkbhit) ;
c=buffer[next_out];
next_out=(next_out+1) % BUFFER_SIZE;
return(c);
}
//------------------------------------------------------------------------------
void ProcessData(int8 *data, IRStruct *Param)
{
int8 i=0;
int8 snd[9] = {0};
int8 cmd = data[2];
int8 header = data[0];
int8 address = data[1];
int8 cksum=data[1]+data[3]+data[4]+data[5]+data[6]+data[7];
// check validity of data (header, address & cksum)
if (header==0x86 &&
address == ADDR &&
cksum==data[8]) {
switch (cmd) {
case 0x07:
//Pulse status query
if (data[6] == 0 || data[6] == 1) {
snd[0] = 0x86;
snd[1] = 0x03;
snd[2] = cmd;
snd[3] = data[6];
snd[4] = Param[data[6]].width;
snd[5] = ( Param[data[6]].width >>8 );
snd[6] = Param[data[6]].phase;
snd[7] = ( Param[data[6]].phase >>8 );
}
break;
case 0x09:
//Pulse status setup
if (data[3] == 0 || data[3] == 1) {
Param[data[3]].phase = data[7] | ((int16)data[8] <<8);
Param[data[3]].width = data[4] | ((int16)data[5] <<8);
}
break;
case 0x11:
//IR intensity setup
if (data[4] == 0 || data[4] == 1)
Param[data[4]].intens = data[5] | ((int16)data[6]<<8);
break;
case 0x13:
//IR intensity query
if (data[6] == 0 || data[6] == 1) {
snd[0] = 0x86;
snd[1] = 0x03;
snd[2] = cmd;
snd[3] = 0x66;
snd[4] = data[6];
snd[5] = Param[data[6]].intens;
snd[6] = ( Param[data[6]].intens >>8 );
snd[7] = 0x01;
}
break;
case 0x15:
//Board status query
snd[0] = 0x86;
snd[1] = 0x03;
snd[2] = cmd;
snd[3] = 0x66;
snd[4] = Param[0].temp;
snd[5] = Param[0].Fwver;
snd[6] = Param[0].Hwver;
snd[7] = Param[0].mode;
break;
case 0x17:
//Bright/Dark query
if (data[6] == 0 || data[6] == 1) {
snd[0] = 0x86;
snd[1] = 0x03;
snd[2] = cmd;
snd[3] = 0x00;
snd[4] = data[6];
snd[5] = 0x66;
snd[6] = Param[data[6]].brightness;
snd[7] = ( Param[data[6]].brightness>>8 );
}
break;
default:
}
snd[8] = snd[1] + snd[3] + snd[4] + snd[5] + snd[6] + snd[7];
// wait a short while before sending the response.
//delay_ms(5);
if (cmd != 0x09 && cmd !=0x11) {
for(i=0;i<9;i++) {
fputc(snd[i],MCU2);
}
}
}
}
//------------------------------------------------------------------------------
#int_rda
void serial_isr() {
int t;
buffer[next_in]=fgetc(MCU2);
t=next_in;
next_in=(next_in+1) % BUFFER_SIZE;
if(next_in==next_out)
next_in=t; // Buffer full !!
}
|
|
|
|
|
|
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
