 |
 |
| View previous topic :: View next topic |
| Author |
Message |
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
 Posted: Tue Jan 08, 2013 4:09 am |
 |
|
The sizes are as follow :
| Code: | #define UART_MAX 9 //de 0 à 8 : 7 utiles + 2 pour CRC16
char INStr[UART_MAX]; |
ckielstra, the INStr[0] will always be the slave adress (ie I_NumperoEsclave)
so INStr[1 to UART_MAX] will be other values, normally the correct query followed by the CRC16.
These byte can be stored only after having received a good slave adress. But I know that this slave adress can be from another message (for another slave or something like that) on the bus but in this case the CRC16 will be false and I skip the entire message.
Am I false ?
What I do not understand really is why there are "bad messages" as in the lab the master is alone on the bus and 0,5m far from the slave !! |
|
 |
RF_Developer
Joined: 07 Feb 2011
Posts: 839
|
|
| Posted: Tue Jan 08, 2013 4:25 am |
|
|
| Are you trying to reinvent the wheel? There is a working Modbus slave example implementation in ex_modbus_slace.c, which uses modbus.c. It should be easy to get this working on your PIC. Many people here have used modbus.c as part of successful projects. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19957
|
|
| Posted: Tue Jan 08, 2013 4:31 am |
|
|
So, you are using Modbus RTU, with four bytes of data?.
Best Wishes |
|
|
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
| Posted: Tue Jan 08, 2013 5:15 am |
|
|
| Quote: | | So, you are using Modbus RTU, with four bytes of data?. |
Byte 0= Adress of slave
Byte 1 = Function code (I only use 0x03, 0x06 and 0x07)
Bytes 2 and 3 = transaction nbr
Byte 4 = nbr of remaining bytes in the query (always 4 here)
Byte 5 and 6 are the parameters of the query (ie the number of registers to be read)
Byte 7 and 8 are CRC16
M | Quote: | | any people here have used modbus.c as part of successful projects |
Thanks, I will have a look on them but they seem really "heavy" ! |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19957
|
|
| Posted: Tue Jan 08, 2013 5:29 am |
|
|
Whatever the modbus code used, I'd still though think things would work a lot better if he could move his current INT_EXT source onto INT_EXT2.
Advantage then is that the only high priority interrupt can be the timer3 one he needs. The INT_EXT stuff can be made the lowest priority on the normal interrupt tree. INT_RDA can be made the top 'priority' on this tree.
The fact that 'ERRORS' improves thing, implies two possibilities:
1) A combination of interrupts is taking longer than the time between bytes, so INT_RDA is not being serviced fast enough, or
2) The Modbus communication, is at times corrupted, so malformed data bytes are being received.
The former is much more likely.
If you think about it he says that INT_TIMER1, is 440uSec. In fact it is perhaps 20uSec longer than this in total. INT_Timer3, triggers every 250uSec, and takes about 25uSec in total. So if INT_TIMER3, triggers right at the start of timer1, it'll then trigger again inside timer1, and if you then add the two handler times for this, it'll then trigger a third time before timer1 exits!. At this point all other interrupts have been suspended for 460+75 = 535uSec. If INT_EXT has triggered in the meantime, it too will have been handled, taking another 270uSec, and there will have been yet another trigger of INT_TIMER3 as well. So the time has now climbed to 830uSec. If the serial is running at 19200bps, this is almost the time for two bytes. If you assume a byte was 90% assembled when this all started, the serial _has_ now overrun....
Key is though that if INT_EXT was not being used, and this was instead INT_EXT2, and a low priority interrupt, this wouldn't happen. The worst case then would be the triple trigger of timer3, inside timer1.
Timer1, is the 'killer' interrupt here, with the multiple loop, and use of an array inside the loop. Ugh. If the values don't absolutely 'have' to all be tested every loop, I'd use a state machine instead.
Best Wishes |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19957
|
|
| Posted: Tue Jan 08, 2013 5:48 am |
|
|
Looking at timer1, it is worth understanding that bulky code, may actually be faster!.
If (for instance), you wanted to test four numbers stored in an array, against a limit value, the 'instinctive' way to code this is:
| Code: |
int ctr;
int number_over=0;
for (ctr=0;ctr<4;ctr++) {
if (array[ctr]>LIMIT) number_over++;
}
//However:
if (array[0]>LIMIT) number_over++;
if (array[1]>LIMIT) number_over++;
if (array[2]>LIMIT) number_over++;
if (array[3]>LIMIT) number_over++;
|
Is actually massively faster!. But possibly a fraction bulkier.
The key here is that accessing a fixed element in an array is solved at _compile time_, and the address is hard coded. Takes only a couple of instructions to access the fixed element, versus a dozen or so to access the one via a variable.
Now he has three different things that may give this effect, the two loops themselves in the interrupt, and also the access to 'LED', which he doesn't post. It is possible that tidying these up might be able to reduce the interrupt time by a huge factor. If so the byte loss problem would vanish.....
Best Wishes |
|
|
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
| Posted: Tue Jan 08, 2013 7:45 am |
|
|
| Quote: | | Looking at timer1, it is worth understanding that bulky code, may actually be faster!. |
Certainly, but SFLS.length depends on where the set will be installed.
My set pilots airfield lighting on an airport, and no airport has the same quantity of lights, unfortunately !
In another hand, the most important routines are INT_EXT and INT_TIMER3. Modbus only exchange datas measured by these routines, with the supervisor. If the modbus send info with 3 seconds late, no problem. If INT_EXT miss an input signal edge, it will interpretate as a misfunctionning lamp on the field and alarm the maintenance team !!!
That is why I need High priority on these 2 INT's |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19957
|
|
| Posted: Tue Jan 08, 2013 8:26 am |
|
|
Do the processing you currently have in INT_TIMER1, in the main loop outside the interrupts. Set a flag in the interrupt to trigger it. This will immediately bring the handing time of this massively down, and allow INT_RDA to occur while the handling is taking place.
Your problem is the fundamental one of trying to do too much in the interrupts.
Just because the number of lights is variable, does not mean you have to use variables to access the array. Consider having a variable 'number_of_lights', and having 'one', 'two' etc., defined as an enum, then a state switch selecting just one fixed routine to handle a fixed number of lights. Much bulkier (the code will be repeated for all the possible light patterns), _but_ only one will ever be executed, using fixed values.
Best Wishes |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Jan 08, 2013 8:41 am |
|
|
Just wondering, what is the possible range for the number of lights? 1 to ...?
| Code: | for(I=2; I<SFLS.Length; I++) //problème si on commence I à 1
{
SFLS.defaut[I]=B_Pas_de_Signal;
LED(I++,B_Pas_de_Signal,ROUGE);
} |
This type of coding where you increase I 2 times is ugly. It makes the code more difficult to read, error prone in maintenance and less efficient to execute. Change to: | Code: | for(I=2; I<SFLS.Length; I+=2) //problème si on commence I à 1
{
SFLS.defaut[I]=B_Pas_de_Signal;
LED(I,B_Pas_de_Signal,ROUGE);
} |
|
|
|
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
| Posted: Tue Jan 08, 2013 9:32 am |
|
|
| Quote: | Posted: Tue Jan 08, 2013 2:41 pm Post subject:
Just wondering, what is the possible range for the number of lights? 1 to ...? |
By experience from 8 to 31, all the configurations are found, and sometimes there are more than one config (switchable) on the same runway.
That's the true life
| Code: | | for(I=2; I<SFLS.Length; I+=2) |
You are right, I change it, but thuis code is quasi never used, only when a missing wire on the control loop on the runway.
But, regarding INT_EXT and INT_Timer3, it is true that INT_Timer3 is skipped by INT_EXT, and the clock duration increased at each edge.
If I reduce the Timer3 period (eg from 250 µs to 100 µs) the probability of coincidence will decrease, but the impact of "call delays", explained by Ttelmah, will increase ! |
|
|
RF_Developer
Joined: 07 Feb 2011
Posts: 839
|
|
| Posted: Wed Jan 09, 2013 2:55 am |
|
|
Out of interest, as this is a airfield lighting application, what standards apply to the software and software development process?
RF Developer |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Wed Jan 09, 2013 5:33 am |
|
|
Having had some time to think it over, I'm not happy at all with UART handling. I didn't have a chance to have a look at the CCS supplied modbus example program but I recommend you do have a look.
1) Your message time-out seems very long. | Code: | | if (L_Cycle_UART>CYCLE_250) // au bout de 250ms |
Is it really 250ms?
The Modbus standard specifies that a Modbus RTU message must be transmitted continuously without inter-character hesitations.
For your baudrate of 38400, that means there is a problem when no character arrives each 0.26ms
Given your 9 bytes message length and all other interfering interrupts a time-out of 5ms seems large enough. This way you will recover 245ms quicker after a receive error. A huge gain!
2) Your current UART receiving routine is bad design. | Code: | c=getc();
if(I_UART_IN_POS==0 && c==I_NumeroEsclave) // on commence à prendre en compte si c'est bien la bonne adresse
{
INStr[I_UART_IN_POS++]=c;
B_UART_OK=FALSE;
}
else if(I_UART_IN_POS<UART_MAX)
{
INStr[I_UART_IN_POS++]=c;
B_UART_OK=(I_UART_IN_POS>=UART_MAX);
} |
When the message you receive has an address matching your device, but if that's not the case then you start reception anyway. The program works, but that's because you are lucky and not because of good design. In fact, the whole address test doesn't do anything because the received data in both execution paths is equal. You can simplify to: | Code: | c=getc();
if(I_UART_IN_POS<UART_MAX)
{
INStr[I_UART_IN_POS++]=c;
B_UART_OK=(I_UART_IN_POS>=UART_MAX);
} |
As I said, this works, but from your original version it suggests you do an address check which you don't. I like the KISS principle: keep it simple and remove the obsolete address check.
3) Reading the UART in the Timer3 interrupt is dangerous. Even after the improvement from TTelmah there is a minuscule race condition bug. Assume that the UART interrupt was activated but not yet up to the point for reading the character (takes about 40 assembly instructions). At this moment the Timer3 interrupt could jump in, as it is a high priority interrupt, and would read all data from the UART. When Timer3 interrupt is finished it continues with the INT_RDA which will block on the getc() until a new character arrives. Big problem. I know, a minute chance this scenario will happen but still...
The solution is simple: remove the getc() from the Timer3 interrupt and don't clear_interrupt(INT_RDA). On a UART time-out there will be no data in the UART so you don't have to read all data from it, only reset the UART message pointers and flags. In the tiny chance there is data in the UART you don't care as this is a new message coming in and it will be processed in the next UART interrupt.
4) As said by Ttelmah, your main problem is that you are trying to do too many things inside the interrupts. Measure the worst case timing of all your interrupts. Then on a piece of paper work out the different timing combinations if there are conflicting combinations where minimum response time is not met. As Ttelmah has shown there is at least one such situation.
I don't understand why you are not implementing Ttelmah's suggestion of making Timer1 interrupt shorter. Instead of doing all the alarm checks inside the interrupt you just set a flag that it is time to do the alarm checks. Then in the main routine you check this flag and start the heavy processing. |
|
|
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
| Posted: Thu Jan 10, 2013 4:02 am |
|
|
I agree that I can reduce this 20ms, but as the supervisor queries every second, it does'nt really matter in my mind. 
Regarding INT_RDA. I change to
| Code: | | else if(I_UART_In_Pos>0 && I_UART_In_Pos<UART_MAX) |
Are you OK ?
I will generate perturbations on the lan with my VB supervisor and verify that it has no effect.
I also deleted the UART read in Timer3 as I share your opinion in this case
I am thinking how I can reduce time in INT_EXT, (maybe by desabling INT_RDA), as Timer3 is perturbated by INT_EXT
Regarding Timer1, I have just done it and it seems working well.  |
|
|
Jean FOUGERON
Joined: 30 Nov 2012
Posts: 110
Location: France
|
|
| Posted: Thu Jan 10, 2013 4:12 am |
|
|
Between
| Code: | if(SFLS.defaut[i]>0 && ++SFLS.defaut[i]>=I_Delai_Lampe_Alarme_Max)//il y a un défaut
{
if(SFLS.defaut[i]==I_Delai_Lampe_Alarme_Max) SFLS.Changed=TRUE;
SFLS.defaut[i]=I_Delai_Lampe_Alarme_Max;// délai d'affichage
B_Alarme=TRUE;
} |
and
| Code: | int Sdefaut_i=&SFLS.defaut[i];
if(*Sdefaut_i>0 && ++*Sdefaut_i>=I_Delai_Lampe_Alarme_Max) //il y a un défaut
{
if(*Sdefaut_i==I_Delai_Lampe_Alarme_Max) SFLS.Changed=TRUE;
*Sdefaut_i=I_Delai_Lampe_Alarme_Max;// délai d'affichage
B_Alarme=TRUE;
} |
What do you prefer ?
They both are 84 bytes long but the 2nd one is 9 cycles more. it's a pity as I founded it more elegant. |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Thu Jan 10, 2013 6:38 am |
|
|
| Code: | | int Sdefaut_i=&SFLS.defaut[i]; |
This is wrong! You shouldn't assign an address to an integer. Correct would be: | Code: | | int* Sdefaut_i=&SFLS.defaut[i]; |
But the PIC is inefficient at address calculations so it is best to avoid pointers and notations like defaut[i]. A popular optimization is to put the pointer contents in a temporary variable and use that everywhere.
Another bad programming practice is to do calculations inside an if-statement (your ++ statement). It makes your program more difficult to read and so the chance of creating bugs increases. Writing compact code does not automatically lead to a more compact programs. Best is to write an easy to read program and then let the compiler do the optimization.
I would try something like: | Code: | int I_defaut_i = SFLS.defaut[i];
if (I_defaut_i > 0)
{
I_defaut_i++;
if (I_defaut_i < I_DELAI_LAMPE_ALARME_MAX)
{
SFLS.defaut[i] = I_defaut_i; // délai d'affichage
}
else //il y a un défaut
{
// If first time we get here
if (I_defaut_i == I_Delai_Lampe_Alarme_Max)
{
SFLS.Changed = TRUE;
SFLS.defaut[i] = I_Delai_Lampe_Alarme_Max;
B_Alarme = TRUE;
}
}
} |
When I look at your code I see a lot of 'clever' constructions, like the if-statement with the '++' in the second term that is only being executed when the first part is TRUE. It is correct code, but I also get the feeling you are showing off. Don't do that. The program won't run any faster. Your job is to write a program that is easy to maintain, also by other people. Now your code has all kind of clever side effects that are difficult to follow and will lead to bugs. And, in the rare case, that you do have to create 'clever' constructs then at least write a comment in your code so people know it is intentional. |
|
|
|
|
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
