STM32F1: Fix stepper timer issue causing motor shocks #14030
Conversation
|
We really try and keep all architecture differences behind the HAL otherwise things will get out of hand quickly, one |
|
the problem is HAL_timer_set_compare is also used (i think, will double check) |
|
no in fact its doable, only for the stepper |
|
@p3p it is not equivalent! As one is immediate and the reload is not immediate. That is the only work around we so far found to get the timer to behave normally |
|
erk no i was right, its also used on the ISR start |
|
I know its not functionally the same and why you needed the change, I've been reading along to your trial and tribulations, I'm just here looking after the abstraction layer ;) and to the stepper code it is equivalent in function.. set the time to the next isr call. Things are never generic enough to cover all platforms, if we have to we may need to add a new api.. |
|
we need both HAL_timer_set_compare and set_reload to fix it |
You may do but we can't break the abstraction layer for every platform that has some unique requirement it defeats the point, if you can't find a way to hide this workaround behind the |
|
How about adding a boolean parameter to Or add a |
|
Would be nice to see if the F4 & F7 need something like that too... |
|
I've been following the skipping steps thread, but I'm still not totally sure I understand this fix. So my understanding is that you are using a "reload" register to set the timer when used by the stepper code. This in effect delays the stepper time value by 1 cycle. Is that correct? What will the impact of doing that be on the stepper timing, does it matter? I assume the problem is that you do not want to use this mode for all timer settings because of the extra cycle delay? Which is why you want to add a new entry point? I seem to remember that the stepper code deliberately sets a long time period at the start of the stepper ISR to prevent a second timer interrupt occurring during the isr. What will be the impact on that code of this change? Could that code trigger any problem here? Do we understand exactly why we are seeing this "long pulse" problem and in particular why it only happens from time to time? What are the exact circumstances that cause it to trigger? I wonder if it might be possible to set things up so that you can always use the reload option, is there anyway to say set the reload register and then force a timer event and hence the loading of the reload value? You would of course have to stop an isr being called for this "extra" event. I think on the LPC176x we use a mode that zeros the timer count when a match is triggered. Is the same mode possible with the STM and are we using it? |
|
basically, if all was running correctly, the reload countdown should never be triggered (its a timer mode which redo automatically the call on expiration). |
|
rmm if you really dont want this stepper ifdef Not proper neither, but unsure what is... |
|
You get it fully right. We effectively delay all timer reloads by one isr. |
|
Here’s an update to update you on the updated status of the update. sorry.. its just my normal life quote... else i updated the code and should not affect others by a dangerous ifdef :p |
I don't mean to seem pernickety, but it is important to keep the |
|
its fine, its just to avoid to waste more time on that.... @hobiseven was talking about changing the first call... and cant be |
|
I'm confused, with that latest change, you will set a long time period at the start of the ISR, and that gets set directly, but then later on you set the actual time using the reload mechanism, but doesn't that second value only get "made active" when the current long time period expires? Or have I totally misunderstood how this works? |
|
This would be correct except that : libmaple sets ARPE flag to 1 which implies using reload method and not direct one.
In any case I do not understand what it protects against. If we take more time than the next pulse time we start to introduce jitter. |
|
first one set the timer channel, even if we have plenty on the SoC |
|
@p3p something I'm not sure about are these timers one shot or repeating? I guess if they are repeating then the first call is there to stop any chance that a repeating timer will cause a second interrupt while still processing the first. |
|
Indeed, the first call is just to guarantee an interrupt isn't queued while in the ISR because (as you said earlier) for precise timing the clock is never stopped, just reset to 0 on match (on LPC176x anyway). The second call is the only one that should be required really, not sure why it was decided to set the match to max timer_t rather than disable that timers interrupt on match until it was updated to the new value. |
|
The platforms can do whatever they need to with interrupts and timers etc using |
|
I was the "culprit" of that implementatiin. The HAL assumes timers are cleared on match and that they will trigger again an ISR if they match again. That is the reason why the compare is set to the maximum value at the start of the stepper ISR. We can't stop the timer because it would lose timing precision as we need the ISR execution time to also be taken into account. I am absolutely sure STM32 timers can also be used in that exact mode because i am using them in a STM32f103 board i have for a different project i am working right now and they work beautifully in that mode. |
|
Hmmmm ok. Which exact version of stm32 do you use please? We use stm32f103vet6 As I have not played at all with interrupts code before getting that issue I wonder what goes on. I took a fresh copy of marlin and plugged the fsmc code not using any interrupt and I have a sw spi touchscreen pooled ( hw cannot be used) I will try to get a genuine stm32 board from stm. And I will flash the code on that one to check if the problem is here too. |
|
@pinches |
|
@hobiseven : I am using the exact same version as yours. I´d suggest, using a JTAG debugger, or just by dumping NVIC settings, to make sure NVIC interrupt priorities are properly setup. |
|
@ejtagle so doesn't "CPSID i" disable all interrupts on a processor? |
|
@gloomyandy : ARM interrupts do not work as 8bit (AVR) ISRs do. Quoting the ARM manual: (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html) ... "If an interrupt was already in the pending state, the processor accepts the interrupt after the “CPSIE I” is executed. However, additional instructions can be executed before the processor enters the exception handler"... So, the "disabling" of interrupts is not inmediate disabling as it is on 8bit processors. All interrupt requests that were already accepted by NVIC, but not processed yet (so, the associated ISR code has not executed yet) WILL BE executed EVEN if the __disable_irq()/cpsid instruction was executed. The logic behind this is a little bit complex... My best advice here is NEVER to depend on interrupts being disabled for some code to work, and if you depend on such behaviour, then there are several extra steps required to ensure no interrupt will execute after a CPSID instruction is executed (i already implemented those steps probably for SAM (Arduino Due) and LPC (Freescale) processors. Marlin code, as it is, does NOT depend on interrupts being disabled. I did a lot of work on the USART/Stepper/temperature ISRs to make sure there is no such dependency. Proper NVIC ISR priority configuration is REQUIRED. IT IS NOT OPTATIVE, IT IS A MUST. The reason is that otherwise, as NVIC allows interrupt handlers to be preempted (= interrupts the execution of a given interrupt handler code to execute a higher priority interrupt handler), timing is just plainly DESTROYED. The Stepper ISR MUST , MUST BE the ISR with the highest priority, or there will be no way to ensure reliable programming of the timer so next step pulse will happen at the right time. I can point exactly to the place of the code that has that critical requirement |
|
@ejtagle |
|
Humm... Let me reread my own notes... I seem to recall something... Direct reload failed... This seems to make me remember something... |
|
Can't you signal to generate an update on reload? I seem to recall the original STM32F1 HAL doing that. I'm experimenting with using priority mask rather than interrupt disabling in order to keep SDIO/DMA/USB/Serial running, but it won't fix this issue. |
|
@hobiseven : I think the problem is the ARPE=1. I know it sounds counterintuitive. Let me explain why using ARPE=0 is causing issues, and how to fix them, and why ARPE=1 is a bad idea. ARPE=0 has problems, just because the timer is never stopped. And that is fine! ... But, assume the ISR is happening too fast (perfectly possible, as the period of the timer is NOT constant, and can wildly vary, as the stepper timer is, in fact, used to perform at least 2 periodic tasks (there is a primitive scheduler implemented in lines 1273 to 1310. The period to the next interrupt is computed based on the requirements of Linear Advance and the stepper itself. But at some point, perhaps, the estimated time of next firing has already passed when the compare register is reloaded. And a full timer rollover will have to happen (16ms) for the next ISR to fire. Lines stepper.cpp:1348/1360 add a "margin"... That margin is how much time we want to leave between ISRs, but also serve as an estimation on how much time the TIMR module requires for a new COMPARE value to be set before the counter value reaches the compare value. So, as a first measure, i would increase it to 2 (2 microseconds) and keep ARRE=0 (ARRE=1 is a bad idea, as the shadow register will only be loaded with the compare value when a match between the old compare value and the timer happens, causing horrible mistimings... |
|
@ejtagle can you provide a reference that describes the exact behaviour of the NVIC/cpsid combination I'd like to understand it better. I've read the link you provided (and have read it before), but that describes what happens when you enable interrupts, not when you disable them using CPSID i. In the same doc there is no mention of interrupt code executing after the execution of CPSID i.
Have I misunderstood? It seems strange that they would not mention that interrupts can be called after the CPSID instruction given the amount of detail provided about what happens following the enable. Is there some confusion here between using the NVIC_DisableIRQ call and using CPSID? My understanding is that in this case a barrier is required (see: https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the). The reason I ask is that on the LPC176x processors the stepper timer runs at priority 1 and there are other interrupts (SysTick possibly others) that run at priority 0 and so can pre-empt the stepper driver. With the code at line 1345 and onwards if CPSID i does not disable higher priority interrupts and take immediate effect then there is a danger that the code that follows the DISABLE_ISRS() which samples the current timer value could be pre-empted and thus add jitter into the calculation. Though perhaps that jitter is so small it will not matter? Just to be clear I understand your points about correctly using the priority settings within the NVIC to handle all of this. I just want to make sure that my own understanding of how this works is correct? |
|
@ejtagle Regarding the indirect/ buffered reload it is working very nicely so far , and I was thinking to also buffer the stepper commands by one isr to realign everything . But if you say this is horrible... |
|
@gloomyandy : Yes, the ARM manual does not state there are issues, but if you read the example of enabling interrupts, then you get the problem on disabling interrupts... IDK why they documented it so confusingly... The "margin" i talk about should also consider the time spent in higher priority interrupts, if there are any. SysTick usually is just incrementing a counter, so 1uS should be more than enough. But yes, if we fail to properly guess, then problems will happen... |
|
@hobiseven : There is already code to properly handle not having a shadow register. The shadow register exists to avoid glitchs in applications where a glitch is not admisible, even at the cost of incorrect timing. In our case, we prefer perfect timing (if possible) |
|
@ejtagle Perhaps we should take this discussion elsewhere (discord?)? Maybe I'm just being dim but I don't see anything in the enable discussion or examples (that are pretty clear), that shows that interrupts can be called following CPSID. They do show that interrupts become pending, but that is not the same as calling them. All of the examples seem to show how a pending interrupt may be delayed by a few instructions after an enable (and steps to take to ensure that it will be called before another disable). I've also spent some time trying to find other sources of documentation that give details of how CPSID operates and I can't find any mention of it allowing interrupt code to be called after the CPSID instruction has been executed. Do you have any references? But anyway I've disrupted this discussion enough with this, but I would like to get a final answer to it as it seems like a pretty crucial operation! |
|
What is the current consensus on this PR? Does it still need more work before it will solve the issue properly, or is it okay as a workaround for now? |
|
rebased after some cleanup |
|
Fix fully ok now. And does what the initial code intended to do. No more isr cycle delay. |
|
this being pushed soon? Both myself and my testers are very satisfied with the results. |
|
Yeah, I'm interested to see if this fixes any issues with the two stm32-based boards I'm tinkering around with. |
+ allow timer 8 in HAL and clean outdated comments/config Without the timer reload feature, some stepper ISR could be skipped which leads into a small time break in stepper pulses (around 16ms) and can cause layer shifting if the bed is heavy. To explain easily the behavoir, its like a car driver hiting a wall.
thinkyhead
force-pushed
the
stm32-stepper
branch
from
b3190f0
to
7f71a39
Compare
Without the timer reload feature, some isrs are incorrectly triggered,
only after a full timer counter rollover.
That leads into a small time break in stepper pulses (around 16ms)
and can cause layer shifting if the bed is heavy.
To explain easily the behavoir, its like a car driver hiting a wall.
To avoid timer inconsistent behavior firing interrupt on current cycle or next cycle,
we trigger it always on next cycle.
Y/bed axis issue is more easy to see due to the bed weight,
but this issue was affecting all steppers the same way.
extras: