 |
 |
| View previous topic :: View next topic |
| Author |
Message |
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
| rs232 pass through in both directions |
 Posted: Tue Mar 22, 2011 5:39 pm |
 |
|
Hi There,
I'm trying to realize a pass through of rs232 data through my 18f87k22. On one side theres a PC and on the other side a 16F883.
For some reason I can't deliver data properly from the 16F back to the PC. I came up with below sample app to show the problem.
Does anyone see where the fluke in my code is?
Thank you!
| Code: | #include <18F87K22.h>
#device ICD=TRUE, HIGH_INTS=TRUE, adc=16
#FUSES NOWDT //No Watch Dog Timer
#FUSES WDT128 //Watch Dog Timer uses 1:128 Postscale
#FUSES HSM //Hi-Speed crystal oscillator
#FUSES NOBROWNOUT //No brownout reset
#FUSES NOPLLEN //No PLL enabled
#FUSES BBSIZ1K //1K words Boot Block size
#FUSES NOXINST //Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#use delay(clock=10000000)
#use rs232(baud=9600,parity=N,xmit=PIN_G1,rcv=PIN_G2,stream=MCU1, ERRORS)
#use rs232(baud=9600,parity=N,xmit=PIN_C6,rcv=PIN_C7,bits=8,stream=DEBUG, ERRORS)
#bit RC1IE = getenv("bit:RC1IE") // Get the receive interrupt enable bit from USART1
int32 outBrec=0;
int32 Brec=0;
int32 outBSnt=0;
int32 BSnt=0;
int8 outcnt=0;
int8 incnt=0;
int8 outbuf[100]={0};
int8 inbuf[100]={0};
void main(void)
{
enable_interrupts(INT_RDA);
enable_interrupts(GLOBAL);
while(RC1IE){
if(incnt){
fputc(inbuf[--incnt],MCU1);
inbuf[incnt]=0x00;
BSnt++;
}
if(outcnt){
fputc(outbuf[--outcnt],DEBUG);
outbuf[outcnt]=0x00;
outBSnt++;
}
}
}
#int_rda
void RDA_isr()
{
static int16 first_esc=0;
int8 ch,outch;
static int8 tmpbuf[3]={0};
if (kbhit(DEBUG)){
BRec++;
ch=fgetc(DEBUG);
if (ch==0x1b){ // check for first byte of HOME (HOME key sends 4 Bytes, 0x1b 0x5b 0x31 0x7e
tmpbuf[0]=0x1b;
} else if(ch==0x5b && tmpbuf[0]==0x1b){// check for 2nd HOME Byte
tmpbuf[1]=0x5b;
} else if(ch==0x31 && tmpbuf[1]==0x5b && tmpbuf[0]==0x1b){//check for 3rd HOME Byte
tmpbuf[2]=0x31;
} else if(ch==0x7e && tmpbuf[2]==0x31 && tmpbuf[1]==0x5b && tmpbuf[0]==0x1b){
if (first_esc){ // has the first home key already been received completely?
disable_interrupts(INT_RDA);// disable interrupt on incoming serial data
tmpbuf[0]=0;
tmpbuf[1]=0;
first_esc=0;
}
else
first_esc=BRec; // set the flag that we have received the first HOME key seq
// to the no of Bytes received so we can expect the next reception within 100bytes
BRec=BRec-4;
return;
}else{
if (first_esc){
if(BRec-first_esc>100){
tmpbuf[0]=0;
tmpbuf[1]=0;
first_esc=0;
}
}
if (incnt>=100){
fprintf(DEBUG,"Variable Overflow inbuf!\r\n");
return;
}
inbuf[incnt++]=ch;
}
}
if (kbhit(MCU1)){
outch=fgetc(MCU1);
outBrec++;
if (outcnt>=100){
fprintf(DEBUG,"Variable Overflow outbuf!\r\n");
return;
}
outbuf[outcnt++]=outch;
}
} |
|
|
 |
bkamen
Joined: 07 Jan 2004
Posts: 1615
Location: Central Illinois, USA
|
|
| Posted: Tue Mar 22, 2011 8:34 pm |
|
|
Are you using one hardware UART and one software?
For an application like that, I would recommend 2 hardware UART's being fed by RCV ISR's for each UART.
-Ben
_________________
Dazed and confused? I don't think so. Just "plain lost" will do. :D |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9226
Location: Greensville,Ontario
|
|
| Posted: Wed Mar 23, 2011 6:49 am |
|
|
| While it might no matter, using 'DEBUG' as a stream name, might confuse the compiler as DEBUG should be considered a 'reserved word' ? |
|
|
bkamen
Joined: 07 Jan 2004
Posts: 1615
Location: Central Illinois, USA
|
|
| Posted: Wed Mar 23, 2011 8:42 am |
|
|
| temtronic wrote: | | While it might no matter, using 'DEBUG' as a stream name, might confuse the compiler as DEBUG should be considered a 'reserved word' ? |
I don't know - I used #define DEBUG all the time as it's not already in use.
But I would agree that "DEBUG" is sort of an unofficial reserved word.
-Ben
_________________
Dazed and confused? I don't think so. Just "plain lost" will do. :D |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Wed Mar 23, 2011 9:59 am |
|
|
| bkamen wrote: | | temtronic wrote: | | While it might no matter, using 'DEBUG' as a stream name, might confuse the compiler as DEBUG should be considered a 'reserved word' ? |
But I would agree that "DEBUG" is sort of an unofficial reserved word.
|
Fair enough, I've replaced DEBUG with PC it however didn't change the (not) behavior of my application... :( |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Wed Mar 23, 2011 12:15 pm |
|
|
Ok, my test app now looks like below and it seems to work well as long as I have my oscilloscope probes connected - so I might have a capacitive issue with my dev assembly - i'll have to see if this disappears once we get our first sample... I just talked to our hardware designer and he thinks it shouldn't be a problem once we have the hardware in our hands...
the code:
| Code: | #include <18F87K22.h>
#device ICD=TRUE, HIGH_INTS=TRUE, adc=16
#FUSES NOWDT //No Watch Dog Timer
#FUSES WDT128 //Watch Dog Timer uses 1:128 Postscale
#FUSES HSM //Hi-Speed crystal oscillator
#FUSES NOBROWNOUT //No brownout reset
#FUSES NOPLLEN //No PLL enabled
#FUSES BBSIZ1K //1K words Boot Block size
#FUSES NOXINST //Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#use delay(clock=10000000)
#use rs232(baud=9600,parity=N,xmit=PIN_G1,rcv=PIN_G2,stream=MCU1, ERRORS)
#use rs232(baud=9600,parity=N,xmit=PIN_C6,rcv=PIN_C7,bits=8,stream=PC, ERRORS)
#bit RC1IE = getenv("bit:RC1IE") // Get the receive interrupt enable bit from USART1
int32 outBrec=0;
int32 Brec=0;
int32 outBSnt=0;
int32 BSnt=0;
int8 outcnt=0;
int8 incnt=0;
int8 outbuf[100]={0};
int8 inbuf[100]={0};
void main(void)
{
enable_interrupts(INT_RDA);
enable_interrupts(INT_RDA2);
enable_interrupts(GLOBAL);
while(RC1IE){
if(incnt){
fputc(inbuf[--incnt],MCU1);
inbuf[incnt]=0x00;
BSnt++;
}
if(outcnt){
fputc(outbuf[--outcnt],PC);
outbuf[outcnt]=0x00;
outBSnt++;
}
}
}
//--------------------------------------------------------------
#int_rda
void RDA_isr()
{
static int16 first_esc=0;
int8 ch;
static int8 tmpbuf[3]={0};
if (kbhit(PC)){
BRec++;
ch=fgetc(PC);
if (ch==0x1b){ // check for first byte of HOME (HOME key sends 4 Bytes, 0x1b 0x5b 0x31 0x7e
tmpbuf[0]=0x1b;
} else if(ch==0x5b && tmpbuf[0]==0x1b){// check for 2nd HOME Byte
tmpbuf[1]=0x5b;
} else if(ch==0x31 && tmpbuf[1]==0x5b && tmpbuf[0]==0x1b){//check for 3rd HOME Byte
tmpbuf[2]=0x31;
} else if(ch==0x7e && tmpbuf[2]==0x31 && tmpbuf[1]==0x5b && tmpbuf[0]==0x1b){
if (first_esc){ // has the first home key already been received completely?
disable_interrupts(INT_RDA);// disable interrupt on incoming serial data
tmpbuf[0]=0;
tmpbuf[1]=0;
first_esc=0;
}
else
first_esc=BRec; // set the flag that we have received the first HOME key seq
// to the no of Bytes received so we can expect the next reception within 100bytes
BRec=BRec-4;
return;
}else{
if (first_esc){
if(BRec-first_esc>100){
tmpbuf[0]=0;
tmpbuf[1]=0;
first_esc=0;
}
}
if (incnt>=100){
fprintf(PC,"Variable Overflow inbuf!\r\n");
return;
}
inbuf[incnt++]=ch;
}
}
}
//--------------------------------------------------------------
#int_rda2
void RDA2_isr()
{
int8 ch=0;
if (kbhit(MCU1)){
ch=fgetc(MCU1);
outBrec++;
if (outcnt>=100){
fprintf(PC,"Variable Overflow outbuf!\r\n");
return;
}
outbuf[outcnt]=ch;
outcnt++;
}
} |
|
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Wed Mar 23, 2011 12:43 pm |
|
|
Well, that euphoria was too early. I realized that I'm "only" able to connect to with the bootloader up to my 16F883 PIC but I'm actually not able to send through any data... :( Read and Write both keep timing out... :(
Well, read worked once after a few tries but...  am unable to explain that, been trying to make sense of what i'm seeing on my scope but...  |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9226
Location: Greensville,Ontario
|
|
| Posted: Wed Mar 23, 2011 1:25 pm |
|
|
Hmm...
do you have a GOOD ground ? Most scope probes are at least 10Meg imp..
I see you have ICD=true, that may cause problems...silly things like pins you thought were 'yours' don't act right....
If code was cut using MPLAB have you compiled with build configuryion set to release and NOT the default of 'debug' ? The default will do 'strange' things to your code....
like adding debug code in your code... |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Wed Mar 23, 2011 1:51 pm |
|
|
GND was my first thought too but I have the two boards powered from the same power source, GNDs are directly connected, GNDs of the RS232 interfaces are connected too thus that shouldn't be the issue.
I however set MPLAB to Release and removed the ICD=TRUE directive and it seems to connect now even when the probes are disconnected. But looks like I can read with the bootloader app from AN1310 only once right after I connected the bootloader-app to the target but I never get to write anything succesfully :( Any clues there maybe  |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Fri Mar 25, 2011 8:46 am |
|
|
I might not have the solution to your problem but here are a few tips to improve your code:
*1*Inside the ISR you don't have to test for a character being present. You got here because there is a character waiting to be processed. Removing this test makes the code easier to read and faster to execute.
Also remove the similar test from the other ISR.
*2*
A serious problem is that you are using variables in the main function that are also modified by the interrupts. This is a disaster waiting to happen. For example this piece of code in main: | Code: | fputc(inbuf[--incnt],MCU1);
inbuf[incnt]=0x00; |
If now in between these two lines a interrupt is triggered, then the variable incnt will be increased and you are setting the wrong offset in inbuf[] to zero.
The normal solution is to temporarily disable the interrupts. Take care to do this around as short pieces of code as possible. | Code: |
if(incnt){
disable_interrupts(GLOBAL); // Temporarily disable to protect incnt variable
fputc(inbuf[--incnt],MCU1);
inbuf[incnt]=0x00;
enable_interrupts(GLOBAL);
BSnt++;
} |
Note: because the fputc function can take some time when another byte is waiting to be sent, it is better to store the new value in a temp variable and call fputc outside the disable/enable section.
*3* | Code: | | static int8 tmpbuf[3]={0}; |
This only initialises the first element of the array. Correct would be to write: | Code: | | static int8 tmpbuf[3]={0,0,0}; |
But because static variables are by definition initialised to zero the whole initialisation can be omitted.
*4*
Your test for the HOME sequence will also accept sequences like:
0x1b 0x1b 0x5b 0x5b 0x31 0x31 0x7e
0x1b 0x5b 0x5b 0x5b 0x31 0x7e
0x1b 0x5b 0x31 0x5b 0x1b 0x7e
etc.
You could improve this by using a state counter, for example: | Code: | int8 CodeState=0;
ch=fgetc(PC);
switch (CodeState)
{
case 0: ch == 0x1b ? CodeState++ : CodeState=0; break;
case 1: ch == 0x5b ? CodeState++ : CodeState=0; break;
case 2: ch == 0x31 ? CodeState++ : CodeState=0; break;
case 3: ch == 0x7e ? CodeState++ : CodeState=0; break;
default: CodeState = 0;
}
if (CodeState == 4)
{
// HOME code was received. Do your processing here.
} |
|
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Fri Mar 25, 2011 11:00 am |
|
|
Yep,
Great tips! Thank you very much! Esp. *2*!!! For this case, I just removed as I don't check for this again anyways. I also implemented a counter as shown in *4* - makes it look a lot nicer!
Thank you for these hints, thety're very much appreciated!
Regarding *3*
to initialize an array you would need to do | Code: | | int array[1024]={0,0,0,0,...,0} |
I agree but most compiler fill all the missing elements with 0 thus | Code: | | int array[1024]={0} |
initializes the array with 0 with most compilers (at least GCC & Borland, M$ probably too, not sure). Thus my initialization should be fine but I agree static variables are initialized with 0 already, thus i just removed it.
*4* I think CCS sadly doesn'tsupports the "short if form" | Code: | | like x==1 ? x++ : x--; |
I just looked at that yesterday and couldn't find it anywhere thus i had to break this up in | Code: | | if (bla) blah; else blahblah; |
- however, works fine! And as mentioned above, looks much nicer!
Thanks pal! |
|
|
SherpaDoug
Joined: 07 Sep 2003
Posts: 1640
Location: Cape Cod Mass USA
|
|
| Posted: Fri Mar 25, 2011 11:13 am |
|
|
CCS does support what you call the "short if form". They call it the "Conditional Expression Operator". It is in the Operators table of my CCS manual and I have been using it for years in trivial IF expressions.
I don't use it in complex expressions because it can make things confusing if someone else has to read my code.
_________________
The search for better is endless. Instead simply find very good and get the job done. |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Fri Mar 25, 2011 11:19 am |
|
|
| SherpaDoug wrote: | CCS does support what you call the "short if form". They call it the "Conditional Expression Operator". It is in the Operators table of my CCS manual and I have been using it for years in trivial IF expressions.
|
But this doesn't compile for me, I'm getting Expecting LVALUE such as a variable name or * expression
I'm using 4.119 |
|
|
bkamen
Joined: 07 Jan 2004
Posts: 1615
Location: Central Illinois, USA
|
|
| Posted: Fri Mar 25, 2011 11:59 am |
|
|
| cerr wrote: | | SherpaDoug wrote: | CCS does support what you call the "short if form". They call it the "Conditional Expression Operator". It is in the Operators table of my CCS manual and I have been using it for years in trivial IF expressions.
|
But this doesn't compile for me, I'm getting Expecting LVALUE such as a variable name or * expression
I'm using 4.119 |
I think the normal format of this type of expression is:
( test )? DoIfTrue: DoIfFalse;
I always put in parentheses around my test.
Maybe that's what your missing?
As for your "if" example, you have
| Code: |
if (bla) blah; else blahblah;
|
which also seems sloppy.
I would write as
| Code: |
if (test) {
do this;
} else {
do this;
}
|
Although is it legal to write it as:
| Code: |
if (test)
Do this;
else
do this;
|
The difference is that in my braced example, I can add extra lines of resulting commands without the need to add braces where without them, any extra commands would require braces or cause a compiler error.
Code should be READABLE. Not superneatoconfusingandextravagant.
_________________
Dazed and confused? I don't think so. Just "plain lost" will do. :D |
|
|
cerr
Joined: 10 Feb 2011
Posts: 241
Location: Vancouver, BC
|
|
| Posted: Fri Mar 25, 2011 1:12 pm |
|
|
Well, I didn't get the conditional expression operator working - even when I added brackets but I think below doesn't look two bad either:
| Code: | // test for two consecutive HOME buttons
switch (CodeState)
{
case 0: if(ch == 0x1b) CodeState++; else CodeState=0; break;
case 1: if(ch == 0x5b) CodeState++; else CodeState=0; break;
case 2: if(ch == 0x31) CodeState++; else CodeState=0; break;
case 3: if(ch == 0x7e) CodeState++; else CodeState=0; break;
case 4: if(ch == 0x1b) CodeState++; else CodeState=0; break;
case 5: if(ch == 0x5b) CodeState++; else CodeState=0; break;
case 6: if(ch == 0x31) CodeState++; else CodeState=0; break;
case 7: if(ch == 0x7e) CodeState++; else CodeState=0; break;
default: CodeState = 0;
}
if (CodeState == 8)
{
// 2 HOMEs were received, leave here!
fprintf(PC,"HOME!\r\n");
CodeState = 0;
return;
} |
|
|
|
|
|
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
