Issue in MSP430X port

peppet wrote on Friday, August 03, 2012:

The line 126 of Source/Portable/IAR/MSP430X/portext.s43

push.w sr

is wrong in that, being it in ISR context, SR was already saved on the stack (after PC) and then modified by CPU (SR &= SCG0).

As a result of this, the line 102 of the same file

bic.w # 0xF0, 0 (sp)

has no effect, since it works on an fake status register.

Moreover vPortTickISR should never be called via a CALL, as at line 217 of port.c, where the return address for the restored task will be that of the RETI (line 218 of port.c).

Of course, the context switch neverthless does most of its job, as finally that RETI restore the SR and the PC of the task to be restored, but no changes were actually made ​​to the SR (as intended by bic instruction above).

As a consequence a task that goes into LPM, remains there indefinitely. In particular, if LPM is invoked in the idle hook, as in line 616 of Demo/MSP430X_MSP430F5438_IAR/main.c, the idle task is blocked forever (which should never happen).

As a solution, I suggest to install directly vPortTickISR as the interrupt handler for the tick event and remove the line 126 of portext.s43

Let me know if I’m wrong or I’m missing anything.

Best regards.

Peppe

westmorelandeng wrote on Friday, August 03, 2012:

Hello Peppe,

I will take a look shortly at this.  I have some tests that will verify this or not.

The port that I was involved directly with handled this a little differently; the idle task definitely executed.

Thanks for your comments,
John Westmoreland

rtel wrote on Saturday, August 04, 2012:

Thanks for taking the time to point this out.

I need to look into this in some detail, but from an initial assessment (without actually running the code, and stepping through the instructions) I’m not sure that saving the dummy SR is the problem, but that clearing the low power bits in the wrong SR is the problem.  The bits should be cleared in the SR that is already on the stack.

Looking at the data sheet it is quite clear that the SR is automatically pushed to the stack on interrupt entry, so I’m thinking that the reason it is pushed again is to ensure that the stack frame is identical to that of the vPortYield() function.

vPortYield() is called with a CALL or CALLA instruction, and that does not save the SR to the stack.  Therefore vPortYield() saves the SR to the stack itself, before calling portSAVE_CONTEXT().

I’m currently thinking that vPortYield() should also manually unstack the SR, that way portRESTORE_CONTEXT does not have to restore it, and vPortTickISR() can ignore it (other than clearing the low power bits in the value already on the stack).

I need to think about this more though, I might find a flaw in that when I try running it.  Especially as vPortYield() is called by other interrupts too - maybe the low power bits should be cleared restoring, rather than saving, a context.

Moreover vPortTickISR should never be called via a CALL

Again, I would have to look at why that is done.  I suspect it is again to ensure the stack frame between the vPortYield() function (which is a genuine function) and the call to vPortYield() in an interrupt are the same.

Regards.

rtel wrote on Saturday, August 04, 2012:

I notice the example UART ISR contains a:

__bic_SR_register_on_exit( SCG1 + SCG0 + OSCOFF + CPUOFF );

so that part is ok.  Adding the same line in vPortTickISR() may be all that is needed, but I haven’t checked yet.

Regards.

rtel wrote on Saturday, August 04, 2012:

I now have it running as follows:

The line suggested above has been added to vTickISREntry(), to make it:

#pragma vector=configTICK_VECTOR
__interrupt __raw void vTickISREntry( void )
{
extern void vPortTickISR( void );
	__bic_SR_register_on_exit( SCG1 + SCG0 + OSCOFF + CPUOFF );
	vPortTickISR();
}

The code:

	/* The last thing on the stack will be the status register.
	Ensure the power down bits are clear ready for the next
	time this power down register is popped from the stack. */
	bic.w   #0xf0, 0( sp )

has been removed (it could be left in, but as it doesn’t do anything it should be removed).

What do you think?

Regards.

peppet wrote on Monday, August 06, 2012:

The last change proposal seems adequate, I’ve tested and it works for me.

I’d also suggest to comment line 126 of portext.s43 to point-up that pushing SR there it’s mandatory to ensure that the vPortTickISR stack frame is identical to that of the vPortYield() function.

Thank you very much for your prompt support.

Best Regards,

Peppe