AVR Freaks :: View topic - Compiler alters SP at the beggining of a function?

AVR Freaks

AVR GCC forum - Compiler alters SP at the beggining of a function?

sparkymark567 - Apr 23, 2005 - 02:20 PM
Post subject: Compiler alters SP at the beggining of a function?

I'm writing a pre-emptive RTOS, some functions are reentrant.....i.e. can be used by more than one task.

Currently my RTOS fails (only sometimes), for example when I add/remove a few lines of useless code to a task.....

Each task has it's own stack. The scheduler does a task switch by:

1. ISR(attribute naked). PC is pushed on to the current task's stack.
2. push r0 to r1 and SREG on to the current task's stack
2. save the stack pointer for the current task.
3. selects a new task
4. restore the stack pointer for the new task.
5. pop SREG and r31 to r0 from the stack.
6. "reti"

Here, in the assembler listing for my version of function malloc(). The stack pointer is manipulated at c72 onwards. This appears several times in the assembler listing for various other functions.

Can someone please tell me what is happening here?
IFF the scheduler is invoked at the wrong moment, could this manipulation of the stack pointer cause my RTOS to fail?

Why: load the stack pointer in to r28 and r29, set bit 10 in word r28 and restore the stack pointer? What is the purpose, suppose bit 10 is already set? is that possible?

Code:

00000c52 <malloc>:

     c52:   4f 92          push   r4

     c54:   5f 92          push   r5

     c56:   6f 92          push   r6

     c58:   7f 92          push   r7

     c5a:   8f 92          push   r8

     c5c:   9f 92          push   r9

     c5e:   af 92          push   r10

     c60:   bf 92          push   r11

     c62:   cf 92          push   r12

     c64:   df 92          push   r13

     c66:   ef 92          push   r14

     c68:   ff 92          push   r15

     c6a:   0f 93          push   r16

     c6c:   1f 93          push   r17

     c6e:   cf 93          push   r28

     c70:   df 93          push   r29

     c72:   cd b7          in   r28, 0x3d   ; 61

     c74:   de b7          in   r29, 0x3e   ; 62

     c76:   2a 97          sbiw   r28, 0x0a   ; 10

     c78:   0f b6          in   r0, 0x3f   ; 63

     c7a:   f8 94          cli

     c7c:   de bf          out   0x3e, r29   ; 62

     c7e:   0f be          out   0x3f, r0   ; 63

     c80:   cd bf          out   0x3d, r28   ; 61

note:
0x3d is SPL
0x3e is SPH
0x3f is SREG

Thanks
Mark

mckenney - Apr 23, 2005 - 03:17 PM
Post subject: RE: Compiler alters SP at the beggining of a function?

Quote:

2. push r0 to r1 and SREG on to the current task's stack

When do you save the rest of the registers?

Quote:

IFF the scheduler is invoked at the wrong moment, could this manipulation
of the stack pointer cause my RTOS to fail?

The sequence looks safe enough -- it was evidently designed with ISRs in mind.
The scheduler (ISR) will either see the "old" SP or the "new".

Quote:

set bit 10 in word r28

SBIW is a (16-bit) subtract instruction.

sparkymark567 - Apr 23, 2005 - 03:28 PM
Post subject: Re: RE: Compiler alters SP at the beggining of a function?

mckenney wrote:

Quote:

2. push r0 to r1 and SREG on to the current task's stack

When do you save the rest of the registers?

Sorry my typo...I mean r0 to r31 and SREG. Is there anything else should be saving on the stack?

Quote:

SBIW is a (16-bit) subtract instruction.

thanks, that makes more sense. I can see what's happening now.

mckenney - Apr 23, 2005 - 04:11 PM
Post subject: RE: Re: RE: Compiler alters SP at the beggining of a functio

Quote:

5. pop SREG and r31 to r0 from the stack.
6. "reti"

Is this ISR the only way task switching happens? Or are there (task) library
functions which block, and use a function-call into the task switcher? If the
latter, you have two concerns:
1) If the (new) task called into the task switcher while enabled (so the saved
SREG has the GIE bit on), restoring SREG will enable interrupts while in
the ISR.
2) If the (new) task called into the task switcher while disabled, the RETI
will enable interrupts. Having a task block while disabled is anomalous,
and you need (as a design thing) to figure out what you want to have
happen. In any case, though, you need to either provide for it or prohibit it.

Maybe everything is fine here, but I'd say that (as is typically the case) you
need to go carefully over the last maybe 5 instructions of the task switcher
to make sure they cover all possibilities.

sparkymark567 - Apr 23, 2005 - 05:23 PM
Post subject:

thanks...good info for me to think about.

The timer 1 compare match ISR is the only place where I call the scheduler. The problem may well be something to do with sei() cli(),
I originally had very few cli and sei instructions where:

1)enterCritical disabled only the timer1 compare match interrupt.
2)exitCritical enabled only the timer1 compare match interrupt.
3)sei and cli were used only when reading/writing to the stack pointer.
The idea was to allow other ISR's to occur at any time.

More recently I had changed the code (in an effort to simplify it for debugging) and used sei and cli in more places including enterCritical and exitCritical.

I have now changed the code back to how it was originally....i.e. it has very few sei and cli instructions. It's working again, but it's difficult to tell whether I have solved the real problem...as the bug/fault is intermittent anyway.

here is the timer 1 compare match ISR:

Code:

//Switch to the highest priority task which is ready to run

void SIG_OUTPUT_COMPARE1A (void) __attribute__ ((naked));

void SIG_OUTPUT_COMPARE1A (void)

{

   enterCritical();

   asm volatile ("sei");

   SAVE_CONTEXT();

   scheduler();  

   RESTORE_CONTEXT();

   exitCritical();

   asm volatile ("reti");

}

any immediate thought? does it seem safe?

here is the code for enterCrictical and exitCritical

Code:

void enterCritical(void)

{

   TIMSK1 &= ~_BV(OCIE1A);

}

void exitCritical(void)

{

   TIMSK1 |= _BV(OCIE1A);

}

Once the RTOS is running neither task nor scheduler are allowed to change the value of TIMSK1. I'm hoping therefore that the functions enterCritical and exitCritcal are also safe.

the assembler listing for enterCritical and exitCritical is as follows

Code:

void enterCritical(void)

{

   TIMSK1 &= ~_BV(OCIE1A);

     364:   80 91 6f 00    lds   r24, 0x006F

     368:   8d 7f          andi   r24, 0xFD   ; 253

     36a:   80 93 6f 00    sts   0x006F, r24

     36e:   08 95          ret

00000370 <exitCritical>:

}

void exitCritical(void)

{

   TIMSK1 |= _BV(OCIE1A);

     370:   80 91 6f 00    lds   r24, 0x006F

     374:   82 60          ori   r24, 0x02   ; 2

     376:   80 93 6f 00    sts   0x006F, r24

     37a:   08 95          ret

Are there any immediate thoughts or noticable mistakes?

Thanks
Mark

sparkymark567 - Apr 23, 2005 - 05:29 PM
Post subject:

another thought: I could have a problem here with enterCritical and exitCritical?
I am losing r24??? in the ISR. How can I avoid this?

mckenney - Apr 23, 2005 - 05:45 PM
Post subject:

Quote:

I am losing r24??? in the ISR

along with some of the flags in SREG.

First thought: Get rid of the "naked" attribute, and let the ISR save some registers.
They should belong to the current task, so putting them back later is redundant
but harmless. If you allow nested interrupts, this gets tricky, since you won't be
finishing the ISR until the switched-from task gets scheduled again, but since
this is true now this change doesn't introduce any new problems.

mckenney - Apr 23, 2005 - 06:01 PM
Post subject:

Code:

   asm volatile ("sei");

   SAVE_CONTEXT();

This looks backwards; personally, I'd be inclined to run disabled through the
entire scheduling process. On the other hand, there's about a zillion (give or take
a few ( Smile

)) ways to design/code an OS, so sweeping generalities (along with
debugging piecemeal) are difficult.

sparkymark567 - Apr 23, 2005 - 08:20 PM
Post subject:

That's a really good idea.
I could drop the naked attripute from the interrupt, so r24 (or whatever) and SREG will be pushed on to the stack by the compiler. But to prevent pushing and popping all the registers used by the scheduler/kernal I could apply the __attribute__((naked)); there insead.

It would propably be easier for me to disable all interrupts while running the scheduler.....but it's not really an option as I need to maintain a fast interrupt response times for my my applictaion. This is part of the reason why I decided to write my own RTOS rather than use someone elses.

N.Winterbottom - Apr 23, 2005 - 10:34 PM
Post subject:

I may have missed it but I couldn't see where you saved the stack frame. Sure you've saved the registers but you must also save the stack frame because it could contain variables or even return addresses etc.

I don't think you can simply leave it on the stack either unless you define a fixed stack size for each task.

Nigel

sparkymark567 - Apr 24, 2005 - 02:26 AM
Post subject:

I have tried losing the naked attribute from the timer1 ISR, but the compiler still doesn't save any registers in the ISR. I have re-writen enterCritical() and exitCritical() using inline asm, this ensures that r24 and SREG are not lost.

N.Winterbottom: I have a fixed stack size for each task. The stack size is passed to the function createTask(). It's not an implementation which I like, since choosing the stack size is trial and error. I am interested to know of any alternative methods which can be used for a pre-emptive scheduler. What is a stack frame?

N.Winterbottom - Apr 25, 2005 - 11:46 AM
Post subject:

An alternative is for each task to have its stack overlay the same area of memory. This increases the work load for task switching immensely but each task can have a varying stack size.

The task switcher must save registers etc then hoover up the stack and stash it somewhere then the saved stack from the new task is restacked,registers restored and task executed.

-----------------------

In your enterCritical and exitCritical functions why dont you write them in assembly or even gcc inline assembly

Code:

    push r24

    lds   r24, 0x006F

    ori   r24, 0x02

    sts   0x006F

    pop r24

sparkymark567 - Apr 26, 2005 - 12:51 PM
Post subject:

OK that's an interesting idea....but it sounds like a lot of work, and will increase the overhead of running the RTOS. Perhaps I may include something like this (as a #define) option in the future........it would prevent stack to stack collision and would be better from a memory point of view. for now though i'll stick with a fixed stack size for each task.
Thanks
Mark

mckenney - Apr 26, 2005 - 02:44 PM
Post subject:

While a "copy the stack" approach is an interesting idea, I don't think (even
ignoring the cost of the copying) it will actually save you much of anything.

The problem is that you need to have space to stow all those bits. Since your
system is pre-emptive, that storage space will have to reflect the (total)
worst-case stack size over all tasks -- which is what you have now -- so in
effect you end up with (N+1) stacks.

N.Winterbottom - Apr 26, 2005 - 04:08 PM
Post subject:

Oh yes, that might be a big problem.

I know...you could interrupt a task and check the size of its stack, you could say yikes thats much too big for me to store and return execution to the original task and try again later.

Who said designing an RTOS was easy.

Nigel

mckenney - Apr 26, 2005 - 04:42 PM
Post subject:

Another approach to the stack space problem is to use a run-to-completion
architecture, in which there's only one stack. Personally, I think these have
gotten a bad rap due to their historical association with (poorly-designed)
"service loop" schemes. You can in fact incorporate priority and blocking
in a run-to-completion OS, but you need to think about your tasks differently --
in particular, the infinite-loop task (as taught in school) goes out the window.
Pre-emption in the general sense is difficult, but you may find that with a
suitable task design pre-emption becomes moot, or at least that a
restricted form of pre-emption suffices.

And it's not just for the "little guys" -- I've seen this implemented in a Unix
(or Mach?) kernel, where they used the term "Continuations".

(And yes, I know this is of no help at all in what you're trying to do -- I'm just blabbing.)