save/restore interrupt status

Last post
34 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am porting a multi-tasking kernel (in C++, WinAVR compiler) to the ATmega2560, and I need to save and then later restore the interrupt status in SREG.

I'm new to both GCC and the AVR instruction set, and I don't see any equivalent instructions to push/pop the SREG to the stack. So it looks like I need to do this manually in this architecture. Or perhaps there is a better way to do this than to use the stack, maybe a shadow register or something?

Would a GCC-guru be so kind as to share their C-macros or assembly sequences to save and restore interrupts please?

-Ray

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In C, this is simply:

...
  uint8_t sreg_save = SREG;
  cli();
  ...
  SREG = sreg_save;

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Excellent!

I see now that SREG is configured as a port in the AVR architecture, and GCC supports port access in C. I'll make the macros to handle this soon.

Thanks!
- Ray

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I made these from a post I found a while back on AVRFreaks - I forget who posted it in the first place. I modified it to allow for both ASSUME_ON (implicit sei() at block exit) and RESTORE_STATE (restore to previous state on block exit):

	// Other General Macros:
	#define ATOMIC_BLOCK(exitmode)   { exitmode cli();
	#define END_ATOMIC_BLOCK         }
	
	#define ATOMIC_RESTORESTATE      inline void __irestore(const uint8_t *s) { SREG = *s; }         \
	                                 const uint8_t __isave __attribute__((__cleanup__(__irestore))) = SREG;
	#define ATOMIC_ASSUMEON          inline void __irestore(const uint8_t *s) { sei(); (void)s; }     \
	                                 const uint8_t __isave __attribute__((__cleanup__(__irestore))) = 0;

Use like the following:

	ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
	{
	    // Code here
	}	
	END_ATOMIC_BLOCK

Neat thing about this macro is that the entire containing block is atomic - and the interrupt flag is restored/set as soon as the block exits, whether than be from a return, break, etc.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Neat. Now please explain *why* this is really working, Dean. :-))

That kind of code belongs into the library though, as it is highly
compiler-dependent. The two leading underscores are a strong
indication that someone implemented them for that purpose.

If they are submitted as a patch to avr-libc, I'm willing to include
them. Alas, this would require proper attribution about the author.
Also, keep in mind that they need to be documented for this. This
includes doxygen comments for each individual macro, as well as some
example blurb for the top of the documentation to
explain how to actually use them.

Your inner { } are not needed: they are implied by these macros.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks Dean,
One issue I want to remind users of your ATOMIC_BLOCK is that interrupts are off within the block. Of course, that is the whole point. In the implementation that I have created, I have to explicitely disable interrupts separately so that the reader easily recognizes what is happening, rather than being hidden until the reader looks at the macro definition.

In the multi-tasking kernel I'm using this with, I have a separation between preventing task switches EnterCriticalSection() and ExitCriticalSection(), and shutting off interrupts entirely. In the critical sections, interrupts still occur, but task switches are held off (using the task event queues) until the end of the section.

Thanks for all your help.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sounds a lot like FreeRTOS. I've included my ATmega2560-specific code here. In particular, pay attention to portSAVE_CONTEXT, portRESTORE_CONTEXT, and the parts of pxPortInitializeStack controlled by the compiler switch NEED_LONG_ADDRESSES.

I keep meaning to send this to the FreeRTOS folks, but haven't gotten around to it.

Hope this helps!

Stu

Attachment(s): 

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl:

Yes, I actually read the GCC documentation after seeing the original version of those macros (I extended it to include the ASSUMEON and RESTORESTATE variants).

The code works by creating a new local variable, initialized to the current SREG status. The macro also creates a new inline function which restores SREG to a value passed to it.

Via the use of the GCC cleanup attribute, the inline function is called when the new local variable exits scope. Since the macro also creates a new block (via the braces) containing the local variable and the desired atomic code, this has the effect of calling the cleanup routine to restore SREG when the block leaves the scope.

In the case of the ASSUMEON mode, the local variable is initialized to 0, and the cleanup routine ignores the passed value and just executes a sei(). The optimizer cleans it all up to make it as lean as possible.

I'm aware that the extra braces aren't needed. In fact, the original macro required the following construct:

{
ATOMIC_BLOCK;

// Code here
}

Which I thought was too confusing. By making a new block in the starter macro I was able to change the construct to the above (making it similar to the other "for", "while" etc. blocks in the C language) with the only negative being the requirement for the ending macro, END_ATOMIC_BLOCK. It's just a matter of personal preference.

Stu:

Neat macros, however that will not take into account early exiting of an atomic block. Example:

while (SomeFlag)
{
   portENTER_CRITICAL();
   SomeAtomicCriticalAction();
   if (SomeOtherFlag)
     break;
   portEXIT_CRITICAL();
}

If "SomeOtherFlag" is set during the check, the while will terminate immediately without restoring the interrupt flag value.

EDIT: Credit where credit is due. I found the original; the macro was originally written by Darcy Watkins, in this thread.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, could you agree with Darcy on a submission as an avr-libc patch?
That would be great.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you don't like the terminating macro requirement, then an alternate formulation which doesn't require it would be something like:

#define CRITICAL_BLOCK(x) do \
{ \
   unsigned char _tmp_sreg = SREG; \
   cli(); \
   { \
      x \
   } \
   SREG = _tmp_sreg; \
}while(0);

An instance would look like this:

CRITICAL_BLOCK(
   //insert code here
)

It would fall victim to the same weakness as Darcy/Dean's implementation, in that you cannot exit from the block prematurely:

while (SomeFlag) 
{ 
   CRITICAL_BLOCK( 
      SomeAtomicCriticalAction(); 
      if (SomeOtherFlag) 
        break; // This line will break things!  (no pun intended)
   )
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

It would fall victim to the same weakness as Darcy/Dean's implementation, in that you cannot exit from the block prematurely:

The macro I posted above works just fine if you exit prematurely - that's the reason I use it despite the requirement for the ugly termination macro.

If a warning was added to indicate the inability to prematurely exit from the above macro, it could be implemented into AVRLibC. But how many people read the docs, anyway? I think all the "Help! _delay_ms(1000); doesn't work!" threads proves that ;).

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just re-read the thread. Neato!

It's really too bad that the low-level details for the more complete, fool-proof implementation you've presented is just so hard on the eyes!

I think I've wrapped my head around the way it works, but I don't imagine somebody new to the toolchian would be able to understand it... On the other hand, if it were placed in the standard library, then they wouldn't need to understand why it works -- just accept that it does, and use it as presented.

That's the reason there are libraries in the first place, isn't it... To protect the user from confusing details that hurt to think about.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dean, Jörg

If you are going to add this to the avr-libc, could you please consider making it work both ways - atomic cli-block yes, but also a sei-block?

void 
some_function(void)
{
  // Interrupts in original state
 
  CLI_BLOCK
  {
     // Ints disabled

     SEI_BLOCK
     {
        // Enabled
     }

     // Disabled again
  }

  // Original state
}

Here's what I came up with after reading about doran's and Darcy's ideas.

static inline uint8_t global_int_disable( void ){ cli(); return 1; }
static inline uint8_t global_int_enable( void ){ sei(); return 1; }
static inline void sreg_restore(uint8_t *sreg_save) { SREG = *sreg_save; }

#define CLI_BLOCK \
for ( uint8_t sreg_save __attribute__((__cleanup__(sreg_restore))) = SREG, \
      ints_off = global_int_disable() ; ints_off ; ints_off = 0 )

#define SEI_BLOCK \
for ( uint8_t sreg_save __attribute__((__cleanup__(sreg_restore))) = SREG, \
      ints_on = global_int_enable() ; ints_on ; ints_on = 0 )

I like the for() construct rather than curly brackets hidden inside two macros, or having the macro as the first statement inside a block. Is there some flaw using a for() - perhaps causing it to fail under some circumstances?

What comes to the ASSUMEON (or ASSUMEOFF), I'd rather use standard cli() or sei() and forget the block constructs altogether in those cases. Dropping this "parameter" also looses the parentheses, making it less prone to be confused with a function call.

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't quite see your point. If interrupts had been disabled
initially, they are sure for a reason. Trying to break with that
(i. e. enforcing the sei()) looks like a design failure to me.

If you are sure you can enable interrupts at the point where you
currently are, what's the point of possibly disabling them again
afterwards (by returning to the previous state)?

I. e. I don't see a legitimate reason why you cannot express your
statement rather by:

void
some_function(void)
{
  CLI_BLOCK
  {
    // ...
  }
  sei();
  // ...
  cli();
  // ...
}

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You're right. Same thing can be achieved like that. I just happen to like symmetry. :)

Anyway, here's an example how I've used a sei-block when walking a doubly-linked, circular list of timer structs:

struct message
{
  struct message * next;
  struct message * prev;

  void (* func)( struct message * );
};
typedef struct message message_t;

struct timer_message
{
  message_t  node;       // base message with callback function
  uint32_t   delay;      // time to wait in milliseconds
};
typedef struct timer_message timer_message_t;


// Timer queue anchor. Also used as a sentinel when walking the list.
static
timer_message_t timer_queue = { .node  = { .next = &timer_queue.node,
                                           .prev = &timer_queue.node,
                                           .func = NULL },
                                .delay = TIMER_MAX_VALUE
};



static
void
timer_queue_insert( timer_message_t * tm )   // Single message only!
{
   // Atomically check if new delay should precede the head.
  CLI_BLOCK
    {        
      // Queue walk pointer.
      timer_message_t * next = (timer_message_t *) timer_queue.node.next;
      
      if ( tm->delay > next->delay )
        {
          tm->delay -= next->delay;
          next = (timer_message_t *) next->node.next;

          SEI_BLOCK
            {
              // Continue searching an insertion place for the new delay.
              while ( tm->delay > next->delay )
                {
                  tm->delay -= next->delay;
                  next = (timer_message_t *) next->node.next;
                }
            }
        }

      // Insert new delay before next.
      queue_insert( &tm->node, &next->node );

      // Adjust the delay following new timer, except if it's the sentinel.
      if ( next != &timer_queue )
        next->delay -= tm->delay;
    }
}

Head must be examined as an atomic action, because tick interrupt may update it, but other timers in the queue are not touched and thus are not critical.

Once the list is walked, insertion must be atomic as it may be a new head we're setting up. Tick ISR finds the timer to touch by following anchor's next-pointer.

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Uh, oh, after rethinking this, I have to agree there is a design error. Thanks for pointing this out, Jörg. Enforcing sei was not the intention - restore was.

Now the code is more correct, but does an unnecessary retesting in the while() if if() has already failed.

static 
void 
timer_queue_insert( timer_message_t * tm )   // Single message only! 
{ 
  // Queue walk pointer. 
  timer_message_t * next; 

  // Atomically check if new delay should precede the head. 
  CLI_BLOCK 
    {        
      next = (timer_message_t *) timer_queue.node.next; 
      
      if ( tm->delay > next->delay ) 
        { 
          tm->delay -= next->delay; 
          next = (timer_message_t *) next->node.next; 
        }
    }

  // Continue searching an insertion place for the new delay. 
  while ( tm->delay > next->delay ) 
    { 
      tm->delay -= next->delay; 
      next = (timer_message_t *) next->node.next; 
    } 

  // Atomically update the queue
  CLI_BLOCK 
    {        
      // Insert new delay before next. 
      queue_insert( &tm->node, &next->node ); 

      // Adjust the delay following new timer, except if it's the sentinel. 
      if ( next != &timer_queue ) 
        next->delay -= tm->delay; 
    } 
} 

I have an itch there's some big problem with this retesting with ints enabled. Feels like a flag needs to be set inside the if() and tested before while().

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry to take us back a bit, but...

Dean:

while (SomeFlag)
{
   portENTER_CRITICAL();
   SomeAtomicCriticalAction();
   if (SomeOtherFlag)
     break;
   portEXIT_CRITICAL();
}

I understand why this would fail in the manner you suggested, but why wouldn't you rearrange this as (braces are my idea here)....

while (SomeFlag)
{
   portENTER_CRITICAL();
   {
      SomeAtomicCriticalAction();
   }
   portEXIT_CRITICAL();
   if (SomeOtherFlag)
     break;
}

to provide the non-atomic time brief interludes, or...

portENTER_CRITICAL();
{   
   while (SomeFlag)
   {
      SomeAtomicCriticalAction();
      if (SomeOtherFlag)
         break;
   }
}
portEXIT_CRITICAL();

to prevent access at all while SomeOtherFlag is not true? I'm not as happy with this last implementation, since atomic functions should generally be done in as little time as possible to avoid whacking the rest of the system.

Maybe I'm just not seeing this. I've certainly been guilty of that before, and shall be again. I'm bringing this up only because I suspect everyone else is seeing something I'm not.

By the way, I like the CLI_BLOCK macro.

Ripa: My concern with your timer_queue_insert function is that you are walking the timer queues with interrupts (and tasking?) enabled. Is it possible for an interrupt to change the timer queues behind your back? If so, then you should leave interrupts disabled while you walk the queues as well as when you modify them.

In a pre-emptive kernel, you would need to lock out task switching, but not necessarily interrupts. Ergo, there would be another set of macros for locking out task switching.

Stu

edit 1: minor typo fix

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Stu,

I think the point Dean is advocating is, if we can offer a solution which is immune to any possible problems due inexperience or carelessness, then we'll be saving our users a lot of heartache and debug time later on when things don't work the way they were expected to.

Seriously... If you are presented with a library-provided CRITICAL_BLOCK{...} macro, and don't take the time to look at the source code, would you have any reason to think that certain innocent-looking syntax features, like a "break" statement, which is otherwise totally innocent and harmless, could actually cause the application to crash simply by virtue of its location inside a code block?

If, right from the start, we make it impossible for that sort of mistake to be made, then everyone'll be happeir for it. Dean makes a good point to prove that people never read the documentation until the bug has already bit them, with his reference to the multitude of questions we field on this forum about the _delay_ms() function... Why pass up an opportunity to improve the signal-to-noise ratio in the support channels as we add a feature like this to the library?

- Luke

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Luke: Indeed, that's the point I'm trying to get across. Yes, my macro is about as pretty as Brittany Spears after an all-week bender, but it works and it's immune to user-error.

eerola: My macro can either enforce a sei(); at the end of the block by using ATOMIC_ASSUMEON as the parameter. If you wish for the previous global enable value to be restored at block breakage, then the ATOMIC_RESTORESTATE parameter should be used.

You can indeed change the block to a for, but I prefer a do/while:

	#define ATOMIC_BLOCK(exitmode)   do { exitmode cli();
	#define END_ATOMIC_BLOCK         } while (0);

This is probably a better option, as it will allow for the break statement to be used inside the block without an extra control structure. Cheers.

Stu: Your second approach yields code very similar to that of my macro. The first example is nice, but again users have a very nasty habit of not reading the important red text in the docs.

I suppose that could be fixed by creating a new "avr/atomic.h" header file, and having the following code in it:

#ifndef READ_ATOMIC_DOCS
  #error Please read the atomic section of the AVRLIBC documentation before using this header file.
#endif

;)

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Okey-doke, Luke and Dean. Thanks for the clue. I hadn't thought of it from the "newbie" point of view and you are both correct. Trying to help those who are just starting is admirable.

I started when 110 baud acoustic couplers and 16K magnetic core RAM were "high tech". :shock: Maybe I'm getting to old for this s**t. :?

Thanks, guys!

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Stu,
It's co-operative. Tick ISR may decrement the head, but when it goes to zero, it just puts an timer event handler to run queue. That handler will eventually move the expired timer to run-queue, but cannot steal any timers in the middle of list walking.

Dean,
What do you mean by "extra control structure?" A for() can be exited with break as well.

Your macro is very good. I just personally don't like the red tape at the end of the block, nor the exitmode. ASSUMEON can be as dangerous as my sei-block turned out to be.

Can we have a restore block that could be nested inside a cli-block? Something like this:

static inline uint8_t global_int_disable( void ){ cli(); return 1; } 
static inline uint8_t global_int_restore( uint8_t sreg_original ){ SREG = sreg_original; return 1; } 

static inline void sreg_restore(uint8_t *sreg_save) { SREG = *sreg_save; } 

#define CLI_BLOCK \ 
for ( uint8_t sreg_original __attribute__((__cleanup__(sreg_restore))) = SREG, \ 
      ints_off = global_int_disable() ; ints_off ; ints_off = 0 ) 

#define RESTORE_BLOCK \ 
for ( uint8_t sreg_save __attribute__((__cleanup__(sreg_restore))) = SREG, \ 
      ints_on = global_int_restore( sreg_original ) ; ints_on ; ints_on = 0 ) 

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

eerola:

Yes, a for is perfectly fine. My original macro uses just braces to create a scope block for the atomic section, rather than a do/while or for control structure. Trying to use a break in my original macro would produce a warning, as there is no control structure to break out of.

ASSUMEON isn't dangerous. I use it in my code in routines where I know that the global enable flag must be set for it to be called, thus I save words by not restoring the previous state. If you need to retain the previous value, just use RESTORESTATE.

I guess I just can't understand the problem you obviously see.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

eerola, I see what you are getting at with the for loops, very clever. Unfortunately, it all involves a tradeoff.

There's my original macro, which requires the extra macro to terminate the block.

Then there's the following, which does not:

	static inline void    __irestorevalue(const uint8_t *s) { SREG = *s; }
	static inline void    __irestorefixed(const uint8_t *s) { sei(); (void)s; }
	static inline uint8_t __ikill(void) { cli(); return 1; }
	
	#define ATOMIC_RESTORESTATE      for ( uint8_t sreg_save __attribute__((__cleanup__(__irestorevalue))) = SREG, __ToDo = __ikill(); __ToDo ; __ToDo = 0 )
	#define ATOMIC_ASSUMEON          for ( uint8_t sreg_save __attribute__((__cleanup__(__irestorefixed))) = 0, __ToDo = __ikill(); __ToDo ; __ToDo = 0 )

The tradeoff here is that the inline functions must be declared in the same file.

You can make the inline functions part of the macro:

	#define ATOMIC_RESTORESTATE      inline void __irestore(const uint8_t *s) { SREG = *s; }  __attribute__((weak))        \
 	                                 inline uint8_t __ikill(void) { cli(); return 1; }                \
	                                 for ( uint8_t sreg_save __attribute__((__cleanup__(__irestore))) = SREG, __ToDo = __ikill(); __ToDo ; __ToDo = 0 )

	#define ATOMIC_ASSUMEON          inline void __irestore(const uint8_t *s) { sei(); (void)s; }     \
	                                 cli();                                                           \
									 for ( uint8_t sreg_save __attribute__((__cleanup__(__irestore))) = 0, __ToDo = 1 ; __ToDo ; __ToDo = 0 )

But the tradeoff here is that using more than one atomic block in the same function will cause a multiple definition error.

A final solution would be to have the inline functions declared as part of a macro:

	#define USES_ATOMIC_BLOCKS     static inline void    __irestorevalue(const uint8_t *s) { SREG = *s; }      \
	                               static inline void    __irestorefixed(const uint8_t *s) { sei(); (void)s; } \
	                               static inline uint8_t __ikill(void) { cli(); return 1; }

And require the programmer to add in "USES_ATOMIC_BLOCKS;" to the top of each C file that requires them.

Which one is the best solution is up for debate, but there's no real answer.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ok, my final solution:

avr/atomic.h

	#define USES_ATOMIC_BLOCKS     static inline void    __irestorevalue(const uint8_t *s) { SREG = *s; }      \
	                               static inline void    __irestorefixed(const uint8_t *s) { sei(); (void)s; } \
	                               static inline uint8_t __ikill(void) { cli(); return 1; }	

	#define ATOMIC_BLOCK(exitmode) for ( exitmode __ToDo = __ikill(); __ToDo ; __ToDo = 0 )

	#define ATOMIC_RESTORESTATE uint8_t sreg_save __attribute__((__cleanup__(__irestorevalue))) = SREG,
	#define ATOMIC_ASSUMEON     uint8_t sreg_save __attribute__((__cleanup__(__irestorefixed))) = 0,

Use as such:

YourFile.c

#include 
USES_ATOMIC_BLOCKS;

int main(void)
{
	ATOMIC_BLOCK(ATOMIC_ASSUMEON)
	{
	    PORTB = 0x23;
		if (PORTA == 0xDC)
		  break;
		PORTC = 0x32;
	}

	ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
	{
	    PORTB = 0x23;
		if (PORTA == 0xDC)
		  break;
		PORTC = 0x32;
	}
	
	PORTD = 0x12;
	
	while (1);
}

Is this worth submitting as an AVRLibC patch?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dean,

Quote:
Trying to use a break in my original macro would produce a warning, as there is no control structure to break out of.
Ok, I see. I use break only in switches and try to exit all loops gracefully.
Quote:
ASSUMEON isn't dangerous. I use it in my code in routines where I know that the global enable flag must be set for it to be called, thus I save words by not restoring the previous state. If you need to retain the previous value, just use RESTORESTATE.
IMHO it is, since it enforces sei in a somewhat hidden manner. If you know beforehand you can use a sei after a block, then why bother to save the previous state at all? A plain cli at the beginning should do.
Quote:
I guess I just can't understand the problem you obviously see.
Well, it's on the previous page, where a timer queue head must be examined atomically, but rest of the queue is not critical. Finally the queue must be updated atomically. Original version with SEI_BLOCK won't work, but a RESTORE_BLOCK would, if there was one. Two consecutive cli-blocks are no good either.
I don't like to retest something for which the result is already known and cannot change. Nor do I like to add a flag and test that after getting out from the critical block.

Alternative approach would be to have:

#define BEGIN_CRITICAL   uint8_t sreg_save = SREG; cli();
#define END_CRITICAL     SREG = sreg_save;
#define PAUSE_CRITICAL   END_CRITICAL
#define RESUME_CRITICAL  cli();

without any braces at all. This can be used in the timer queue code:

{
   BEGIN_CRITICAL

   timer_message_t * next = blaa blaa...

   if ( tm->delay > next->delay ) 
     { 
        tm->delay -= next->delay; 
        next = (timer_message_t *) next->node.next; 

        PAUSE_CRITICAL 

        while ( tm->delay > next->delay ) 
          { 
           tm->delay -= next->delay; 
           next = (timer_message_t *) next->node.next; 
          } 

        RESUME_CRITICAL
   
     }
  
  // Atomically update the queue

  END_CRITICAL
}

But that is just terrible.

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't see how a requirement of everything being in a single file would be a tradeoff. Putting it all to avr/interrupt.h or, if you like avr/atomic.h, is fine. I don't think USES_ATOMIC_BLOCK is necessary. Just have the helper functions defined in the header file. These are static and inlined, and we have include sentry in place anyway. I prefer the first solution, but you might want ot consider changing ATOMIC_ASSUMEON to e.g. ATOMIC_FORCEON?

My original problem about creating a non-critical section in the middle of a critical one is still open.

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've attached a new header file. If anyone wants me to, I'll add in DoxyGen and submit as an AVRLibC patch.

The USES_ATOMIC_BLOCK is no longer necessary, as it is taken care of now that the macros and functions are in a separate header file.

I've added in a NON_ATOMIC_BLOCK macro which does the opposite to the ATOMIC_BLOCK macro. Nest inside an ATOMIC block, or use standalone as you wish. Like the USES_ATOMIC_BLOCK, it has both a NONATOMIC_RESTORESTATE and NONATOMIC_FORCEON variants.

- Dean :twisted:

Attachment(s): 

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

guys, this was such a SIMPLE question, but it certainly does have profound ramifications. While I love an elegant solution, the simple solution is almost always better.

This was all I had in mind when I asked the question.

/******************************************************************************
* processor_variant.h
*   local file to project that contains the includes for specific processor
*   used in the project
*  
*   PROCESSOR: Atmel ATmega1280/2560
******************************************************************************/

#ifndef PROCESSOR_VARIANT_H
#define PROCESSOR_VARIANT_H

#include 
#include 
#include 
#include 
#include 
#include 

#define EnableInterrupts  sei()
#define DisableInterrupts cli()

// NOTE: these macros must be used within the same stack frame to have access
//   to the shadow variable (sreg_save)
#define SaveInterrupts      uint8_t sreg_save = SREG
#define RestoreInterrupts   SREG = sreg_save

#endif

I use them in my kernel like this example:

/******************************************************************************
* enter into critical section - no task switches allowed, but interrupts
* are not disabled
******************************************************************************/
RETURN_CODE pmKernel::enterCriticalSection(void)
{
    //TODO:  is it an error to multiply enter critical section?

    SaveInterrupts;
    DisableInterrupts;

    operating_flags |= FLAG_IN_CRITICAL_SECTION;    // set flag

    RestoreInterrupts;
    return SUCCESS;
}

/******************************************************************************
* exit critical section - re-enable normal task switching 
******************************************************************************/
RETURN_CODE pmKernel::exitCriticalSection(void)
{
    SaveInterrupts;
    DisableInterrupts;

    operating_flags &= ~FLAG_IN_CRITICAL_SECTION;   // clear flag
    RestoreInterrupts;

    // handle the scheduling as soon as possible after exiting a critical section
    runNextTask();

    return SUCCESS;
}

By definition, this sort of messing about with the processor is an advanced topic. If you play at this level, you need to know exactly what you are doing. Trying to come up with a bomb-proof library solution is not an easy thing. I probably wouldn't use a library solution myself anyway since the author doesn't have any clue what I want to do. It is up to the programmer to handle the ramifications. The simpler solutions are easier to understand and debug.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dean,
Now the __irestore does exactly what my sei-block did. That is the dangerous thing, because it enforces sei instead of rolling back to original state that was effective before the start of the critical block. What I was looking for is something like this:

static inline void    __irestoreoriginal(const uint8_t *s) { SREG = *s; return 1; }

#define NORMAL_GETATOMICSTATE      uint8_t sreg_temp = SREG
#define NORMAL_SAVEATOMICSTATE     uint8_t sreg_save __attribute__((__cleanup__(__irestorevalue))) = sreg_temp

#define ATOMIC     for ( ATOMIC_RESTORESTATE, __ToDo = __ikill(); __ToDo ; __ToDo = 0 )
#define NORMAL     for ( NORMAL_GETATOMICSTATE, __ToDo = __irestoreoriginal( sreg_save ), NORMAL_SAVEATOMICSTATE ; __ToDo ; __ToDo = 0 )

I'm not a c-pro, so I'm assuming multiple statements within the for-loop initialization part are executed left to right. If not, everything will fall apart.
The code relies on seeing the parent block's sreg_save when __irestoreoriginal() is called.
Drawback here is the need for sreg_temp for saving the atomic state SREG before it is overridden with a local variable of same name.

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ripa:

It's not dangerous at all, and it works perfectly.

ATOMIC_BLOCK(ATOMIC_FORCEON) does the following:

    1) Executes a CLI 2) Executes Block Code
    3) Once block exits (at any time) it executes a SEI

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) does the following:

    1) Saves SREG status 2) Executes a CLI
    2) Executes Block Code
    3) Once block exits (at any time) it restores SREG status

The NONATOMIC_BLOCK macro works the same way, but with SEI and CLI interchanged.

Use the FORCEON block when you know the block is always called when interrupts are enabled - it saves a few cycles as it doesn't have to save the current SREG status.

Use the RESTORESTATE when you want to restore the current SREG status as soon as the block exits (from any exit path).

What's dangerous about that? Used correctly, they work perfectly.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Agreed. When used correcly it works fine. I just fell to a trap with my sei-block - which also, when used correctly would work fine. Jörg saved me, so please see his mail on the previous page.

But, what do you think about the NORMAL block?

-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hey guys?
Where'd ya all go?
I really don't like this silence.
You could at least say it is plain stupid/not feasible/against all good programming practices/originating from Finland/

Please

Best regards
-Ripa

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ripa,

Isn't your macro above just different names for the last design I posted?

Also, I've found (thanks Joerg!) that you need a "__asm__ volatile ("" ::: "memory");" in your epilogue routines to prevent GCC from re-ordering the contents.

Here's my latest version of the header file, which has been changed to be suitable for inclusion in AVRLibC. Once I add in documentation, I'll submit this as a patch to the project:

#ifndef _AVR_ATOMIC_H_
#define _AVR_ATOMIC_H_

#include 

static __inline__ uint8_t __iSeiRetVal(void) { sei(); return 1; }   
static __inline__ uint8_t __iCliRetVal(void) { cli(); return 1; }   
static __inline__ void    __iSeiParam(const uint8_t *__s) \
 { sei(); __asm__ volatile ("" ::: "memory"); (void)s; }
static __inline__ void    __iCliParam(const uint8_t *__s) \
 { cli(); __asm__ volatile ("" ::: "memory"); (void)s; }
static __inline__ void    __iRestore(const  uint8_t *__s) \
 { SREG = *__s; __asm__ volatile ("" ::: "memory"); }

#define ATOMIC_BLOCK(type) for ( type, __ToDo = __iCliRetVal(); \
	                       __ToDo ; __ToDo = 0 )

#define ATOMIC_RESTORESTATE uint8_t sreg_save \
	                        __attribute__((__cleanup__(__iRestore)))  = SREG

#define ATOMIC_FORCEON uint8_t sreg_save \
	                   __attribute__((__cleanup__(__iSeiParam))) = 0
	
#define NONATOMIC_BLOCK(type) for ( type, __ToDo = __iSeiRetVal(); \
	                          __ToDo ;  __ToDo = 0 )
	
#define NONATOMIC_RESTORESTATE uint8_t sreg_save \
	                           __attribute__((__cleanup__(__iRestore))) = SREG

#define NONATOMIC_FORCEOFF uint8_t sreg_save \
	                       __attribute__((__cleanup__(__iCliParam))) = 0

#endif

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dean,

My ATOMIC is indeed just a shorthand for ATOMIC_BLOCK(ATOMIC_RESTORESTATE). This will be the most used macro, so I think it deserves a nickname.

But NORMAL is something different. It creates a block that can be nested inside ATOMIC and restores the state *present before* the ATOMIC. (At least that is the intention. Haven't verified it though.)
Like this:

void foo(void)
{
  // cli or sei is in effect just like before entering foo()

  ATOMIC
  {
    // ints disabled
    FirstPartOfCriticalOperation();

    NORMAL  // Not the same as NONATOMIC
    {
       // ints restored to state they were before foo() was called
      NonCriticalPart();
    }

    // ints disabled again
    RestOfCriticalOperation();
  }

  // restored to state before foo()
}

// the code calling foo()
void bar(void)
{
  sei();  // how things normally are
  
  foo();  // non-critical part executes with ints on
  
  ATOMIC
  {
    foo();  // the whole function executes with ints off 
  }
}

If there would be a NONATOMIC-block inside foo(), enclosing the call to foo() inside an ATOMIC-block would guarantee nothing. Nonatomic block would still enforce sei() regardless of history. This is what I previously meant when I said nonatomic is "dangerous."

BR
-Ripa

EDIT: Also note that NORMAL-block is not the same thing as gap between two consecutive ATOMIC-blocks. Scope rules are different, and a gap cannot be inside a conditional statement.