Various Small Fixes to F4/7 HALs

[SJ-Innovation]

Description

This is one in a series of PRs that will be the foundation of a work in progress 32 bit controller. A few fixes needed to be made so it would compile though namely the ISR enablers and disablers in the F4/7 HALs, as well as a few small typos here and there.

Benefits

Now compiles on STM32F4 and F7.

First public contribution, suggestions and comments would be lovely, as well as tips for the future.

[thinkyhead]

@xC0000005, @hasenbanck —

Does this set of changes fit in with your work?

[xC0000005]

Is there a reason you're disabling all interrupts this way? The previous way leaves other ISRs intact, this does not.

[thinkyhead]

@xC0000005 — Can you point out the problem lines under the Files tab?

[hasenbanck]

@thinkyhead — When I read the change correctly, in line 115 and 116 of the STM32F7/HAL.h we are already defining the macros for ENABLE_ISRS and DISABLE_ISRS, where we are disabling all interrupts toggling the I-bit in the CPSR.

With this change, we would only disable / enable the temp/stepper interrupts. Since the Arduino core is enabling ISRS for other timers (for example servo), I think it was changed by the original author to handle all interrupts.

From my understanding, these macros are used for critical code paths to not be interrupted by interrupts. What is the intended mode of operation for these macros? The F4 HAL currently seems to use your proposed change.

EDIT: Ah I didn't look hard enough. The F4 HAL is also changed. Ignore my last sentence.

[xC0000005]

I think I misunderstood what was being done - there probably is not a problem with this.

…

On Sep 8, 2018, at 10:16 PM, Scott Lahteine ***@***.***> wrote: @xC0000005 <https://github.com/xC0000005> — Can you point out the problem lines under the Files tab? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aeppca45HC9IvHwFZq7HbZ2e2OXkmNLOks5uZKQtgaJpZM4Wf7DC>.

[thinkyhead]

So I assume that if the changes to the two HAL.h files (double-defining ENABLE_ISRS/DISABLE_ISRS) are removed, the rest should be ok.

[thinkyhead]

@xC0000005 — You're right. Take STM32F7/HAL.h for example. It has this already…

#define ENABLE_ISRS()  __enable_irq()
#define DISABLE_ISRS() __disable_irq()

…and this PR redefines them further down as this…

#define DISABLE_ISRS() cli()
#define ENABLE_ISRS() sei()

Clearly they can't be both!

[hasenbanck]

@thinkyhead I think that

#define ENABLE_ISRS()  __enable_irq()
#define DISABLE_ISRS() __disable_irq()

is the correct version of the two.

I'm also pretty sure that cli() and sei() should be changed to __enable_irq() / __disable_irq().

#define cli()  __enable_irq()
#define sei() __disable_irq()

Their only usage of cli() in Marlin currently are right now in softspi.h, the kill() function in Marlin.ccp and the Stepper::babystep() function in the stepper.cpp. The babystep() function at least states that NO interrupt should ever interrupt that function, so disabling all interrupts should be the intended behavior.

But I haven't tested this change yet (I use TMC2130 in SPI mode).

In the long run it might be wise to change all cli() and sei() to the ENABLE_ISRS()/DISABLE_ISRS() macros anyhow?

[SJ-Innovation]

Hi guys, the changes proposed with this do in fact overwrite some things that i thought they didnt. I think ive done goofed with my git client and left out a commit or two that adds those changes.

The version of the HAL that i was developing with didnt even have those ENABLE / DISABLE macros, hense why i added them. If they were already there i wouldnt have redefined them xD Im not that stupid... Though evidently I cant use git... Ever so sorry for the confusion that its caused.

[SJ-Innovation]

Corrected the SEI CLI screwup, sorry again. Also changed for __enable_irq and __disable_irq.
Would there be any harm in changing all references of cli and sei for the ENABLE / DISABLE_ISRS macro as @hasenbanck suggests? (Seeing as theres only two of them) Or would it be better to keep it for back-compatibility sake?