Forum Menu




 


Log in Problems?
New User? Sign Up!
AVR Freaks Forum Index

Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Author Message
ray62202
PostPosted: Dec 30, 2006 - 07:11 PM
Rookie


Joined: Sep 15, 2005
Posts: 23


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
 
 View user's profile Send private message  
Reply with quote Back to top
dl8dtl
PostPosted: Dec 30, 2006 - 09:50 PM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

In C, this is simply:
Code:

...
  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.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
ray62202
PostPosted: Dec 30, 2006 - 11:24 PM
Rookie


Joined: Sep 15, 2005
Posts: 23


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
 
 View user's profile Send private message  
Reply with quote Back to top
abcminiuser
PostPosted: Dec 31, 2006 - 01:58 AM
Moderator


Joined: Jan 23, 2004
Posts: 10228
Location: Melbourne, Australia

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):

Code:
   // 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:

Code:
   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 Evil

_________________
Make Atmel Studio better with my free extensions. Open source and feedback welcome!
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
dl8dtl
PostPosted: Dec 31, 2006 - 07:39 AM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

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 <avr/interrupt.h> 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.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
ray62202
PostPosted: Dec 31, 2006 - 06:01 PM
Rookie


Joined: Sep 15, 2005
Posts: 23


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.
 
 View user's profile Send private message  
Reply with quote Back to top
stu_san
PostPosted: Jan 01, 2007 - 05:14 AM
Raving lunatic


Joined: Dec 30, 2005
Posts: 2327
Location: Fort Collins, CO USA

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
 
 View user's profile Send private message  
Reply with quote Back to top
abcminiuser
PostPosted: Jan 01, 2007 - 11:36 AM
Moderator


Joined: Jan 23, 2004
Posts: 10228
Location: Melbourne, Australia

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:

Code:
{
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:

Code:
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 Evil

_________________
Make Atmel Studio better with my free extensions. Open source and feedback welcome!
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
dl8dtl
PostPosted: Jan 01, 2007 - 10:22 PM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

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.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
lfmorrison
PostPosted: Jan 01, 2007 - 11:36 PM
Raving lunatic


Joined: Dec 08, 2004
Posts: 4722
Location: Nova Scotia, Canada

If you don't like the terminating macro requirement, then an alternate formulation which doesn't require it would be something like:
Code:
#define CRITICAL_BLOCK(x) do \
{ \
   unsigned char _tmp_sreg = SREG; \
   cli(); \
   { \
      x \
   } \
   SREG = _tmp_sreg; \
}while(0);


An instance would look like this:
Code:
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:
Code:
while (SomeFlag)
{
   CRITICAL_BLOCK(
      SomeAtomicCriticalAction();
      if (SomeOtherFlag)
        break; // This line will break things!  (no pun intended)
   )
}
 
 View user's profile Send private message  
Reply with quote Back to top
abcminiuser
PostPosted: Jan 02, 2007 - 02:30 AM
Moderator


Joined: Jan 23, 2004
Posts: 10228
Location: Melbourne, Australia

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 Wink.

- Dean Twisted Evil

_________________
Make Atmel Studio better with my free extensions. Open source and feedback welcome!
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
lfmorrison
PostPosted: Jan 02, 2007 - 02:39 AM
Raving lunatic


Joined: Dec 08, 2004
Posts: 4722
Location: Nova Scotia, Canada

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.
 
 View user's profile Send private message  
Reply with quote Back to top
eerola
PostPosted: Jan 02, 2007 - 04:28 PM
Rookie


Joined: Aug 20, 2002
Posts: 26


Dean, Jrg

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?

Code:

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.

Code:

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
 
 View user's profile Send private message  
Reply with quote Back to top
dl8dtl
PostPosted: Jan 02, 2007 - 04:41 PM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

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:
Code:

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.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
eerola
PostPosted: Jan 02, 2007 - 05:25 PM
Rookie


Joined: Aug 20, 2002
Posts: 26


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

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

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
 
 View user's profile Send private message  
Reply with quote Back to top
eerola
PostPosted: Jan 02, 2007 - 07:36 PM
Rookie


Joined: Aug 20, 2002
Posts: 26


Uh, oh, after rethinking this, I have to agree there is a design error. Thanks for pointing this out, Jrg. 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.

Code:

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
 
 View user's profile Send private message  
Reply with quote Back to top
stu_san
PostPosted: Jan 02, 2007 - 08:57 PM
Raving lunatic


Joined: Dec 30, 2005
Posts: 2327
Location: Fort Collins, CO USA

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

Dean:
Code:
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)....
Code:
while (SomeFlag)
{
   portENTER_CRITICAL();
   {
      SomeAtomicCriticalAction();
   }
   portEXIT_CRITICAL();
   if (SomeOtherFlag)
     break;
}
to provide the non-atomic time brief interludes, or...
Code:
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
 
 View user's profile Send private message  
Reply with quote Back to top
lfmorrison
PostPosted: Jan 02, 2007 - 11:46 PM
Raving lunatic


Joined: Dec 08, 2004
Posts: 4722
Location: Nova Scotia, Canada

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
 
 View user's profile Send private message  
Reply with quote Back to top
abcminiuser
PostPosted: Jan 03, 2007 - 12:45 AM
Moderator


Joined: Jan 23, 2004
Posts: 10228
Location: Melbourne, Australia

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:

Code:
   #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:

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


Wink

- Dean Twisted Evil

_________________
Make Atmel Studio better with my free extensions. Open source and feedback welcome!
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
stu_san
PostPosted: Jan 03, 2007 - 04:59 AM
Raving lunatic


Joined: Dec 30, 2005
Posts: 2327
Location: Fort Collins, CO USA

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". Shocked Maybe I'm getting to old for this s**t. Confused

Thanks, guys!

Stu
 
 View user's profile Send private message  
Reply with quote Back to top
Display posts from previous:     
Jump to:  
All times are GMT + 1 Hour
Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Powered by PNphpBB2 © 2003-2006 The PNphpBB Group
Credits