 |
 |
| View previous topic :: View next topic |
| Author |
Message |
AArdvark
Joined: 09 Mar 2015
Posts: 10
|
| pic18F4520 acting odd while using RS232 |
 Posted: Thu Jun 04, 2015 2:37 am |
 |
|
Hi,
The code below is from a larger program but this section keeps causing me problems intermittently. I am sending it command in the format of FF9ABL30FFF0502DFE. I have 2 start chars FF, next is a length of the command in this case 9, then the command ABL30FFF0, crc is next 502D and lastly end chars FE. The code checks this command and replies back with an ACK or NACK. Everything is ok, it actions the command ABL30FFF0.(omitted for this)
I have cut the code right down and it still fails, it can fail for several reasons firstly
| Code: | if (n >0)
cmdgtzero = 1;
|
if n is greater than zero cmdgtzero does not change to 1 but stays at 0.
f[0] somehow changes itself from "F" to another char received by the buffer
| Code: | if (strcmp (f[0], c[0]) == 0 && strcmp (f[0], c[1]) == 0) //check if first 2 chars ar "FF"
startcmdreceived = 1; //set flag to say start command received
|
The crc check misses off leading zeros so i put a check in place that if it is less than 4, pad it with a zero and again even though the length was 4, this part of the code was run
| Code: |
if (strlen(crc) < 4)
{
strncat(crc_pad, crc,4);
}
else
strcpy(crc_pad,crc);
|
this is the code
| Code: |
#include <main.h>
#include <stdlib.h>
#include <crc.c>
#define BUFFER_SIZE 32
BYTE buffer[BUFFER_SIZE];
BYTE next_in = 0;
BYTE next_out = 0;
#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 !!
}
#define bkbhit (next_in!=next_out)
BYTE bgetc()
{
BYTE c;
while(!bkbhit);
c=buffer[next_out];
next_out=(next_out+1) % BUFFER_SIZE;
return(c);
}
void main()
{
setup_adc_ports(AN0_TO_AN3);
setup_adc(ADC_CLOCK_INTERNAL|ADC_TAD_MUL_20);
enable_interrupts(INT_RDA);
enable_interrupts(GLOBAL);
int16 n= 0; //length of command
int count = 0; //counter to count num of chars
int charcount = 0; //counts the chars comming in on the serial port
int32 crc_temp = 0; //temp storage for crc
char c[18] = " "; //string sent to the PIC
char f[2] = "F"; //commands need to be in caps
char e[2] = "E"; //commands need to be in caps
char temp[20]; //temp storage for the command
char command[18] = " "; //final command string
char crc[5] = " "; //crc value
char crctobechecked[5] = " "; //hold var for crc contined in command
char crctobecheckedtemp[5] = " "; //temp storage for crc while being checked
short startcmdreceived = 0; //boolean to say start command of "FF" received ok
short crc_passed = 0; //boolean to say crc passed the checks
short endcmdreceived = 0; //boolean to say end command of "FE" received ok
short cmdgtzero = 0; //boolean to say that the command length is greater than zero
short charsreceived =0; //boolean to say if any chars received from the input
char crc_pad[5] ="0";
while(1)
{ //loop forever waiting for an input
count = 3;
while(bkbhit)
{
c[charcount] = bgetc(); //get char from input and put it into 'c'
charcount++; //increment to move char array on by one
charsreceived = 1; //set flag to say chars received
if (charcount >18)
break;
}
if (charsreceived == 1) //if chars received then check command
{
charsreceived = 0;
if (strcmp (f[0], c[0]) == 0 && strcmp (f[0], c[1]) == 0) //check if first 2 chars ar "FF"
startcmdreceived = 1; //set falg to say start command received
if (startcmdreceived == 1)
{
strcpy(temp, &c[2]); //copy to string to make it include 0x00 termination char needed for atoi
n = atoi(temp); //convert to int
if (n >0)
cmdgtzero = 1; //sets the smdgtzero flag
While ((count -3) != n) //Loop to build command
{
command[(count -3)] = c[count]; //build the command array
count++;
}
}
if (cmdgtzero == 1)
{
crc_temp = generate_16bit_crc(temp, (n+1), CRC_CCITT); //calls crc code and calulates crc from command received
itoa(crc_temp,16,crc); //converts it to hex string
if (strlen(crc) < 4)
{
strncat(crc_pad, crc,4);
}
else
strcpy(crc_pad,crc);
count = (3+n);
While ((count -3 - n) != 4)
{
crctobechecked[(count -3 - n)] = c[count]; //extracts the crc from received command
count++;
}
strcpy(crctobecheckedtemp, crctobechecked); //copies the crc to be cheked to a temp var
if (strcmp(crc_pad, crctobechecked) == 0) //check if both crc's matc
crc_passed = 1; //if matched set crc_passed flag
}
if(strcmp (f[0], c[3 + n + 4]) == 0 && strcmp (e[0], c[3 + n + 5]) == 0)//check if LAST 2 chars ar "FE"
endcmdreceived = 1; //set end command received flag
if (startcmdreceived && cmdgtzero && crc_passed && endcmdreceived ==1)
{ //check that all flags are set
fputs("ACK"); //send "ACK" back to the PC
// delay_ms(500); //waits 500ms
// actioncmd(command); //action the received command
}
Else { fputs("NACK"); } //if not all flags set return a "NACK" back to PC
crc_pad ="0"; //Clean up for next command
n = 0;
charsreceived = 0; //sets all the booleans to 0
crc_passed = 0;
startcmdreceived = 0;
endcmdreceived = 0;
cmdgtzero = 0;
charcount = 0; //Reset charcount
command = " ";
c = " ";
}
delay_ms(250);
}
}
|
I only have little knowledge of C so if i have done something silly please educate me where i have gone wrong.
I am using a PIC 18F4520
compiler ver 5.044
I would be grateful if someone could point me in the right direction to solving this?
Thanks in advance. |
|
 |
alan
Joined: 12 Nov 2012
Posts: 357
Location: South Africa
|
|
| Posted: Thu Jun 04, 2015 2:56 am |
|
|
You do realize that FF are actually one char and not two.
You have to send 0F and 0F for the first two characters to be F and F.
Regards |
|
|
AArdvark
Joined: 09 Mar 2015
Posts: 10
|
|
| Posted: Thu Jun 04, 2015 4:28 am |
|
|
Alan,
these are not hex they are just arbitrary characters used to look for the start and end on the data. or have i miss the point on this? |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9243
Location: Greensville,Ontario
|
|
| Posted: Thu Jun 04, 2015 4:51 am |
|
|
I suggest you go back a couple of steps. Just cut code to receive 'commands' form the PC and display what the PIC receives in the circular buffer. You need to confirm that step 100% before doing the 'parsing' of the buffered data.
this code...
if (strcmp (f[0], c[0]) == 0 && strcmp (f[0], c[1]) == 0)
seems 'complicated' to me and might generate a lot of code.
Perhaps a simple if( c[0]=='F') might be better?, easier to read, faster?
Have to admit I'm outside farming, no time for PICking this week...
just an idea.
Jay |
|
|
AArdvark
Joined: 09 Mar 2015
Posts: 10
|
|
| Posted: Thu Jun 04, 2015 5:01 am |
|
|
the 'circular buffer' is working fine i can see its getting the correct data.
i thought i tried using if( c[0]=='F') but it didn't work which is why i came up with what i used maybe i will retry it.
Thanks |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9243
Location: Greensville,Ontario
|
|
| Posted: Thu Jun 04, 2015 5:30 am |
|
|
good news you know the incoming data is fine..
just had a silly idea. Unless you 'reset' the buffer position counter then you don't necessarily have an 'F' in C[0] meaning the start of the data string.
I didn't read your code too closely but you need some 'method' to KNOW the start of the data, c[0] isn't it necessarily.
Jay |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Thu Jun 04, 2015 6:26 am |
|
|
| Code: | int charcount = 0; //counts the chars comming in on the serial port
...
char c[18] = " "; //string sent to the PIC
...
while(bkbhit)
{
c[charcount] = bgetc(); //get char from input and put it into 'c'
charcount++; //increment to move char array on by one
charsreceived = 1; //set flag to say chars received
if (charcount >18)
break;
} |
The loop will repeat until 19 characters have been received. this means you have a buffer overflow on array c[] and the next message will be invalid because the first byte was already processed (unless you are actually sending 19 bytes?).
| Code: | | crc_pad ="0"; //Clean up for next command |
This is not how you copy a string in C. It is valid in Java or C++ where the string is an object, but in C it is just a pointer to memory. What you have done here is changing the crc_pad pointer to point to the constant string "0" in memory. The pointer is no longer directed at you 5 bytes allocated memory. In the following strcpy() and strncat() calls you will then overwrite data in memory with unpredictable results.
Use strcpy() instead. Fix this in all other locations as well.
Note: this notation is valid for variable initialization so only fix the three locations where you assign a new string.
A few minor remarks on coding style:
1) You are mixing the type declaration styles: int, char, short, int16, int32. The size of 'short' and 'int' are different on other compilers. To avoid confusion and to make porting your code easier it is advised to not use these declarations. Only use int1, int8, int16 and int32
2) For boolean variables it is more clear to use 'TRUE' and 'FALSE' rather than 0 and 1.
3) Names like 'crctobecheckedtemp' are difficult to read. Enhance readability by marking the start of each word. Either use underscore characters like crc_to_be_checked_temp or use camel casing like crcToBeCheckedTemp.
4) Don't use 'magic' values, use a defined value instead. The value '18' is several times in your code. It's not clear what this value stands for, and just in case you ever change the buffer size there is a risk you forget to change one instance. Better: | Code: | #define COMMAND_LENGTH 18
...
char c[COMMAND_LENGTH] = " "; //string sent to the PIC
char command[COMMAND_LENGTH] = " "; //final command string
etc. |
| Code: | | strcpy(temp, &c[2]); //copy to string to make it include 0x00 termination char needed for atoi |
This is bad coding.
strcpy() will not automagically add a terminating zero. In fact, this is a well known security problem in the C language. The strcpy function will copy until the first zero value it finds. If the terminating zero is not where you expect it to be this could mean it copies lots of data and overwrites memory outside your array.
Since your command has a known fixed length you could do something like | Code: | #define COMMAND_LENGTH 18
char c[COMMAND_LENGTH + 1]; //string sent to the PIC. '+1' for terminating zero.
c[COMMAND_LENGTH] = 0; // Add string termination needed for atoi. |
|
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1354
|
|
| Posted: Thu Jun 04, 2015 8:45 am |
|
|
I don't have the CCS manual in front of me here, but is
| Code: |
if (strcmp (f[0], c[0]) == 0 && strcmp (f[0], c[1]) == 0)
|
even valid? I thought strcmp took pointers to strings. f[0] and c[0] and c[1] are just characters. For strings you would want &f[0], &c[0], and &c[1].
Without seeing the manual, I would guess strcmp is taking the character values and using them as random addresses instead of using addresses of the string itself. |
|
|
AArdvark
Joined: 09 Mar 2015
Posts: 10
|
|
| Posted: Fri Jun 05, 2015 8:15 am |
|
|
| Thanks for all your comments, I haven't been well lately so I haven't yet try them. I will do and. I also think know you have prompted me to do a bit more reading and research. |
|
|
|
|
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
