! Problem with UART and TIMER1 interrupt

[mikimtb]

Hi guys,
in title of this topic you can see what is my problem.

I'm using PIC18F4431 and write code for motion controller. CCS C v4.120
My code is:

[PCM programmer]

Get rid of all the lines where you enable/disable interrupts inside the
interrupt routines. That's causing your problem. Never do this in CCS.

Also add the ERRORS parameter to your #use rs232() statement.

[Mike Walne]

You're doing far too much in timer1 ISR.

(1) Keep isr's as short as possible, get in, set flags, get out.
(2) Do most of work in main().
(3) There's no need to enable / disable interrupts in ISR, CCS compiler does all the work for you.
(4) You also should not need to control tris, for same reason.
(5) You are enabling RDA interrupt in timer1 ISR.
(6) Avoid maths in ISR, floats take ages.

Reduce code to a few lines which show problem, rather than having to wade through pages of it.

Mike

[Ttelmah]

To re-iterate, ignoring the much too long routine, and the general enable/disables, which are unnecessary, the one that is the killer, is the line:

enable_interrupts(GLOBAL);

in the ISR. This must _never_ _ever_ be done in a PIC. Basically, the standard PIC cannot have interrupts at the same operating 'level' occurring inside themselves. In order to _ensure_ this does not happen, the _hardware_ disables the global interrupt bit, when the interrupt handler is called. There is then a special 'return from interrupt' instruction, which re-enables this bit _after_ the return is executed. Enabling it before the return, _will_ kill the code completely.

It is just worth emphasising, just how 'bad' this is. It really is 'shooting yourself in the foot', to do this.

Best Wishes

[mikimtb]

Thanks for support.
I change everything you said, and now it's going better. But again after I don't know, sometimes 20, sometimes 30, sometimes 4 packages UART receive stop to operate.

I knew that ISR has to be as short as possible. But as you can see on source code, I have a couple of sensors connected to the uC.

One of tasks is to have to keep a constant motor current from point A to point B. Because of that I have implemented PID controller. As you know PID controller is working in discrete time domain and in my case it's 1.5ms sample time. In that time I have to read sensors output and calculate PID which is motor pwm. Because of that I have a lot of math in ISR routine. Is there some other way to synchronize the PID and sensors outside ISR routine?

Thanks one more time.
Best regards.

[Ttelmah]

First, remember interrupts only respond to signaled events. You can always do the same things in the main code, by testing interrupt bits. On my own PID's, I commonly trigger the maths at the instant the cycle finishes. in the main.
However realistically the first key thing is to get rid of FP. This is _not_ needed for a PID. Microchip have a number of examples, doing all the PID maths, using scaled integers. Typically treating the integer values as 1/256th step values. Your position code is a killer.
Then look at taking your ADC readings _while_ other things are being done.
There are also problems _with_ your ADC readings. You are selecting a channel, then reading it immediately. This will give incorrect readings. The internal capacitor _must_ be connected to the incoming voltage for Tacq, _before_ you start a reading.
As another comment, ADC_CLOCK_INTERNAL, is usually _slower_ than using the right divisor off the master clock, and is not legal for clock rates above 1MHz.....

This is where the commands:
read_adc(ADC_START_ONLY);
read_adc(ADC_READ_ONLY);
adc_done();

come in.

You can (for example):

1) select you next required ADC channel.
2) Perform other operations that take at least Tacq.
3) Perform 'read_adc(ADC_START_ONLY)'.
4) _Immediately select the next channel_. This will be connected to the ADC as soon as the conversion finishes, reducing the need to wait next time.
5) Do other things for the time the ADC takes to perform the reading.
6) If you are sure you have taken longer than than the acquisition time, then perform:
val=read_adc(ADC_READ_ONLY);

If you are not sure, then check adc_done() first.

etc..

The point is that the ADC takes 13 cycles of it's master clock. Typically 2 to 4uSec/cycle for the internal oscillator (selecting master clock/32, gives you instead just 1.6uSec/cycle). Say 3uSec for your current clock, so the conversion takes 39uSec. For the whole of this time, you can be doing other things, which then don't have to be done later. Parallel tasking!....

You still show the enable/disable interrupt code in the interrupt routines.