Re: [avr-gcc-list] generic queue library for AVR GCC?

[David Brown]

> Ok, I take back everything I said...  Its not any worse then
> INTERRUPT(), or SIGNAL(), and with a little training even the newbies
> should be able to understand that critical sections should be treated
> with care. :)
>
> Mike
>

I think it might be worth adding a couple of comments here, especially as I
have written queing code for the msp using "critical" functions.

#define bufferSize 32                    // Power-of-two makes % easy
static uint8 buffer[bufferSize];
static uint8 start, length;                // Note - no need for "volatile"
or anything

void critical addToBuffer(uint8 b) {
    if (length >= bufferSize) return;    // Safe to write this sort of thing
    buffer[(start + length) % bufferSize] = b;
    length++;
}

uint critical getFromBuffer(void) {
    uint b;
    if (!length) return 0;
    length--;
    b = buffer[start];
    start = (start + 1) % bufferSize;
    return b;
}

More below...


> On Wed, 2004-11-17 at 15:07, Mike Panetta wrote:
> > On Wed, 2004-11-17 at 12:30, E. Weddington wrote:
> > > David Brown wrote:
> > >
> > > >Personally, I'd like to see the addition of a "critical" function
attribute,
> > > >like in mspgcc.  It is clearly readable, avoids any requirements for
extra
> > > >local variables, generates optimal code, and generates any required
"goto
> > > >out" automatically.  You can use it on inlined functions to avoid
function
> > > >call overheads if you want.
> > > >
> > > >
> > > I think that's a *great* idea!
> > >
> > > Personally, I don't care for all the methods that have been proposed
on
> > > this thread that "hide" the manipulation of SREG (various macros,
etc.),
> > > because of the very reason that they can be too easily be the causes
of
> > > some bad behaviour unless someone is vigilant about using them. I
> > > generally prefer to be explicit about it.
> > >
> > > *But*, if there is a reasonable way to have the compiler enforce good
> > > behaviour, such as the "critical" function attribute proposed above,
> > > then I think that beats out all other methods.
> >
> > ACK!  NOO!!  Its bad bad bad.  It hides the fact that the function is
> > critical so well that it makes it many times easier to make a simple
> > mistake that would throw interrupt latency (or worse) all to hell, and
> > the programmer may not even know why!
> >

The "critical" attribute doesn't hide anything more or less than a decent
set of macros or inline functions.  It makes it harder to get it wrong,
which is a good thing.  I suppose there is a danger of newbies mistakanly
using "critical" when they really mean "I want this function to run as fast
as possible".

> > Ill give you 2 ways that this can be innocently broken.  way number 1,
> > allocate extra variables on the stack (or malloc) in the 'critical

Variables only get allocated on the stack if there are no convenient
regsiters available (or if they are big locals), and even then it is a very
cheap operation.  "malloc" has little place in an avr program in the first
place - any code using dynamic memory allocation should be checked
thoroughly regardless of whether interrupts are disabled or not.

> > function'.  An end user (programmer) that may not be the one that
> > origionally designed the function may add code that allocates space on
> > the stack or mallocs space inside said critical function.  This will
> > cause interrupts to be disabled far too long.  Example code:
> >
> > functionheader.h :
> > void criticalfunction(void) __attribute__ ((critical));
> >
> > functionbody.c :
> > void criticalfunction(void) //notice you do not see the attribute here,
> > a comment could get around this however

Actually, you can put the attribute in the C code - it is not needed in the
header code (calling functions don't care if the function is critical or
not).

> > {
> > int a, b, c; //I assume the compiler would allocate this space BEFORE

Why do you assume that?  What code do you think "int a, b, c" creates?
Absolutely none - these will use registers.

> > disabling interrupts
> > a = somefunc();
> > b = someotherfunc();
> > if (b)
> > {
> > int d; //This would be allocated with interrupts disabled!!!

Again, declaring small local variables is (close to) zero cost.

> > Interrupt latency increases.
> > d = somefunc();
> > c = d + b;
> > }
> > }
> >

Obviously you want to minimise function calls (unless they are inlined) in a
critical function - in fact, you want to do as little as possible during
them.

> > Another example, in this example someone else has picked up the code (or
> > it may even be the same person that wrote it) and is trying to debug a
> > problem:
> >
> > functionbody.c :
> > void criticalfunction(void)
> > {
> > int a, b, c;
> >
> > a = somefunc();
> > b = somefunc2();
> > c = a - b;
> > if (a < 0) //this is an error case that should not happen and is being
> > debugged
> > {
> > printf("Why is a %d?\n", a);
> > }
> >
> > }
> >
> > Now one of 2 things could happen, either the interrupt latency is shot
> > to hell and the code stops working, or the code locks up because some
> > function that printf() is calling requires interrupts to be enabled.
> > Either way we are screwed.  If functions like enter_critical(), and
> > exit_critical() were used, it would be obvious where the printf should
> > go, and the code could even be manipulated to make it safe to put it
> > where it would be needed if need be.  Having 'critical functions'
> > defeats this.
> >
> > I admit that both of the examples above are very contrived, but it
> > should be obvious why this is bad when one actually gets to writing a
> > critical piece of code.  It makes debugging difficult for sure, and that
> > is not a good thing at all.

Debugging critical sections is *always* difficult.

“Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it.” – Brian W. Kernighan

mvh.,

David



> >
> > My $3.50 ;)
> >
> > Mike
> >
> > >
> > > My $0.02.
> > >
> > > Eric
> > >