 |
 |
| View previous topic :: View next topic |
| Author |
Message |
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
| Setting Delimiters |
 Posted: Thu Jul 26, 2007 5:04 pm |
 |
|
Hi all,
I am using a serial based routine based on the ex_sisr code.
Could anyone tell me how to filter the characters coming in to detect 2 delimiters?
I am trying to use * then # as delimiters. I would like to return from the RDA ISR if the first 2 characters are not correct.
Any suggestions wouls be welcome.
Cheers
Mark
_________________
Life is too short to avoid asking questions |
|
 |
treitmey
Joined: 23 Jan 2004
Posts: 1094
Location: Appleton,WI USA
|
|
|
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
| Calculating the time |
| Posted: Fri Jul 27, 2007 2:41 pm |
|
|
Hi There,
I already take in the string to a buffer then check the buffer but it is too slow responding.
How can I calculate what the time taken to process the RDA ISR should be?
I have no delays anywhere yet if I send strings every 20mS the comms just gives up and jams the programme.
Cheers
Mark
_________________
Life is too short to avoid asking questions |
|
|
Ttelmah
Guest
|
|
| Posted: Sat Jul 28, 2007 3:00 am |
|
|
You need probably to rethink your whole 'approach', if there is a speed problem working outside the ISR.
I'd suggest an approach something like this:
| Code: |
//Use EX_SISR. Note _vital_ that your buffer size is a binary size:
//4,8,16,32 etc.. If not, the '%' operation in the interrupt handler, will
//switch to having to use a division, instead of a bitwise &, and will
//take hundreds of times longer.....
#define SEARCHING (0)
#define DELIM1 (1)
#define HDR (2)
#define INMSG (3)
void main() {
int8 search_state=0;
int8 temp;
enable_interrupts(global);
enable_interrupts(int_rda);
do {
if(bkbhit) {
temp=bgetc();
switch (search_state) {
case SEARCHING:
//Here just test for the first delimiter
if (temp=='*') search_state=DELIM1;
break;
case DELIM1:
//Now test for the first character of the message
if (temp=='A') search_state=HDR; //put your character here.
else search_state=SEARCHING; //start again if not
break;
case HDR:
//Now just look for the second character in teh message
if (temp=='B') search_state=INMSG; //put second char here
else search_state=SEARCHING;
break;
case INMSG:
//Now do what you want with the message
if (temp!='#') {
//process sequential message bytes here
}
else {
//possibly deal with complete message here
search_state=SEARCHING;
}
break;
}
} while (TRUE);
}
|
This is a classic example, of a 'state machine'. Each time round the loop, the code has a 'state' (search_state), and knows what it has to test for in this state. It checks just this, and nothing else. So, when sittng 'idle', it just looks for '*'. If it finds an *, it then starts looking for a 'A', and then moves to looking for a 'B'. Once it has found all of these in sequence, it arrives at the 'INMSG handler, and processes the message, till it sees '#'.
The point is that the work in each state is tiny. Though the code itself becomes long, and can be complex (if for example, there are two different possible 'headers', the state may advance to two different places according to which is seen), the time per loop is short. Combined with a buffer, this can handle very high rates if properly written. I have a similar 'parser', that searches through data arriving at 56000bps, wth some dozens of different messages (the 'INMSG handler itself switches to a second state machine that walks through a tree of possible message), which has been running for over ten years, in trading floors across the world, without missing a beat.
Best Wishes |
|
|
inservi
Joined: 13 May 2007
Posts: 128
|
|
| Posted: Sat Jul 28, 2007 3:52 am |
|
|
Hello Mark,
Ttelmah give you a very good solution.
I can propose a few enhancement for win some time more.
First in the 'serial_isr()' procedure, the check for Buffer full is not necessary for two reason. First, if the buffer is full, there is no warning and no special work can be do for recover the data loosing! second as some data is loosed anyway, why testing it !
With no flow control, the pic MUST be sure to get all characters sended !
I suggest : | Code: | void serial_isr() {
buffer[next_in]=getc();
next_in=(next_in+1) % BUFFER_SIZE;
} |
I think its enough !
Second, In the 'bgetc()' function, i do not like the 'while(!bkbhit) ;' because the main program call bgetc() after testing bkbhit. I propose to delete this line.
Thirth enhancement,
I suppose that the two delimiters are sended at the start of message. If not then the 'state machine' proposed by Ttelmah is better.
If the order of the delimiters is not important, you can simply count the number of delimiters and reset if there is no delimiters as :
| Code: |
...
int delimCount = 2;
...
if(bkbhit) {
temp=bgetc();
if (temp=='*' || temp=='#') --delimCount ;
else delimCount = 2 ;
if ( delimCount == 0 ) {
delimCount = 2 ;
while ( delimCount == 2 ) {
if(bkbhit) {
temp=bgetc();
if (temp=='*' || temp=='#') --delimCount ;
...
// your code to do when the delimiters are
received
...
}
}
}
...
}
...
|
Best regards,
dro
_________________
in médio virtus |
|
|
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
|
| Posted: Sun Jul 29, 2007 7:01 pm |
|
|
Thanks you both for your suggestions.
I will test them both in my application and come back to you with some feedback.
Cheers
Mark
_________________
Life is too short to avoid asking questions |
|
|
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
| More Questions |
| Posted: Tue Jul 31, 2007 3:31 pm |
|
|
Some feed back for Ttelmah and inservi:
Guys, I have had an attempt at combining your sugesstions. All looked well at first however when I received a complete string into the buffer then tried another with an incorrect delimiter it was still received.
I have simplified things so that I just look for a * then take in the string until I see a <cr>.
Here is my attempt:
| Code: | #include <18F4620.h>
#fuses HS,NOWDT,NOPROTECT,NOLVP
#use delay(clock=20000000)
#use rs232(baud=19200, xmit=PIN_C6, rcv=PIN_C7)
int1 valid_flag=0,string_flag = 0,eom_flag = 0;
int i = 1 ;
int search_state=0;
int temp;
long loop=0;
#define valid PIN_B3
#define SEARCHING (0)
#define MESSIN (1)
#define BUFFER_SIZE 32
BYTE buffer[BUFFER_SIZE];
BYTE buffer1[BUFFER_SIZE];
BYTE next_in = 0;
BYTE next_out = 0;
#int_rda
char serial_isr() {
int t;
buffer[next_in]=getc();
t=next_in;
next_in=(next_in+1) % BUFFER_SIZE;
}
#define bkbhit (next_in!=next_out)
BYTE bgetc() {
BYTE c;
c=buffer[next_out];
next_out=(next_out+1) % BUFFER_SIZE;
return(c);
}
void main() {
enable_interrupts(global);
enable_interrupts(int_rda);
do {
++loop;
if(bkbhit)
{
temp=bgetc();
switch (search_state)
{
case SEARCHING: if (temp=='*') buffer1[0] = temp; search_state=MESSIN; break;
// Test each character for a * until we find one
case MESSIN:
// Start taking in the string until we find a CR
if(temp != '9')
{
buffer1[i] = temp;// Buffer what comes in
++ i;
}
if(temp =='9')
{
for(i=0;i<10;++ i)
putc(buffer1[i]);// Print it back for testing
}
search_state=SEARCHING;// We have a string so reset the search state
valid_flag = 1;
break;
}
}
if(valid_flag ==1) // Turn on the valid LED
{
valid_flag = 0;
output_high(valid);
loop = 0;
}
if(loop >= 40000) // Wait for a number of loops and turn the valid LED off
{
output_low(valid);
}
} while (1);
}
Can either of you see where I am going wrong?
Cheers
Mark |
_________________
Life is too short to avoid asking questions |
|
|
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
| Please ignore the last post!! |
| Posted: Tue Jul 31, 2007 9:12 pm |
|
|
Hi All,
I have found a couple of my errors from the last post.
This one actually runs and filters the * character But.....
the buffer gets very muddled so i cant pass this to the rest of my loop yet.
Any thoughts?
| Code: | /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//// Interrupt Comms8.C
////
//// This program implements an interrupt service routine to buffer up incoming serial data.
//// The data string must start with a * and will terminate with a <CR>
////
////
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
#include <18F4620.h>
#fuses HS,NOWDT,NOPROTECT,NOLVP
#use delay(clock=20000000)
#use rs232(baud=19200, xmit=PIN_C6, rcv=PIN_C7)
int1 valid_flag=0,string_flag = 0,eom_flag = 0;
int i,j ;
int search_state=0;
int temp;
long loop=0;
#define valid PIN_B3
#define flush PIN_B2
#define on output_high
#define off output_low
#define tog output_toggle
#define SEARCHING (0)
#define MESSIN (1)
#define BUFFER_SIZE 32
BYTE buffer[BUFFER_SIZE];
BYTE buffer1[BUFFER_SIZE];
BYTE next_in = 0;
BYTE next_out = 0;
void CommRxFlush(void);
#int_rda
char serial_isr() {
int t;
buffer[next_in]=getc();
t=next_in;
next_in=(next_in+1) % BUFFER_SIZE;
}
#define bkbhit (next_in!=next_out)
BYTE bgetc() {
BYTE c;
c=buffer[next_out];
next_out=(next_out+1) % BUFFER_SIZE;
return(c);
}
void main() {
enable_interrupts(global);
enable_interrupts(int_rda);
do {
++loop;
if(bkbhit)
{
temp=bgetc();
switch (search_state)
{
case SEARCHING:
// Test each character for a * until we find one
if (temp=='*')
{
search_state=MESSIN;
}
else
search_state=SEARCHING;
break;
case MESSIN:
// Start taking in the string until we find a CR
for(j=0;buffer[j]!='\r';++j)
{
fputc(buffer[j]);
}
search_state=SEARCHING;// We have a string so reset the search state
valid_flag = 1;
on(flush);
CommRxFlush();
break;
}
}
if(valid_flag ==1) // Turn on the valid LED
{
valid_flag = 0;
on(valid);
loop = 0;
}
if(loop >= 60000) // Wait for a number of loops and turn the valid LED off
{
off(valid);
off(flush);
loop = 0;
}
} while (1);
}
void CommRxFlush(void)
{
disable_interrupts(int_rda); //disable USART Rx interrupt
//reset all Rx indexes and counters
next_in = 0;
next_out = 0;
search_state=SEARCHING;
eom_flag = 0;
enable_interrupts(int_rda);
}
|
Cheers
Mark
_________________
Life is too short to avoid asking questions |
|
|
inservi
Joined: 13 May 2007
Posts: 128
|
|
| Posted: Wed Aug 01, 2007 1:38 am |
|
|
Hello,
Here are tree idee:
| Code: | | if ( temp == ' * ' ) { // you test temp with 3 chars ' * '. try to compare with '*' not ' * ' |
| Code: | #int_rda
char serial_isr()
{
//int t; // loosing time and space for nothing
buffer[next_in] = getc ( );
//t = next_in; // loosing time and space for nothing
next_in = ( next_in + 1 ) % BUFFER_SIZE;
}
|
| Code: | case MESSIN:
// Start taking in the string until we find a CR
// --This loop is not right for two reasons --------------
// --1) use bgetc ( ) for read the buffer !!! ------------
// --2) use the principal loop for process the buffer because the '\r' is probably not yet in the buffer after the ! ----
// for ( j = 0; buffer[j] != '\r'; ++j )
// {
// fputc ( buffer[j] );
// }
// -------------------------------------------------------
// in the 'case', just treating one char at a time
if ( temp != '\r' )
{
fputc ( temp );
}
else
{
search_state = SEARCHING; // We have a string so reset the search state
valid_flag = 1;
on ( flush );
// it is normaly not necessary to flushing the buffer
CommRxFlush ( );
}
break;
|
if you absolutely need a loop in the 'case', try something like that
| Code: | case MESSIN:
while ( temp != '\r' )
{
fputc ( temp );
while ( !bkbhit ) ;
temp = bgetc ( );
}
search_state = SEARCHING; // We have a string so reset the search state
valid_flag = 1;
on ( flush );
// it is normaly not necessary to flushing the buffer
CommRxFlush ( );
break;
|
_________________
in médio virtus |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Wed Aug 01, 2007 1:56 am |
|
|
| Code: | #int_rda
char serial_isr() {
int t;
buffer[next_in]=getc();
t=next_in;
next_in=(next_in+1) % BUFFER_SIZE;
} |
In the original example code there are two more lines at the end of this function: | Code: | if(next_in==next_out)
next_in=t; // Buffer full !!
|
These lines are protecting your buffer from overwriting data that has not been read yet. Why did you remove these lines? |
|
|
inservi
Joined: 13 May 2007
Posts: 128
|
|
| Posted: Wed Aug 01, 2007 2:38 am |
|
|
Hello Ckielstra,
I proposed to remove this test of buffer overflow because it is not usefull. If the PIC cannot read rather quickly, some characteres will be lost any way. If characteres are lost, the main loop does not know it and the loss is not treated.
The result is in any way a loss of characteres.
I think that to remove this tests decreases the waste of time and thus the risk of loss of characteres.
Either the PIC is fast enough to receive all or not!
If the PIC is not fast enough, the only good way of dealing with this problem, consists in implementing a flow control with hand check.
What do you think about ?
Best regards,
dro.
_________________
in médio virtus
Last edited by inservi on Thu Aug 02, 2007 7:09 am; edited 1 time in total |
|
|
treitmey
Joined: 23 Jan 2004
Posts: 1094
Location: Appleton,WI USA
|
|
| Posted: Wed Aug 01, 2007 2:24 pm |
|
|
| Code: | #define VER_MINOR 05
#define VER_MAJOR 0
#include <18F4525.h>
#use delay(clock=20000000)
#fuses HS,NOWDT,NOLVP,PROTECT,PUT,BROWNOUT
//#use rs232(baud=19200,xmit=PIN_B3,INVERT,stream=DEBUG,DISABLE_INTS)
#use rs232(baud=19200,xmit=PIN_B3,INVERT,stream=DEBUG)
#use rs232(baud=9600,errors,xmit=PIN_C6,rcv=PIN_C7,stream=PC)
#case
#zero_ram
#define INTS_PER_SECOND_TMR2 77
#define BUFF_SZ 32
int8 rx_buf[BUFF_SZ];
int8 rx_i = 0;
int8 rx_o = 0;
int8 int_count_tmr2;
int8 timer[8];//set a side 8 different 1sec timers
#define tmr_led timer[0]
//=== prototypes====
init();
int8 bgetc();
//======================= Main ==============================
void main() {
enum states {Looking,Found_star,Found_both};
states rx_state=Looking;
int8 rxdata=0;
init();
while(1)
{
while(rx_i!=rx_o)//do this loop when data is recieved
{
if(rx_state==Looking && bgetc() =='*') rx_state=Found_star;
if(rx_state==Found_star) //only if "Found_star" do we read another char
{
if(bgetc() =='#') rx_state=Found_both;//check if next char is # else reset state
else rx_state=Looking;
}
if(rx_state==Found_both)//if I found both, read 1 char at a time
{
fprintf(DEBUG,"0x%2X ",bgetc());
}
}
}
}
init() {
setup_adc_ports(NO_ANALOGS);
fprintf(DEBUG,"Starting\n\r");
fprintf(DEBUG,"test V%u.%02u\n\r",VER_MAJOR,VER_MINOR);
enable_interrupts(GLOBAL);
enable_interrupts(INT_RDA);
enable_interrupts(INT_TIMER2);
setup_timer_2(T2_DIV_BY_16,255,16);
set_timer2(250);
int_count_tmr2 = INTS_PER_SECOND_TMR2;
}
//=======================bgetc============================//
int8 bgetc() {
int8 c;
c=rx_buf[rx_o];
rx_o=(rx_o+1) % BUFF_SZ;
return(c);
}
//=======================int_rda============================//
#int_rda
char serial_isr() {
int t;
rx_buf[rx_i]=fgetc(PC);
rx_i=(rx_i+1) % BUFF_SZ;
}
//=======================timer 2 ISR============================//
#INT_TIMER2 // This function is called every time
void timer2_isr() { // timer 2 overflows (250->0), which is
int8 x;
if(--int_count_tmr2==0){ // approximately 15 times per second for this program.
//fprintf(DEBUG,".");//1 sec heartbeat
for (x=0;x<sizeof(timer);x++){
restart_wdt();
if (timer[x]>0) timer[x]--;//reduce individual timer counts until all==0
}
int_count_tmr2 = INTS_PER_SECOND_TMR2;
}
}
|
|
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Wed Aug 01, 2007 3:14 pm |
|
|
| inservi wrote: | I proposed to remove this test of buffer overflow because it is not usefull. If the PIC cannot read rather quickly, some characteres will be lost any way. If characteres are lost, the main loop does not know it and the loss is not treated.
The result is in any way a loss of characteres.
I think that to remove this tests decreases the waste of time and thus the risk of loss of characteres.
Either the PEAK is fast enough to receive all or not!
If the PIC is not fast enough, the only good way of dealing with this problem, consists in implementing a flow control with hand check.
What do you think about ? |
I guess it is matter of taste. As you correctly said data will be lost anyway, so it is the higher software level that will have to detect and correct for this.
Hmmm, this makes me think. In this particular project we are only testing for start and end markers but to make it really complete there should also be a mechanism added for proofing the data is correct. In the code library examples can be found for several CRC calculation routines. |
|
|
Mark Weir
Joined: 11 Sep 2003
Posts: 51
Location: New Zealand
|
| Packet Setup |
| Posted: Wed Aug 01, 2007 3:25 pm |
|
|
Hi Ckielstra,
I am just working through the response from Treitmey. Just to clarify the packet we use is constructed thus:
* # 111122223333................ cksum1 chsum2 cksum3 <cr>
The start is *# the 1111 is a type code any number between 0 and 9999, the 2222 is an address any number between 0 and 9999, the 3333 is a command any number between 0 and 9999.
Then we send some data bytes ........, this can be any number from 1 to 100 bytes, then we have a checksum stored in three bytes. The checksum comes from adding the total of each preceeding byte but as a rolling byte so that the answer is between 0 and 255, the answer digits are sent in three bytes then finally we send a <cr> to end the string.
data heading back from these modules is identical in form it just uses ## at the start of the string.
I am very greatful for all the help I am getting here, I have struggled with this for a while now.
I will say though that i am learning a great deal in the process.
Cheers
Mark
_________________
Life is too short to avoid asking questions |
|
|
Ttelmah
Guest
|
|
| Posted: Thu Aug 02, 2007 2:10 am |
|
|
I hope your fields, actually vary from '0000'to '9999', rather then '0' to '9999' as you say!. In the latter case, parsing would basically be impossible...
What marks the end of the 'up to 100 bytes'?. Is it a space as you show?.
Best Wishes |
|
|
|
|
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
