avr-gcc-list
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [avr-gcc-list] Problem with interrupt on a MEGA8535


From: David Brown
Subject: Re: [avr-gcc-list] Problem with interrupt on a MEGA8535
Date: Wed, 20 Feb 2008 09:19:45 +0100
User-agent: Thunderbird 2.0.0.9 (Windows/20071031)

dlc wrote:
I have an odd problem and I'm hoping that by trying to explain it I can solve it...


This should perhaps be on avr-chat rather than avr-gcc-list, since it is not really a gcc issue.

Paulo (and others) have given you ideas that might help your problem - I can give you a few pointers towards writing better code, especially for the AVR. A few small changes could make your ISR many times smaller and faster.

What I have is an timer overflow ISR that tics off every 10us. I've coded it thusly:

// variables at the top (global)
//ISR variables, volatile by nature
volatile uint16_t t_10us=0;
volatile uint32_t t_1ms=0;
volatile uint16_t repeat=0;
volatile uint16_t  match=0;        // The single servo used
uint8_t    servo = 150;


First off, ISR variables should *not* be volatile. "Volatile" tells the compiler that the values might change (or be read) outside the code flow it is handling at the time. So anything used only be the ISR should not be volatile - "volatile" costs time and space.

On the other hand, variables that are used in both the main loop, and ISRs, should normally be volatile. Thus "servo" should be volatile, as should "t_1ms" (since it is read in WaitMS), but all the others should not be volatile.

In fact, the missing "volatile" on servo could be part of the cause of your problems - there is no reason for the compiler to update the variable's memory just because you wrote "servo = i" - it could well leave the update until after the for() loop in your main code.

Secondly, the AVR is an eight bit processor. Don't use 16-bit or 32-bit data unless you really need it (or you're writing portable code, or something). Using a 16-bit variable for a counter that maxes out at 100 is much less efficient. Just try changing your "volatile uint16_t t_10us" to "static uint8_t t_10us" and look at the difference in the generated assembly for the ISR. Go through your other variables, and make sure they are of minimal size, are only volatile if they have to be, and that they are "static" if they are not exported by this module.

One last thing - I don't know if it was just a matter of format mangling by email clients, but the following line pair is hideously dangerous. You are hereby sentenced to three days Python programming.

>     if (match == servo)
>     SPIN=0;                //drop servo bit

The only sane way to write if() statements is to include the brackets on every occasion, even if you have only one statement in the conditional. It's sometimes appropriate to omit the brackets if your code is all on a single line, and the it's a simple assignment (no risk of complications with macros), if that makes your code clearer. But if you are using two lines, use brackets every time.

mvh.,

David



// past initializations and such ...

SIGNAL(SIG_OUTPUT_COMPARE0)
/*
 * 10 microsecond ISR
 */
{
    t_10us++;
    if (t_10us > 100)
    {
        t_1ms++;            //1ms background clock
        t_10us = 0;
        repeat++;
        if (repeat == 19)
        {
            repeat = 0;
            SPIN = 1;        //raise servo pin high
            match = 0;        //start servo timer
        }
    }
    match++;                //increment every 1
    if (match == servo)
    SPIN=0;                //drop servo bit
}

Now for the weird part. This all above seems to work fine unless I update "servo" too often. As seen below:

// More code goes by...

            servo = 100;
            for (i=100;i<215;i+=2)
            {
                servo = i;

// If these lines are used it seems to work, but with skipped beats
//        printf("servo = %d\n\r",i):
//        WaitMS(40);

// With this next line I just get the extremes, no intermediate steps
                WaitMS(150);
                if(PIR)
                {
                    RIGHT_STATUS = 1;
                    break;
                }
            }

What happens is that I only get the servo to move to the extreme locations, none of the intermediate ones when I am not using the printf() routine. This suggests a timing issue, but at 9600 baud those printf()'s aren't taking more than 10-12ms I'd guess, and its hardware so they shouldn't block either. I'm baffled.

 void WaitMS(uint16_t mswait)
 /*
  * WaitMS() will delay for the considered number of ms.
  */
 {
    uint32_t started = t_1ms + mswait;
while (t_1ms < started)
          ;
    return;
 }

While I'm typing this it comes to me that perhaps the assembly in the ISR is not so compact as I would like and that it simply does not have time to do what needs done - But 150ms? I'm not so sure. Can anyone offer some suggestions why this code will not function properly? Any ideas accepted, including "Dennis you ignorant <expletive deleted>...

  In the mean time, I'm going to examine the assembler output...

many thanks,
DLC






reply via email to

[Prev in Thread] Current Thread [Next in Thread]