STM32 HAL + FreeRTOS (tickless) -> systick drift

system · November 17, 2016, 9:39am

embeddedoli wrote on Thursday, November 17, 2016:

Hello,
we are using a STM32F0 with FreeRTOS. Code was generated using the ST CubeMX and therefore the ST HAL is implemented as well.

We have FreeRTOS running in tickless idle mode and several task / peripherals. As our overall system was designed to rely on the systick and/or the accuracy of the FreeRTOS wait/delay/suspend features we run into problems if anything corrupts the system time.

The HAL uses a timer as timebase generating a tick every ms. Same for FreeRTOS - we expect the systick to increase by 1 each ms.

At the very moment we figured out that sometimes we run into a SPI error condition. In this condition the HAL waits for 50ms for a certain flag (that will not be changed) and runs into a timeout after (all in the ISR). We absolutely understand that this is not a desired behaviour but the HAL is implemented that way.

But from this behaviour we figured out a problem that seems bigger than that. The systick in idle mode seems to miss all the time that is passed in an ISR. So even if all our interrupts are handled within several µs we miss those times on the system clock. That leads to the problem that those missed times summ up to a huge drift of about 100ms each 10s runtime. This highly depends on the amount and the time passed in the interrupts that occure during the sleep.

If we disable the tickless idle feature our power consumption goes up but we see a better tick. It seems like in this case we only miss interrupts that are longer than 1ms - but I’m not sure about this. We know that the drift in time is smaller without the tickless idle feature but if the huge lock in the SPI ISR occures we still miss those 50ms on the clock.

We do not understand why the systick misses the times of the interrupts. We understand that as long as an interrupt is active no change to the systick can be achieved but we do not understand why the missed time is not corrected as soon as the next “tick correction” takes place (probably directly after the ISR is handled)

Are any of my assumptions wrong? Is there anything we can do about this issue?
So there are two issues:
1.) Drift in systick in tickless idle mode
2.) All time passed in an ISR is lost for the sysclock

We are using the latest Cube HAL (1.6.0) for the STM32F072 and the FreeRTOS 8.2.3.

rtel · November 17, 2016, 3:29pm

rtel wrote on Thursday, November 17, 2016:

When using tickless idle mode the clock is stopped and started as it is
reprogrammed to change the time at which the next interrupt is
generated. For that reason it is expected that the time will drift a
little bit - and that is well documented. I’m not sure if 100ms in 10
seconds is ‘normal’ or not, although some of the ST ISRs do take a
little longer than some as there are busy loop timeouts inside the ISRs.

There is a fiddle factor used to compensate for this, which you can try
playing with as its default value is set according to the system clock.
Have a look at the constant portMISSED_COUNTS_FACTOR in
FreeRTOS/Source/portable/[compiler]/ARM_CMn/port.c, and how it is used
to set ulStoppedTimerCompensation, and then how the
ulStoppedTimerCompensation variable itself is set. You can then
increase or decrease the value of portMISSED_COUNTS_FACTOR as
appropriate to see if it helps.

system · November 18, 2016, 6:33am

embeddedoli wrote on Friday, November 18, 2016:

Hello,
thanks for the reply. I already tried to change that value according to the amount each interrupt takes. Unfortunately this amount does compensation in respect to the “normal” drift.

What we now figured out is that the logical order after the wfi instructions is somehow strange as it excludes the time passed in the ISR from the calculation:

xModifiableIdleTime = xExpectedIdleTime;
			configPRE_SLEEP_PROCESSING( &xModifiableIdleTime );
			if( xModifiableIdleTime > 0 )
			{
				__DSB();
				__WFI();
				__ISB();
			}
			configPOST_SLEEP_PROCESSING( &xExpectedIdleTime );

			/* Stop SysTick.  Again, the time the SysTick is stopped for is
			accounted for as best it can be, but using the tickless mode will
			inevitably result in some tiny drift of the time maintained by the
			kernel with respect to calendar time. */
			ulSysTickCTRL = portNVIC_SYSTICK_CTRL;
			portNVIC_SYSTICK_CTRL = ( ulSysTickCTRL & ~portNVIC_SYSTICK_ENABLE );

			/* Re-enable interrupts - see comments above __disable_interrupt()
			call above. */
			__enable_interrupt();

The point is the position on which the interrupts are enabled again (__enable_interrupt()). Right before that instruction the systick is disabled. The problem is that if a normal interrupt starts it will start at the __WFI(); instruction and go further till the __enable_interrupt(); statement. Right there the context switch takes place and the control is handed over to the ISR. So all time passed in the ISR is missed by the systick counter!

So if the order is switched to enable the interrupt right before the stop of the systick the time of the interrupt that caused the wakeup will be took into consideraton aswell.

Somehow this looks to easy for a solution and I think that changing this order will have negativ side effects as well.

So can you maybe explain why this order has been chosen and/or what I’m missing in my assumptions here?

rtel · November 18, 2016, 8:14pm

rtel wrote on Friday, November 18, 2016:

This is getting into a rather detailed area so will need to study a bit
more - like you say side effects are easy to introduce in this area.

rtel · November 21, 2016, 6:06am

rtel wrote on Monday, November 21, 2016:

Working through this I think the following code change, which would help
the scenario you describe, would be safe. Do you agree?

xModifiableIdleTime = xExpectedIdleTime;
configPRE_SLEEP_PROCESSING( xModifiableIdleTime );
if( xModifiableIdleTime > 0 )
{
     __asm volatile( "dsb" );
     __asm volatile( "wfi" );
     __asm volatile( "isb" );
}
configPOST_SLEEP_PROCESSING( xExpectedIdleTime );

/*** CODE ABOVE HERE UNCHANGED, CODE BELOW HERE HAS CHANGED. ***/

/* Re-enable interrupts - see comments above the cpsid instruction()
above. */
__asm volatile( "cpsie i" );

/* Disable the SysTick clock without reading the
portNVIC_SYSTICK_CTRL_REG register to ensure the
portNVIC_SYSTICK_COUNT_FLAG_BIT is not cleared if it is set. */
portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT | 
portNVIC_SYSTICK_INT_BIT );

/* Determine if the SysTick clock has already counted to zero and
been set back to the current reload value (the reload back being
correct for the entire expected idle time) or if the SysTick is yet
to count to zero (in which case an interrupt other than the SysTick
must have brought the system out of sleep mode). */
if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
{

system · November 22, 2016, 8:46am

embeddedoli wrote on Tuesday, November 22, 2016:

Hello and thanks for the reply again!

I’ve tried to implement the code you gave me. You probably did not use the CM0 port but that doesn’t matter that much. I’ve tried to use the CM0 defines instead.

So from my current perspective it seems like you do not disable the tick at all now?

The portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT | portNVIC_SYSTICK_INT_BIT ); statement will set the clock and interrupt bit but the old code “removed” the clock bit. If this is the case do we really need to set those bits here? I mean the clock is probably still running and the interrupt should be active as well…

Furthermore you adapted the order change that I performed - the first instruction you mention is the enable interrupt statement. In the old code the instruction was performed after the clock was put into suspension state. Has that been put into consideration?

What I understood from your reply was that it might be unsafe to read the complete control register as we might remove the count flag. In the CM0 port the control register is read and saved for this case at the very beginning. So the first statment is ulSysTickCTRL = portNVIC_SYSTICK_CTRL; followed by the changes to the control register. All further calculations are so based on the here saved value. If I’m not mistaken your implementation is based on the actual control register and it is not saved before. For example the check of the count flag is directly performed on the register - in the CM0 port the saved value is used: if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 ) { vs. CM0 implementaion: if( ( ulSysTickCTRL & portNVIC_SYSTICK_COUNT_FLAG ) != 0 )

So I think the issue you targeted with your change might not apply to the CM0 port.

rtel · November 22, 2016, 5:23pm

rtel wrote on Tuesday, November 22, 2016:

I’ve tried to implement the code you gave me.

Note the posted code was the result of an analysis - I didn’t actually
try compiling or running it - so there is the potential for err.

You probably did not
use the CM0 port but that doesn’t matter that much. I’ve tried to use
the CM0 defines instead.

I don’t think we provide a tickless idle for the M0, although it can be
copied from the M3, M4F or M7 code. I was looking at code that used a
different compiler though, so some of the compiler specifics will have
been different - for example how the lines of asm code are added in to
the C code.

So from my current perspective it seems like you do not disable the
tick at all now?

Um, if that is the case, it was not intended!

The |portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT |
portNVIC_SYSTICK_INT_BIT );| statement will set the clock and
interrupt bit but the old code “removed” the clock bit. If this is
the case do we really need to set those bits here? I mean the clock
is probably still running and the interrupt should be active as
well…

When the system is running normally (with ticks) the
portNVIC_SYSTICK_CTRL_REG register has the portNVIC_SYSTICK_CLK_BIT,
portNVIC_SYSTICK_INT_BIT and portNVIC_SYSTICK_ENABLE_BIT set. That is,
bits 0, 1 and 2 set.

The old code:

ulSysTickCTRL = portNVIC_SYSTICK_CTRL_REG;
portNVIC_SYSTICK_CTRL_REG = ( ulSysTickCTRL & 
~portNVIC_SYSTICK_ENABLE_BIT );

read the portNVIC_SYSTICK_CTRL_REG (SYST_CSR) value, then wrote back the
value with the portNVIC_SYSTICK_ENABLE_BIT bit (bit 0) clear. That will
stop the counter and clear the COUNTFLAG bit (reading the register
clears the COUNTFLAG bit). After the write the
portNVIC_SYSTICK_CTRL_REG register will still have bits 1 and 2 set, but
bit 0 clear.

The intention of the new code, which as per my comments above I didn’t
actually run:

portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT | 
portNVIC_SYSTICK_INT_BIT );
if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )

is to clear bit 0 (ENABLE) in the portNVIC_SYSTICK_CTRL_REG register
without reading the register first. Avoiding reading the register
avoids clearing the COUNTFLAG bit. The register is set to have bits 1
and 2 set, and bit 0 clear, just as per the old code (?). So the net
result should not be different between the new and old, other than the
fact that this is now happening after interrupts have been disabled
rather than before.

The if() condition then simultaneously tests and, because it reads the
portNVIC_SYSTICK_CTRL_REG register, clears the COUNTFLAG bit.

Furthermore you adapted the order change that I performed - the first
instruction you mention is the enable interrupt statement. In the
old code the instruction was performed after the clock was put into
suspension state. Has that been put into consideration?

Repeating myself just for emphasis - yes it has been considered and
analysis (but not experimentation) suggests that the above code change
will be ok. This would need more testing though.

What I understood from your reply was that it might be unsafe to read
the complete control register as we might remove the count flag. In
the CM0 port the control register is read and saved for this case at
the very beginning. So the first statment is |ulSysTickCTRL =
portNVIC_SYSTICK_CTRL;| followed by the changes to the control
register. All further calculations are so based on the here saved
value. If I’m not mistaken your implementation is based on the actual
control register and it is not saved before. For example the check of
the count flag is directly performed on the register - in the CM0
port the saved value is used: |if( ( portNVIC_SYSTICK_CTRL_REG &
portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 ) {| vs. CM0 implementaion:
|if( ( ulSysTickCTRL & portNVIC_SYSTICK_COUNT_FLAG ) != 0 )|

So I think the issue you targeted with your change might not apply to
the CM0 port.

Your interpretation of how the code change is reading the count flag is
correct, but I don’t fully understand the point you are making at the
end of this paragraph. As far as I know it shouldn’t make any
difference if this is running on the M0, M3, M4F or M7. Does my
explanation above help our mutual understanding?

system · November 23, 2016, 8:33am

embeddedoli wrote on Wednesday, November 23, 2016:

Hello again,
I think now I understand what your intention was. From my perspective we both do the same now. Save the count flag -> Stop the systick by removing the enable flag -> Check all other conditions.

The order change was applied and tested by us. It seem to work fine. So if you do not see any negative side effects by now we will keep the change unless we recognize other impacts.

So just as a final statement, can this be considered a bug and we have been the first that found it or is that some kind of unexpected behaviour we implemented? I made a small research and think others might have the same issue but used the missed_counts_factor to comprise the missed interrupts (e.g. https://sourceforge.net/p/freertos/discussion/382005/thread/dac50e55/)

Furthermore taken from the CM3 port.c line 501:

portNVIC_SYSTICK_LOAD = ( ( ulCompleteTickPeriods + 1 ) * ulTimerCountsForOneTick ) - ulCompletedSysTickDecrements;
			}

			/* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG
			again, then set portNVIC_SYSTICK_LOAD_REG back to its standard
			value.  The critical section is used to ensure the tick interrupt
			can only execute once in the case that the reload register is near
			zero. */
			portNVIC_SYSTICK_CURRENT_VALUE = 0UL;
			portENTER_CRITICAL();
			{
				portNVIC_SYSTICK_CTRL |= portNVIC_SYSTICK_ENABLE;
				vTaskStepTick( ulCompleteTickPeriods );
				portNVIC_SYSTICK_LOAD = ulTimerCountsForOneTick - 1UL;
			}
			portEXIT_CRITICAL();

There is some issue here with the portNVIC_SYSTICK_LOAD value. It is first calulated and then overwritten with a default value in the critical section! Should I open an other topic for this? This looks quite obvious as a bug.

And sorry for the confusion regarding the CM0 port. CubeMX seems to implement a port.c from any other CM type than the CM0, as you do not provide tickless as you mentioned! (I’ve just looked into the 9.0 release and did not found one in the CM0 port )

Best regards,
Oliver

rtel · November 23, 2016, 2:25pm

rtel wrote on Wednesday, November 23, 2016:

portNVIC_SYSTICK_LOAD = ( ( ulCompleteTickPeriods + 1 ) * ulTimerCountsForOneTick ) - ulCompletedSysTickDecrements;

This sets the portNVIC_SYSTICK_LOAD value to be correct for the next
[adjusted] period which brings the tick interrupt back in line with the
regular frequency after the tick has been stopped.

}

/* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG
again, then set portNVIC_SYSTICK_LOAD_REG back to its standard
value. The critical section is used to ensure the tick interrupt
can only execute once in the case that the reload register is near
zero. */
portNVIC_SYSTICK_CURRENT_VALUE = 0UL;
portENTER_CRITICAL();
{
portNVIC_SYSTICK_CTRL |= portNVIC_SYSTICK_ENABLE;

This starts the tick again, which starts from the value loaded into the
portNVIC_SYSTICK_LOAD register above. The SysTick is now running and
the portNVIC_SYSTICK_LOAD register value is not used until the SysTick
count reaches zero.

vTaskStepTick( ulCompleteTickPeriods );
portNVIC_SYSTICK_LOAD = ulTimerCountsForOneTick - 1UL;

Now the SysTick clock is running, the portNVIC_SYSTICK_LOAD register is
updated again so the next time the SysTick clock reaches zero it is
reloaded with the value necessary to create the normal tick frequency.

system · November 24, 2016, 7:30am

embeddedoli wrote on Thursday, November 24, 2016:

Edit:
Okay, after further investigation I guess the systick load is immediately taken for the immediate reload that happens as soon as the timer is enabled again and the “next” reload value is loaded to be used as the default value for future ticks…In this case the code looks ok.

Obsolete:
Well that’s the point:
First the register is loaded with the correct adjusted value:

portNVIC_SYSTICK_LOAD = ( ( ulCompleteTickPeriods + 1 ) * ulTimerCountsForOneTick ) - ulCompletedSysTickDecrements;

and right after that it is overwritten with the value for a normal tick

portNVIC_SYSTICK_LOAD = ulTimerCountsForOneTick - 1UL;

As a result the corrected and adjusted value is lost and never put into consideration…

rtel · November 24, 2016, 7:13pm

rtel wrote on Thursday, November 24, 2016:

I think the bit you are missing is this from my previous email “This starts the tick again, which starts from the value loaded into the portNVIC_SYSTICK_LOAD register above.”. So the adjusted value IS used before it gets overwritten. When the timer starts, it starts from the value in the portNVIC_SYSTICK_LOAD regster, and only then is it overwritten (it is overwritten AFTER the timer has started, and therefore AFTER the adjusted value has been used).

Does that make sense? Let me know if you think I am wrong on this.

system · November 25, 2016, 12:39pm

embeddedoli wrote on Friday, November 25, 2016:

No, makes perfect sense and looks good to me. So I dont have any further questions regarding this issue. Thanks for your help. I guess the issue with the changed order of interrupt activation might be an issue for other people aswell but the other thing seems fine.

Best regards,
Oliver

system · November 28, 2016, 4:47pm

janoshnosh wrote on Monday, November 28, 2016:

Hello,
I have a follow-up question on this topic, perhaps you can give advise on it too.
As I see it, the tickless-idle mode after sleeping acts like this:

We wake up because of an interrupt (the systick timer is running)
We turn on interrupts to immediately serve them (the systick timer is still running)
We want to calculate the next tick-time. For this case we stop the systick timer in order to calculate it. (the systick timer is halted)
We set the systick timer interrupt for the calculated value (the systick timer is running).

But now in point 3. i see the problem, that again an interrupt could kick in and would be served immediately - with a halted systick timer. Isn’t this a problem?
Why isn’t the critical section extended to the whole time where the systick timer is halted? Or even simpler, why aren’t the interrupts only enabled after the re-setting of the systick timer?

rtel · November 28, 2016, 5:47pm

rtel wrote on Monday, November 28, 2016:

Again I will have to look at this in some detail, with the code in front
of me, to give a proper answer so will get back to you after that.

system · December 2, 2016, 2:12pm

janoshnosh wrote on Friday, December 02, 2016:

Hello, do you have any update on this?
Thanks and have a good weekend.
Cheers Janos

rtel · December 2, 2016, 4:09pm

rtel wrote on Friday, December 02, 2016:

Not yet - but will do.

system · December 15, 2016, 10:46am

janoshnosh wrote on Thursday, December 15, 2016:

Hello again,
are there any news here?
Can I ask an intermediate question to keep the thread running: Who is actually providing the ARM CM0 port for FreeRTOS and the tickless idle mode? Is it developed by FreeRTOS developers, or by specific ARM vendors?

rtel · December 15, 2016, 5:47pm

rtel wrote on Thursday, December 15, 2016:

Sorry not to get back yet.

We provide the tickless mode for M3/M4F/M7 parts. I’m not sure if we
have an example for M0 too, but I know people are using the exact same
implementation on M0 parts.

system · January 9, 2017, 10:29am

janoshnosh wrote on Monday, January 09, 2017:

Hello and a good year to you all.
Have you had the chance to look at this issue yet?

rtel · January 9, 2017, 9:21pm

rtel wrote on Monday, January 09, 2017:

Not yet - but will do - I need to roll the changes into all the Cortex-M
ports and re-test so it will get done. Sorry to not be more specific as
to when but things are VERY busy at the moment.