|
|
View previous topic :: View next topic |
Author |
Message |
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
UART again |
Posted: Mon May 19, 2014 9:51 am |
|
|
One year ago ckielstra and Ttelmah helped me a lot to write a receive procedure, which works perfectly untill now.
The message to be received is as follow
aa fc nn nn 04 xx xx crc CRC
where :
aa is the adress of the set
fc is a function code the set has to proceed
nn nn is a transaction number
xx xx are the datas linked used with the function code
crc CRC is ... as they say themselves
Code: | #INT_RDA
void Receive()
{
char c;
c=getc();
if(I_UART_In_Pos==0 && c==I_Adresse_Box) // The take in account only if I am the correct set
{
INStr[I_UART_In_Pos++]=c; // store the character
B_UART_OK=FALSE;
}
else if(I_UART_In_Pos>0 && I_UART_In_Pos<UART_MAX)
{
INStr[I_UART_In_Pos++]=c;
B_UART_OK=(I_UART_In_Pos>=UART_MAX); // is the buffer full ?
}
L_Cycle_UART=1; // en prévision du Time_out
} |
It works, I think, because the set alway waits for a message beginning by its adress and because there was only one set connected at the same time
But, when have to pilot more than one set at the same time (32 in fact) I want to use a broadcast adress, which I fix to 0
So the sets have to wait for their adress AND adress 0
Code: | if(I_UART_In_Pos==0 &&( c==I_Adresse_Box || c==0x00)) |
But adress 0 can be anywhere in the message, so the set which does not react to the first character because it is not its adress (so I_UART_In_Pos stays =0) will react to the first zero seen in the message
for example, if the message is 01 03 00 03 04 00 0D 8B B3, the set 01 will react OK but the set 02 will react to the 3rd character of the message as it must not.
As the messages are always the same length, I thought I could use a Time out system : If set 02 react to the 3rd character, it will not have the 9 characters at the end of the message, so if I re-initialisate I_UART_In_Pos to 0 before the next message it could be good. Could'nt it ?
So I have introduced a test in a Timer procedure
Code: | #int_TIMER1
void Lit_Switches() // every 13 milliseconds
{
... some code ...
if(I_UART_In_Pos>0)
{
I_UART_TimeUp++;
if(I_UART_TimeUp>=8) // 8 x 13ms = 104ms, it will occurs after the end of the message wich is 75ms
{
I_UART_TimeUp=0;
//I_UART_In_Pos=0;
}
}
} |
But it does not work, and I do not understand why yet
Do you have any idea to help me find a solution to this issue ? |
|
|
ckielstra
Joined: 18 Mar 2004 Posts: 3680 Location: The Netherlands
|
|
Posted: Mon May 19, 2014 10:53 am |
|
|
To avoid your problem many protocols start with a fixed sequence of several known bytes. The chance of this special start sequence occurring in the middle of your message is very small and is a good filter for detecting start of the message.
Disadvantage is that you have to send extra bytes, this takes time and power.
Your solution with a time-out is another approach used in many systems.
One reason for it not working is this line:Remove the comments.
Another potential problem is that you are waiting 8 character times before you reset the receiver. How much time do you have between your messages? When this is less than 8 * 13ms = 104ms then you will miss the start of the next message. Not a really big problem, but you can optimize this a bit when you reset I_UART_TimeUp after every received character. Then you can reset the receiver when for example 4 characters are missing.
Something like this: Code: | #define BROADCAST_ADDRESS (0x00) // Using a define makes your code easier to understand.
#INT_RDA
void Receive()
{
char c;
c=getc();
if ((I_UART_In_Pos==0)
&& ((c==I_Adresse_Box) || (c==BROADCAST_ADDRESS))) // Accept only if I am the correct set
{
INStr[I_UART_In_Pos++]=c; // store the character
B_UART_OK=FALSE;
}
else if ((I_UART_In_Pos>0) && (I_UART_In_Pos<UART_MAX))
{
INStr[I_UART_In_Pos++]=c;
B_UART_OK=(I_UART_In_Pos>=UART_MAX); // is the buffer full ?
}
L_Cycle_UART=1; // en prévision du Time_out
I_UART_TimeUp=0; // reset the receiver time-out.
} |
Code: | #define RECEIVE_TIMEOUT 4 // 4 x 13ms = 52ms. After receiving no new characters for this time we reset the receiver logic.
#int_TIMER1
void Lit_Switches() // every 13 milliseconds
{
... some code ...
if(I_UART_In_Pos>0)
{
I_UART_TimeUp++;
if (I_UART_TimeUp >= RECEIVE_TIMEOUT)
{
I_UART_TimeUp=0;
I_UART_In_Pos=0;
}
}
} |
|
|
|
asmboy
Joined: 20 Nov 2007 Posts: 2128 Location: albany ny
|
|
Posted: Mon May 19, 2014 5:32 pm |
|
|
Rather than the ISR + timer route you are struggling with, have you thought about simply catching ALL the chars in a standard circular serial receive buffer
and sorting out what is in the buffer in MAIN(); at your leisure ??
Far simpler really and very fast in and out of the ISR -
with a big enough circular receive buffer - you can sort out what you have
in the foreground and never risk a dropped character.
Your timeout-timing can then be a foreground process as well, possibly not even needing a ISR for timer service either. |
|
|
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
|
Posted: Tue May 20, 2014 2:16 am |
|
|
Thanks friends, for your message
To ckielstra
Quote: | One reason for it not working is this line:
Code:
//I_UART_In_Pos=0;
Remove the comments. |
I comment because it does not work on my panel. I have a spy on the time out procedure and I observe that it loses one shot on two
I mean that the supervisor queries 2 sets, nbr 01 and nbr 02. On my panel, which is nbr 01, reacts to query 01, sleep query 02 SLEEP query 01 sleep query 02 and reacts query 01
The time out procedure should go on RECEIVE_TIMEOUT x 13 ms on each query 02, but it goes on 2,4 seconds (query repetition time : approx 0,5s)
And for the time beeing, I have not understood yet what happens
To asmboy
Quote: | with a big enough circular receive buffer - you can sort out what you have
in the foreground and never risk a dropped character. |
You mention a good proposal, I will study today also, so I will have two possibilities if I need.
The supervisor queries either :
aa 03 nn nn 04 xx xx crc crc where aa is 01 to 20 (32 sets maxi on the line) (03 is the JBus command to read many registers)
or 00 06 nn nn 04 xx xx crc crc which is the brodcast command (06 is the Jbus command to write a register)
So I could look for the sequence (I_Adresse_Box 03 or 00 06) && (5th character ==04) ?
That means I store 5 characters, if OK I store the 4 followers, if not I left shift the 5 characters to accept a new 5th one
Do you think it could be a correct method ? |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19513
|
|
Posted: Tue May 20, 2014 2:55 am |
|
|
Have to agree.
Thing is that a well written circular buffer, even at quite a small size, then allows you to take more than one character time between things being handled. A 'classic' problem would be if (for instance) at the end of line, the code then performs calculations to check the CRC, followed by some other processing of the data. It is always quite easy for this to take more than a character time, and then things start to go wrong....
I tend to use a 'per character' parser, which whenever a character arrives, uses a state machine that is stepping forwards through the expected values and actions for each, performing the required operations.
Now, I have to suggest that you 'reconsider' the format if you can. The problem you are seeing, is fundamental, with any message where the same value is used potentially inside a message, and in an address or control. Though a 'timeout' may allow a form of recovery, this doesn't appear to be guaranteed, since what happens if two successive messages both contain the same address?.
Using some form of unique marker, is the best way to avoid problems. As a 'suggestion', consider setting your UART to 9bit mode, and setting bit 9, in the address only. The cost is 11 bits per byte sent, versus the current 10 bits, so just 10 percent. Then your handler can just look for bit 9 being set, and when it is, use this as the 'address' marker. Nice thing then is that even if a byte is lost, or an extra one received so a count goes faulty, you can still recover. The bytes received with the count 'wrong' can be rejected when the CRC doesn't work, and the new start of packet location identified.
Best Wishes |
|
|
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
|
Posted: Tue May 20, 2014 3:18 am |
|
|
Thanks Ttelmah
Quote: | Now, I have to suggest that you 'reconsider' the format if you can |
But I can't, the supervisor is an existing product I cannot change, I have to do with |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19513
|
|
Posted: Tue May 20, 2014 3:55 am |
|
|
Is there a guaranteed time gap between the messages?.
If so, then this can be used as the marker. Reset a timer to a value that will not reach it's limit, between standard characters, each time the character is received. Then if the timer interrupt is called, you are in the gap between messages.
Unless there is a guaranteed time marker, then the message is really badly designed (I'm not even sure 'designed' would be the applicable word....). Without something to distinguish the start of message, you degenerate into 'trial and error'. You'd have to use the count approach, with a state machine, and when the limit count is reached, check the CRC. If this is OK, accept the message, and pull the address from the expected byte. If it isn't, throw away the first character from the sequence being checked, and loop back to checking. There is still a tiny chance of accepting a wrong message, but this is as close as you'll get. |
|
|
temtronic
Joined: 01 Jul 2010 Posts: 9226 Location: Greensville,Ontario
|
|
Posted: Tue May 20, 2014 5:22 am |
|
|
Is the 5th character always '04' ?
If so, using a circular buffer makes it possible,though 'messy'.
1) capture a datastream.
2) locate the '04'.
3) create a new buffer 'test_buffer[]',filling it with the previous 4 bytes before the '04' and those after to the crc bytes.
4) run a 'CRC checking' function to confirm/deny this 'test_buffer' has the correct data.
5) if correct, continue your program, parsing address,command,etc.
6) if NOT correct, go back to step 1, try again.If your 'raw data' buffer is large enough,just find the next '04' and retest.
This might work but ONLY if '04' is always the 5th element of the datastream.
As others have said you really NEED a 'delimiter', a 'reference' or 'starting point' in order to properly capture and deal with the data.
hth
jay |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1349
|
|
Posted: Tue May 20, 2014 6:32 am |
|
|
Another "messy" way would be to do the ISR buffer they are talking about and in the main do a multi-pass CRC check until you find a valid CRC, then go back and parse the message to see if it is for your address or the broadcast address
So if you had the byte stream:
b00 b01 b02 b03 b04 b05 b06 b07 b08 b09 b0a b0b b0c b0d b0e b0f b10
So the first pass would use b0-b8, checking versus the CRC bytes in b7 and b8. If the computed CRC matches those, go back and look b0 to see if it is your address or a broadcast address.
If not, look at b01-b09, assuming b8 and b9 are the CRC bytes. If this doesn't pass, do b02-b0a, etc.
This is tricky because it is a lot of processing of the same data, so you'll have to figure out how to manage processing time, getting the data out of the circular buff, and possibly adding new bytes to the end of your saved copy if new ones come in while you processing earlier ones.
This isn't a 100% solution, but since you cannot change the supervisor, it might make due. Really the best solution is to talk with your management and do a new revision on the supervisor firmware and add new parts to the protocol (or rewrite it).
Last edited by jeremiah on Tue May 20, 2014 8:03 am; edited 1 time in total |
|
|
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
|
Posted: Tue May 20, 2014 7:13 am |
|
|
Yes the 5th is always 0x04 because the message is always 9 bytes long and the MODBUS standart implies the succession of set adress, function code, content. In content is included remaining bytes quantity in the message, which counts data by itself = 2 bytes plus CRC = 2 bytes -> total 4 bytes
So, the 5th is always 0x04
Taking that in account, I presently am testing a solution like as follow, according to asmboy idea
Code: | #define UART_MAX_NEW 18
#define UART_MESSAGE_LENGTH 9
int UART_Cursor=0;
char UART_INStr[UART_MAX_NEW]; |
In the UART RECEIVE Interrupt, I store the successives char in an array, and store the next empty location in UART_Cursor variable
If UART_Cursor is higher than 8, that means a complete message length has been received, so I position B_UART_OK flag
Code: | #INT_RDA
void Receive()
{
char c;
c=getc();
UART_INStr[UART_Cursor++]=c;
B_UART_OK=(UART_Cursor>=UART_MESSAGE_LENGTH);
if(B_UART_OK) Spy_HI();
} |
I still have to manage when accidentally UART_Cursor is higher than UART_MAX_NEW
In main procedure, if B_UART_OK is positioned, I test whether the 9 bytes are a correct message possible or not
Code: | void main()
{
//.../...
while(TRUE)
{
//.../...
if(B_UART_OK)
{
if(IsMessageOK())
{
GetData();
}
}
//.../...
}
} |
The validation of the message is as follow
is the first == BROADCAST_ADDRESS and the 2nd == function code 0x06 or the first == Adress_box and the 2nd == function code 0x03
is the 5th == MODBUS_DATA_LENGTH == 0x04
If yes, the message is a valid message and I treat it (I presently test CRC in treatment)
I first position the UART_Cursor to the correct position : -1 position if the message is not correct, - message length if it is correct
Code: | Boolean IsMessageOK()
{
int I;
Boolean B_OK=FALSE;
Spy_MI();
// 8 char at least are in UART_INStr
if(UART_INStr[0]==BROADCAST_ADDRESS) // Broadcast, so I need it
{
if(UART_INStr[1]==MODBUS_WRITE_ONE) // write one register
{
if(UART_INStr[4]==MODBUS_DATA_LENGTH) // correct data length
{
B_OK=TRUE;
}
}
}
else if(UART_INStr[0]==I_Adresse_Box) // It's me !
{
if(UART_INStr[1]==MODBUS_READ_ANY) // read many registers
{
Spy_MA();
if(UART_INStr[4]==MODBUS_DATA_LENGTH) // correct data length
{
B_OK=TRUE;
}
}
}
if(B_OK)
{
for(I=0; i<9;OUTStr[I]=UART_INStr[I]); //copy to treated string
for(I=0;i<UART_MESSAGE_LENGTH;UART_INStr[I]=UART_INStr[UART_MESSAGE_LENGTH+I]); //left shift UART_MESSAGE_LENGTH=9 char
UART_Cursor=UART_Cursor-UART_MESSAGE_LENGTH;
}
else
{
for(I=0;i<UART_Cursor;UART_INStr[I]=UART_INStr[++I]); //left shift 1 char
UART_Cursor--;
}
B_UART_OK=(UART_Cursor>8);
return B_OK;
} |
But, still something goes wrong I haven't yet understood |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1349
|
|
Posted: Tue May 20, 2014 8:06 am |
|
|
Code: |
for(I=0; i<9;OUTStr[I]=UART_INStr[I]);
for(I=0;i<UART_MESSAGE_LENGTH;UART_INStr[I]=UART_INStr[UART_MESSAGE_LENGTH+I]); //left shift UART_MESSAGE_LENGTH=9 char
|
where does "i" increment in these for loops? Looks like this would loop forever. Also, you shouldn't mix case I and i can get dangerous to manage and it isn't very readable in that form anyways. |
|
|
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
|
Posted: Tue May 20, 2014 10:52 am |
|
|
Quote: | where does "i" increment in these for loops? |
True
Stupid of me !!
read
Code: | for(i=0; i<9;OUTStr[i]=UART_INStr[i++]); //copy to treated string
for(i=0;i<UART_MESSAGE_LENGTH;UART_INStr[i]=UART_INStr[UART_MESSAGE_LENGTH+i++]); //left shift UART_MESSAGE_LENGTH=9 char |
|
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19513
|
|
Posted: Tue May 20, 2014 2:39 pm |
|
|
If this is Modbus, then the time gap is defined.
Frame start, is 3.5 characters or more of silence. Well defined, and easy to test for. |
|
|
Jean FOUGERON
Joined: 30 Nov 2012 Posts: 110 Location: France
|
|
Posted: Wed May 21, 2014 7:10 am |
|
|
I have implemented the following, and it seems it works (at least since this morning)
I have one set in the lab
If I configure it in adress 01 it answers, and it acts when broadcast (adress 0)
Idem if I configure it in adress 2
... and 3, and 4 ...
Youpiii !
Code: | #INT_RDA
void Receive()
{
char c;
c=getc();
if (I_UART_Cursor<=UART_MAX) INStr[I_UART_Cursor++]=c; // avoid overflow
B_UART_OK=(I_UART_Cursor>=UART_MESSAGE_LENGTH); // received at least one length of message
} |
B_UART_OK is ON when we received 9 bytes (which is the message length)
then in main routine
Code: | void main()
{
initialisation();
while(TRUE)
{
//.../...
if(B_UART_OK)
{
isMessageOK();
}
//.../...
}
} |
Then the routine which verify whether thes 9 chars are a real message or not
Code: | Boolean IsMessageOK()
{
int i;
Boolean B_OK=FALSE;
// UART_MESSAGE_LENGTH char at least are in INStr
if(INStr[4]==MODBUS_DATA_LENGTH) // correct data length
{
if((INStr[0]==BROADCAST_ADDRESS) && (INStr[1]==MODBUS_WRITE_ONE)) // Broadcast procedure and write one register
{
B_OK=TRUE;
}
else if((INStr[0]==I_Adresse_Box) && (INStr[1]==MODBUS_READ_ANY)) // It's me ! and read many registers
{
B_OK=TRUE;
}
}
if(B_OK)
{
for(i=0;i<UART_MESSAGE_LENGTH;OUTStr[i]=INStr[i++]); //copy to treated string
for(i=0;i<UART_MAX-UART_MESSAGE_LENGTH;INStr[i]=INStr[UART_MESSAGE_LENGTH+i++]); //left shift UART_MESSAGE_LENGTH=9 char
I_UART_Cursor=I_UART_Cursor-UART_MESSAGE_LENGTH;
GetData(); // Proceed
}
else
{
for(I=0;i<I_UART_Cursor;INStr[I]=INStr[++I]); //left shift 1 char
I_UART_Cursor--;
}
B_UART_OK=(I_UART_Cursor>=UART_MESSAGE_LENGTH); //Stops parsing, if needed
return B_OK;
} |
I test whether char 4 is 0x04, which has to be.
if yes, I tes whether char 0 is my adress or broadcast and if yes if the command (char 1) is the corresponding command
if yes, I copy the UART_MESSAGE_LENGTH char in a string OUTStr I then will analyse to proceed
and I shift UART_MESSAGE_LENGTH char the reception array INStr
If no, I left shift one char to do it again
I also update UART_Cursor in order to store the next received char at the correct position.
I update B_UART_OK. If everything is normal, UART_Cursor =0 so B_UART_OK=FALSE
Normally the next message will arrive in approx 500ms
Do you think there still is some risk in this code ? |
|
|
ezflyr
Joined: 25 Oct 2010 Posts: 1019 Location: Tewksbury, MA
|
|
Posted: Wed May 21, 2014 7:20 am |
|
|
Hi,
Well, is the '0x04' totally unique, or might it also appear as part of the 'data' or 'CRC' later on? If this character is not 100% unique, then yes, your method is still flawed....
As Ttelmah pointed out, the interval between messages in Modbus is well defined, and easy to test for. It seems to me that this is the only unique thing that occurs in your communications protocol. Why not use it?
John |
|
|
|
|
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
|