| View previous topic :: View next topic |
| Author |
Message |
julienm
Joined: 28 Jul 2020
Posts: 31
|
| Homemade string compare randomly fails [Solved] |
 Posted: Thu May 19, 2022 1:34 pm |
 |
|
I'm chasing a bug that appears sporadically after hours of operations: some portion of code gets called when it should not.
Using the ICD debugger I was able to narrow it down to a section processing the text received over rs232.
I discovered some code in charge of comparing the received command with constant strings in order to call the corresponding functions:
The debugger showed me the PIC had gone into
| Code: | if( startsWith(cmd_buf, "fbon") )
{
// do something
} |
although cmd_buf was equal to "1led0"
Now the code behind startsWith is a bit questionnable, but still I don't understand why it would work 99.9% of the time:
| Code: | #inline
BOOLEAN startsWith(char *txt, char *toCompare)
{
int16 size = strlen(toCompare);
int16 i = 0;
while( i < size )
{
if( txt[i] != toCompare[i] ) return FALSE;
i++;
}
return TRUE;
} |
Please note we have #DEVICE PASS_STRINGS = IN_RAM
CCS Compiler is at revision 5.080 (I bought an upgrade just today)
So, I now have 2 goals.
1. What's the explanation for startsWith() returning TRUE with different strings
2. What's the clean way to rewrite it (I suppose using the built-in strcmp)
Thanks in advance for your help
EDIT: same issue with 5.109
I added another call to startsWith(cmd_buf, "fbon") inside the if block but when stepping into it, it properly return FALSE (and I can't see much with the debugger)
I'm now trying without the #inline directive |
|
 |
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Thu May 19, 2022 8:08 pm |
|
|
1. What is your PIC ?
2. Post a complete test program that shows all variable declarations
and that compiles without errors or warnings.
The test program should be very short and only have code in it that is
essential to show the problem (and to compile without errors). |
|
|
julienm
Joined: 28 Jul 2020
Posts: 31
|
|
| Posted: Fri May 20, 2022 3:04 am |
|
|
My PIC is a 18F6722
Regarding a reproducer it is going to be quite difficult, because the PC side sending commands over RS232 is needed.
However I'm almost sure that it has to do with the #INT_RDA occurring in the middle of startsWith().
Would an interrupt explain the problem?
What could be made to make it robust to interrupts?
Last edited by julienm on Fri May 20, 2022 4:32 am; edited 1 time in total |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19215
|
|
| Posted: Fri May 20, 2022 4:02 am |
|
|
No, and interrupt should not change anything, _unless_ your interrupt
handler has a problem.
Post how you are handling the RDA interrupt, and the variable
declarations for everything used in this. Also the RS232 setup. |
|
|
hmmpic
Joined: 09 Mar 2010
Posts: 314
Location: Denmark
|
|
| Posted: Fri May 20, 2022 5:35 am |
|
|
Why not put a printf() in the top of your function, and see the string you compare.
int16 for long string i understand, in you case you have about 3 char... |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9097
Location: Greensville,Ontario
|
|
| Posted: Fri May 20, 2022 6:02 am |
|
|
As others have said, really we need to see your code.
One possible problem may be the potential length of the variables ?
startsWith() has the 'size' defined as an int16. that's a HUGE number, 'length' of incoming characters from the PC.
From a 'concept', I doubt the incoming strings would be more than 255 characters, so an int8 would 'fit'. Your 2 examples are 4 bytes.
It might be the compiler has created 'complicated' code that somehow 'stalls' or misreads the data.
I'm only guessing here, trying to think of possible ways it fails.
Bottom line, need to see the actual code and a list of 'commands' from the PC. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19215
|
|
| Posted: Fri May 20, 2022 9:26 am |
|
|
One thought comes to me, if he is using a buffer that is over 255
characters long, and handling things like checking if it has data by
comparing int16's that are updated in the INT_RX. Result could be
a potential issue with the comparison, if the interrupt fires between
the first byte of the int16 being accessed and the second. |
|
|
PrinceNai
Joined: 31 Oct 2016
Posts: 452
Location: Montenegro
|
|
| Posted: Fri May 20, 2022 11:05 am |
|
|
The master PIC must somehow send out the data that is terminated in some distinct way, for the receiving PIC to know when one message ends. Given the speed of the PIC compared to the speed of the serial, there shouldn't be an issue of something else coming in and also complete while the previous message isn't serviced yet.
As all the others have said, it would be much easier if you posted the receiving code, how you store the received data and WHEN you are working with it. Delays can do interesting things. |
|
|
julienm
Joined: 28 Jul 2020
Posts: 31
|
|
| Posted: Fri May 20, 2022 2:55 pm |
|
|
Thanks to all who took time to answer!
Ok so you think the startsWith() function cannot get corrupted by an interrupt?
| Code: | | #use rs232(stream = UART1, baud = baudrate, bits = 8, parity = N, xmit = PIN_C6, rcv = PIN_C7, ERRORS, DISABLE_INTS) |
This is how the RDA interrupt is handled.
The received character is stored in a circular buffer of 32 characters.
| Code: | #int_RDA
/* Handler Interrupt for UART1 RX */
void RDA_isr()
{
int8 t;
in_buf[next_in] = getc(); // get char from UART1
t = next_in;
if( in_buf[next_in] == CR_ASCI ) get_cmd_f++;
next_in = (next_in + 1) % IN_BUFF_SIZE;
if( next_in == next_out ) next_in = t; // Buffer full !!
} |
When a \r character is received, get_cmd_f is incremented and in the main loop if get_cmd_f>0 then it processes the content of the buffer and copies it into cmd_buf.
| Code: | int1 get_command()
{
int8 i = 0;
enable_Cts; // block next commands from the PC
while( next_in != next_out )
{
// Copy character from RS232 buffer to cmd buffer and increment index
cmd_buf[i] = in_buf[next_out];
next_out = (next_out + 1) % IN_BUFF_SIZE;
// Carriage return encountered: stop here and replace it with \0
if( cmd_buf[i] == CR_ASCI )
{
get_cmd_f--;
cmd_buf[i] = '\0';
return 1;
}
i++;
}
// No CR but end of buffer encountered, this should not happen
return 0;
} |
Right after that, if get_command returns 1, then the cmd_buf is parsed in order to decide what to do.
This is when startsWith(cmd_buf, "fbon") is called and return TRUE when it should not.
Regarding "how variable used are declared", here it is.
| Code: | #define IN_BUFF_SIZE 32
volatile char in_buf[IN_BUFF_SIZE];
char cmd_buf[IN_BUFF_SIZE] = "\0";
volatile unsigned int8 next_in = 0;
unsigned int8 next_out = 0;
volatile signed int8 get_cmd_f = 0; |
There is no master PIC, on the other side of the serial port, there's a Windows PC
Regarding printf, I'm usually cautious with that, a bit afraid that calling printf many times can create issues with RS232.
I agree that the command messages are very short, the longest might be 8 characters long |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Fri May 20, 2022 3:43 pm |
|
|
At the beginning of the StartsWith() routine, put in code to print the buffer.
Print it out in hex bytes. Then you will see exactly what's there.
In the program below, I only print the first 25 buffer locations.
Test program:
| Code: | #include <18F46K22.h>
#device PASS_STRINGS=IN_RAM
#fuses NOWDT
#use delay(internal=8M)
#use rs232(baud=9600, UART1, ERRORS)
#include <string.h>
int8 cmd_buf[100] = "fbon - command\n\r";
BOOLEAN startsWith(char *txt, char *toCompare)
{
int16 size = strlen(toCompare);
int16 i = 0;
// Display first 25 bytes in txt array.
for(i=0; i<25; i++)
printf("%x ", txt[i]);
printf("\n\r");
while( i < size )
{
if(txt[i] != toCompare[i]) return FALSE;
i++;
}
return TRUE;
}
//==============================
void main()
{
if(startsWith(cmd_buf, "fbon"))
printf("Found string\n\r");
else
printf("Did not find string\n\r");
while(TRUE);
}
|
Output of the program:
| Quote: |
66 62 6f 6e 20 2d 20 63 6f 6d 6d 61 6e 64 0a 0d 00 00 00 00 00 00 00 00 00
Found string
|
|
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19215
|
|
| Posted: Sat May 21, 2022 5:15 am |
|
|
Aaargh!....
Get rid of DISABLE_INTERRUPTS in your #use RS232.
This should only ever be used for software serial. In your case if means
that receive characters can be lost when you are transmitting on the
RS232. Result could be disaster. |
|
|
ezflyr
Joined: 25 Oct 2010
Posts: 1019
Location: Tewksbury, MA
|
|
| Posted: Sat May 21, 2022 5:35 am |
|
|
Hi,
Another potential problem is that you are using ‘getc’ in your ISR. You have a stream defined, so you should be using ‘fgetc’ with the appropriate stream name.
John
_________________
John
If it's worth doing, it's worth doing in real hardware! |
|
|
julienm
Joined: 28 Jul 2020
Posts: 31
|
|
| Posted: Sat May 21, 2022 1:34 pm |
|
|
Ok, I'll try to print the buffer content when the comparison returns TRUE (my problem is that on the PC side, the software will be spammed with messages, and not only I need to handle this, but it could prevent the random bug to appear)
I can also try to use fgetc, maybe you can explain what's the potential problem @ezflyr?
And remove DISABLE_INTERRUPTS
And use int8 for string sizes.
Just for recall, I already ran the program with the ICD and a break point, and I confirmed that the next call to startsWith does not return TRUE, so something must have changed in between
| Code: | void main()
{
if(startsWith(cmd_buf, "fbon"))
{
if(startsWith(cmd_buf, "fbon")) <- breakpoint here is hit
{
dummy = 1;
}
else
{
dummy = 0; <-- in step by step we end up here
}
}
} |
|
|
|
julienm
Joined: 28 Jul 2020
Posts: 31
|
|
| Posted: Wed May 25, 2022 12:32 pm |
|
|
Ok, so I've tested a few modifications in the past days.
So far nothing prevented the bug to appear:
- changing int16 for int8 in startsWith()
- replacing getc() with fgetc(UART1)
- removing #inline from startsWith() declaration
- removing DISABLE_INTERRUPT from #rs232 statement
I have also added tests and breakpoints for size==0, i!=0 and even i>=size
None of these breakpoints have been hit, despite startWith() returning TRUE when it shouldn't
| Code: | #inline
BOOLEAN startsWith(char *txt, char *toCompare)
{
int8 size = strlen(toCompare);
int8 i = 0;
if( i >= size )
return FALSE; <--------------------- breakpoint here
while( i < size )
{
if( txt[i] != toCompare[i] ) return FALSE;
i++;
}
return TRUE;
} |
I have now added printf("%s-%s", txt, toCompare) just before return TRUE;
It is style cycling and I'm afraid that the change of timing created by this addition will prevent the bug to appear.
Does someone have a clue what else I should test? |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Wed May 25, 2022 1:23 pm |
|
|
| Quote: | I'm chasing a bug that appears sporadically after hours of operations
|
Before doing anything more on software, consider that it could be a hardware bug.
What Vdd voltage are you using for the 18F6722 ? Is it stable ?
There are four sets of Vdd and Vss pins. Are the all connected to power
and ground ?
There are also AVdd and Avss pins. Are they connected to power and ground ?
Do you 100 nF ceramic capacitors connected across all five set of pins
listed above ? Are the caps placed very close to the pins ?
These posts list other things to check:
http://www.ccsinfo.com/forum/viewtopic.php?t=41964&start=1
http://www.ccsinfo.com/forum/viewtopic.php?t=39439&start=1 |
|
|
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
