Add Emergency Parser support for STM32

[rudihorn]

Requirements

• The goal is to get emergency parser working on STM32.

Description

This is some initial work that gets the emergency parser function to work on STM32. This requires patching of both the USB and the serial parsers.

Currently I have the serial emergency functionality working. This is done by creating a MarlinSerial class, which hooks the interrupt handler to a custom defined handler that feeds the information into the emergency parser.

USB emergency parser is now also working. See comment below.

Benefits

Emergency functionality.

Related Issues

#17705

[rudihorn]

Some progress on the USB side. STM32Duino offers a hook for DTR toggling (https://github.com/stm32duino/Arduino_Core_STM32/blob/master/cores/arduino/hooks.c). Essentially this allows one to define a function:

void dtr_togglingHook(uint8_t *buf, uint32_t *len) { /* ... */ }

Which the user can then implement. It requires the DTR_TOGGLING_SEQ compiler flag to be set. I have just committed this and pushed it to the pull request, though I still need to add the DTR_TOGGLING_SEQ flag to PIO. Unfortunately the current platform io file makes this a big change, so I would suggest adding a section:

[ststm32-common]
platform = ststm32
build_flags_usb = -DUSBCON -DDTR_TOGGLING_SEQ ...

and then for each stm targeting board it is possible to import them:

[env:BIGTREE_SKR_PRO]
build_flags = ${common.build_flags} ${ststm32-common.build_flags_usb}

it would still be a big change to the file though.

[rudihorn]

I have noticed that M108 behaves seemingly fishy though. It should immediately stop a waiting heat, but I'm not sure it always works. M112 seemed to work better though, so it might be in the heating code.

Update: This seems to be a combination of issues with multiple things on my end. As far as I can tell the BTT TFT 35 I have synchronously sends M105 commands in a queue, and relies on ok responses before sending the next command. Sending M108/M112 quickly before the TFT is able to send a M105 seems to solve this problem. Sending a M112 in octoprint causes octoprint to kill the connection, but the emergency stop command is not actually processed, so seemingly USB is not fixed yet.

[rudihorn]

Okay I've just committed a new approach which hooks the USBD_CDC_Receive_hook function. This is done by swapping out the Receive function from the USB interface USB_CDC_ItfTypeDef.

The hooked function feeds the buffer into the emergency parser, and then calls the original hook function as follows:

static int8_t USBD_CDC_Receive_hook(uint8_t *Buf, uint32_t *Len) {
  for(uint32_t i = 0; i < *Len; i++) 
    emergency_parser.update(emergency_state, Buf[i]);
  return USBD_CDC_Receive_original(Buf, Len);
}

This is tested and works well over USB.

Is anyone able to test this properly with UART (e.g. with a screen that properly supports emergency stop)?

[GMagician]

@rudihorn I' would like to do something similar for AGCM4, are all those functions hooks given by framework or are you doing something different? unluckly Adafruit don't want to add a virtual call to uart and I don't think they want to do something heavier on usb side....

[sjasonsmith]

so I would suggest adding a section:

@rudihorn, your platformio.ini suggestions overlap with what I have in progress in two separate PRs. I've been playing with this and thing there is a lot to gain from something like you descibed.

My first revision of this was tied to upgrading the framework used by HAL/STM32. That review is stalled out at the moment, and isn't likely to go in right away. It won't go in until we either get a 1.9 framework through PlatformIO or some I2C issues get figured out in 1.8. At this point I would want to change the way I modified platformio.ini to match what I did below for HAL/STM32F1.
#17970

My latest revision is only for STM32F1 environments, but seems to be a better way of doing things. I'm currently waiting for feedback on that PR. If it goes in, it can that serve as a template for greater re-use in the platformio.ini. I don't want to spend more time applying it to other environments until @thinkyhead provides input on my current STM32F1 implementation.
#18099

[rudihorn]

@GMagician sadly and quite annoyingly it is not given by the framework, and especially the USB one is a little bit hacky. These things should be provided by the framework, and if you could get them to add it by submitting a PR that would be best.

With that platform you aren't relying on adafruit, you can simply submit a PR to https://github.com/arduino/ArduinoCore-sam I believe, though you can try getting away without changing it like I did. For your platform you would probably want to override the IrqHandler function (https://github.com/arduino/ArduinoCore-sam/blob/eed66e7977b4037f49bbcd832437d4bcf72c0f3f/cores/arduino/UARTClass.cpp#L169) by defining a custom class. USB you would need to look around as they are all very different. In general you want to try to find the read interrupt which writes any incoming USB CDC packets into a buffer, before the serial->read() function is called.

[rudihorn]

@sjasonsmith it luckily turns out that I won't need to add a new flag after all as the DTR thing never worked in any case. I do however support such a change.

[GMagician]

With that platform you aren't relying on adafruit

not so sure: C:\Users\xxx\.platformio\packages\framework-arduino-samd-adafruit

and Adafruit will not help on this.

your platform you would probably want to override the IrqHandler

for serial in stm32 I saw the virtual function into uart code. It's called by framework and it return tru or false if char has to be stored in buffer or not. Hence just deriving serial class is enought. Don't know what about usb, but not so easy

[rudihorn]

@GMagician could you create a seperate issue for your thing and then we can discuss there?

[thisiskeithb]

How much work is it to get this working on SMT32F1 and STM32F4?

[rudihorn]

@thisiskeithb Is there any need for these two platforms as STM32 generic supports both?

[thisiskeithb]

@thisiskeithb Is there any need for these two platforms as STM32 generic supports both?

I assumed it wasn't supported yet for STM32F1/F4 since the sanity checks for those HALs weren't updated in this PR.

Marlin/Marlin/src/HAL/STM32F1/inc/SanityCheck.h

Lines 28 to 30 in 6c99400

	#if ENABLED(EMERGENCY_PARSER)
	#error "EMERGENCY_PARSER is not yet implemented for STM32F1. Disable EMERGENCY_PARSER to continue."
	#endif

Marlin/Marlin/src/HAL/STM32_F4_F7/inc/SanityCheck.h

Lines 31 to 33 in 6c99400

	#if ENABLED(EMERGENCY_PARSER)
	#error "EMERGENCY_PARSER is not yet implemented for STM32F4/7. Disable EMERGENCY_PARSER to continue."
	#endif

[rudihorn]

@thisiskeithb It is a bit confusing, because there are two different platform packages that can be used. I am using a BTT Pro V1.1 which has an STM32F4 chip, but uses https://github.com/stm32duino/Arduino_Core_STM32 as its platform.

There are some boards which rely on https://github.com/rogerclarkmelbourne/Arduino_STM32, and this chip introduces the different HAL's for STM32F4_7 and STM32F1. I'm asking why these other HALs exist when the arduino core stm32 platform already targets both (and more!)?

[thisiskeithb]

I'm asking why these other HALs exist when the arduino core stm32 platform already targets both (and more!)?

You'll have to ask @thinkyhead as that's above my paygrade 🙂

[sjasonsmith]

@rudihorn i think the F1/F4 HALs simply existed first.

I don’t know if anything actually uses the F4 HAL. The goal should be to eliminate that one rather than improve it.

As for these F1 HAL, many boards use it. Eliminating it—while possible—would likely be more controversial. That doesn’t mean it would be wrong, just that more care would be needed to ensure it wasn’t a feature or performance downgrade.

[AnHardt]

The different frameworks/kernels supporting STM32 have different degrees of maturity.

The Maple based https://github.com/rogerclarkmelbourne/Arduino_STM32 is the most mature if you have to deal with STM32F103 and similar but was never near to finished for the F4 processors and does not support other processors. This is still maintained, but the speed of development decreased over the last years.

The 'official' https://github.com/stm32duino/Arduino_Core_STM32 is the newest, supporting all STM32 processors. We still miss some features like a 'USB-composite device for mas-storage and serial at the same time'. Also Marlins HAL-layer for this is far from perfect - it just works (mostly) (awful ADC handling, ...). But this is the future because it is constantly developed. Not as fast as we want to - because they only have one payed full time developer for that.

Please don't use the adjective 'generic' if you try to talk about https://github.com/stm32duino/Arduino_Core_STM32.
STM32GENERIC https://github.com/danieleff/STM32GENERIC is a third pair of shoes. We used that for a few boards but have (hopefully) dropped that now.

To make it short.
If you want to explain to the users why several features, now working on the STM32F1xx processor-boards, are not working anymore because we dropped the 'Maple' kernel in favor for the 'official' kernel. Let's do that. It will only last some years until we will have them back.
If the F4-part of the 'Maple'-kernel is in use for some board is indeed questionable.

[GMagician]

@rudihorn

@GMagician could you create a seperate issue for your thing and then we can discuss there?

I think I've no wayout. I checked your code and you can do what you did because of hooks/callback handled by framework. In my case I don't have any and worst, AGCM4 is samd not sam. Arduino IDE (if I didn't downloaded the wrong package) is using Adafruit framework (more recent than platformio but the same source) so...With no Adafruit help I can't do anything (interrupt handler for serial1 linked by framework and there is no way, that I know, to override it)

[Haschtl]

There seems to be one missing line.

I get the compile-error 'MSerial6' was not declared in this scope because MSerial6 is not defined.

I don't know if this was intentional or by mistake, but adding the line extern MarlinSerial MSerial6; at the bottom of the MarlinSerial.h fixes the compile-error (but I haven't checked it yet, because I mostly don't use it). I have an SKR Pro 1.1 and an Esp8266 with Esp3D connected to serial port 6.

[rudihorn]

@sjasonsmith @AnHardt
Thanks for the explanation, I assumed there must be some things which aren't working as well. Are there any issues tracking which features aren't working too well in the STM32 HAL? I might be open to trying to get some of those implemented. Also if there are specifics on the issue with ADC I would be interested to know more on this.

@GMagician you could try submitting PR requests to adafruit https://github.com/adafruit/ArduinoCore-samd.

@Haschtl this is a bug, best to submit a PR for this.

[AnHardt]

Off topic: Why is the current implementation of ADC handling in the STM32-HAL crap.

Arduino ADC-handling is usually done with analogRead().
Arduino anologRead() does several things. It looks up and configures the ADC-registers and pins, starts a conversion, busy waits until the conversion is done and finally returns the value.
Marlin instead splits this in tree phases. Configure, start, take the value. The registers are configured only once at startup. For reading a value there are two phases. Start the conversion and pick up the result. This mainly avoids the busy waiting for the result. In that time we do better things than waiting.
The STM32-Hal is doing it about like this: During the setup phase it does about nothing. In the start conversion phase it executes a analogRead() and stores the result in a temporary variable. In the third phase it picks up the stored value from phase two. Worst of all - analogRead() from STM32 additionally calibrates the ADC every time, resulting in a additional busy delay compared to the AVRs analogRead(). On a 168MHz STMM32F4 the analogRead is about twice as fast as on a AVR at 16MHz - but when splitting the analogRead(), removing the busy waits, it is at least 10 times faster.

Even if calling analogRead() should be currently unavoidable, it would better be done in phase tree, avoiding the additional variable.

The current implementation completely fails Marlins intention to avoid analogRead() to save time.

A good implementation would set up the ADCs/ADC-channels and start DMA to store the results per channel/pin in an array. Phase two would do noting. Phase three would just pick up the value from the array. Since DMA is done in the background the only remaining load for the processor would be picking up a value at any timer. That should be magnitudes faster than what we do with the AVRs not 5 to 10 times slower like that what we do now.

[casterle]

I apparently need to enable EMERGENCY_PARSER to get ADVANCED_PAUSE_FEATURE working. I downloaded the latest bugfix version today, and am still unable to build because "EMERGENCY_PARSER is not yet implemented for STM32F1."

Is the implementation for my hardware still under development?

I'm building for an Ender-3 V2 with an SKR Mini E3 V2.0. I've got both the stock V2 display as well as a BTT TFT 3.5 E3 V3.0. Is either of these a "supported LCD controller" so I don't need the EMERGENCY_PARSER?

If I enable DWIN_CREALITY_LCD (for the V2's display, which is different from previous enders) the build fails with undefined identifiers such as "'buzzer' was not declared in this scope".

[fengbaochun]

After I use the emergency stop command (M410), I must restart the machine to send the command again

[thisiskeithb]

After I use the emergency stop command (M410), I must restart the machine to send the command again

You should use M112 as that's Emergency Stop. M410 is Quick Stop, which is different.

[fengbaochun]

After I use the emergency stop command (M410), I must restart the machine to send the command again

You should use M112 as that's Emergency Stop. M410 is Quick Stop, which is different.

Then after using M410 emergency stop, what command can be used to restore the state before the stop again?

[schlaegerz]

I am having the same problem as @fengbaochun

When I call M410 I expect to be able to continue to send commands, but it appears marlin either crashes or hangs and I need to reset the board.