Re: [avr-libc-dev] Re: New Atomic.h header?

[Joerg Wunsch]

As Dean wrote:

> Joerg: If eventually accepted I will of course ask the original
> author for permission, and add him to the copyright. I hope I don't
> get too annoying here bouncing my ideas off you all; I tend to get
> over-enthusiastic about the simplest of things.

No, it's completely fine to discuss this here, that's exactly the
purpose of this list.  I'm really glad I'm not the only one discussing
it with you.

I appreciate that there appears to be general agreement to use a
different header file name for this (<avr/atomic.h>) rather than
putting it into <avr/interrupt.h>

> First of all, my macros require compilation in GNU99 standards
> mode. I can't see why people don't do this all the time anyway, but
> since the rest of AVRLibC is ANSI compliant (IIRC) this may cause
> some concern with users.

I tried to get around this using __extension__, but that doesn't
appear to work, alas.  So unless you'd like to completely modify the
idea behind these, I don't see any other option than requiring the
users of these macros to use either std99 or gnu99 as their -std.
That's probably not too hard.

If you were to go back to your original idea in the avrfreaks thread,
and split it into two parts, you could move the save variable out into
a surrounding block (the ATOMIC_END macro would then close the block
and thus terminate the scope of that variable).

> Second is the serious one. GCC4.1 may re-order the block and move
> out access to variables. In Paulo's test case:

> Which is no good. Unless anyone can think of a way to force the
> compiler to not re-order the code, then this macro won't be suitable
> for general use.

The force is to apply a "memory barrier":

static inline void    __iRestore(const uint8_t *s)   \
{ SREG = *s; asm volatile("" ::: "memory"); }

> As for the requirements of the NON_ATOMIC block, ...

> The FORCE modes are useful where the current interrupt state is a
> known constant, for example inside a nested interrupt. ...

OK, I'm fine with that.  It needs some demonstration in the
introductionary doxygen blurb then however, in order to illustrate all
the different options to the users.

Minor stylistic things to consider when you are about to redo the
final submission:

. C preprocessor directives have their # in the first column,
  according to the C standard.  Space is allowed after the #, so
  you can indent like:

#if foo
#  define bar foo
#else
#  define bar 0
#endif

  In your case, no indentation is needed at all though.  All the
  remaining include files don't indent their various #defines, even
  though the all have the idempotency #ifdef bracket around.

. All non-standard keywords need to be protected: so use __asm__
  rather than asm etc.  Even inline should be protected that way, as
  we'd otherwise require the entire header file to be compiled with at
  least c99 (or gnu89) mode.  By using __inline__, the declarations
  would be OK even in c89 mode, only the macros could not be used
  there (but perhaps the file will also collect other macros later on
  that would be applicable even in c89 mode).

. All identifiers have to be chosen from the implementation namespace.
  That starts with the ATOMIC_H in the idempotency statement (should
  be _AVR_ATOMIC_H_ then), but it's got more subtle.  Consider what
  happens to the above __iRestore function above if a user has

#define s short int

  somewhere in scope...  So it must be __iRestore(const uint8_t *__s).

. Please break lines around column 80, that's the conventional size.
  Long macros can be wrapped across severa lines using
  backslash-newline.

-- 
cheers, J"org               .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/                        NIC: JW11-RIPE
Never trust an operating system you don't have sources for. ;-)