[BUG] M42 seems broken on stm32 architecture

[Angelic47]

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

M42 - Set Pin State, seems broken on stm32 architecture and produce unexpected results.
Recently I have been trying to use M42 to control a relay on my machine,
which connected to pin PE10 on my MKS Monster 8 board (it's actually a STM32F407VET6 board).

By checking M43 and STM32 HAL header, I found that PE10 is mapped to pin 74 on stm32:

#define PE10 0x4A

According to the M42 documentation, I should be able to control the pin by sending M42 P74 S1 or M42 P74 S0 to turn it HIGH or LOW.
However, it seems that the pin is producing unexpected behavior (most time it's LOW, but sometimes it's HIGH).

I also tried to use M42 I P74 S1 T1, which I means "ignore protection on pins", and T1 means "set the pin mode to OUTPUT". Still, the pin is not working as expected.
I even tried to use M42 I P74 S255 T1 to set the pin to PWM 255 (which should be HIGH), but it's still no luck.
All other pins on my board are not working as well.

For some deep dive, I found that M43 P74 reports something like this:
PIN: 74 PE10 Output HIGH

But after I send M42 P74 S1, it reports:
PIN: 74 PE10 Alt Function: 0 - system (misc. I/O)

This makes me believe that there could be somethings wrong with M42 gcode implementation on stm32 architecture,
seems gpio alt function is triggered by M42 command accidentally.

After some research, I found Marlin/src/gcode/control/M42.cpp#L106-L112 is the place where the pin is set to OUTPUT mode:

  // ...
  extDigitalWrite(pin, pin_status);

  #ifdef ARDUINO_ARCH_STM32
    // A simple I/O will be set to 0 by hal.set_pwm_duty()
    if (pin_status <= 1 && !PWM_PIN(pin)) return;
  #endif
  hal.set_pwm_duty(pin, pin_status);
  // ...

This codes checks if the pin is a PWM pin, then set the PWM duty to this pin, or just skipped the PWM duty setting.
It seems there is no problem with this code, but actually, seems all pins are treated as PWM pins on stm32 architecture, whatever the pin is a PWM pin or not.

I tried to add some debug code to #ifndef ARDUINO_ARCH_STM32 and #endif block, to check that macro is working or not. Like this:

  // ...
  extDigitalWrite(pin, pin_status);

  #ifdef ARDUINO_ARCH_STM32
    #error test test test
    // A simple I/O will be set to 0 by hal.set_pwm_duty()
    if (pin_status <= 1 && !PWM_PIN(pin)) return;
  #endif
  hal.set_pwm_duty(pin, pin_status);
  // ...

And I found that the #error is triggered, which means the #ifdef ARDUINO_ARCH_STM32 is working as expected.

Now things become more interesting, does this mean that the PWM_PIN(pin) macro is not working as expected?
To check this, I added some code to force the gpio works as digital output, like this:

  // ...
  extDigitalWrite(pin, pin_status);

  if(parser.boolval('D')) return; // D means force digital output

  #ifdef ARDUINO_ARCH_STM32
    // A simple I/O will be set to 0 by hal.set_pwm_duty()
    if (pin_status <= 1 && !PWM_PIN(pin)) return;
  #endif
  hal.set_pwm_duty(pin, pin_status);
  // ...

Then I send M42 I D P74 S1 T1 to the machine, and it works. Digital output is working as expected.

So the problem is, I think, the PWM_PIN(pin) macro is not working as expected on stm32 architecture.
Maybe somethings I missed? I don't know why...

By searching the issue list, I found this issue #17522 M42 is killing me, which seems related to this issue.
It's also about M42 command on stm32f407 board, and finally not get solved 3 years ago.

I'm not sure if this is a bug or not, but I think it's worth to report it here.
Any ideas with this? Thanks.

Bug Timeline

About 3 years ago?

Expected behavior

1. Send M42 I P74 S1 T1
2. The pin should be set to HIGH and the relay should be turned on.

Actual behavior

1. Send M42 I P74 S1 T1
2. The pin is randomly set to HIGH or LOW, most time it's LOW, but sometimes it's HIGH.

Steps to Reproduce

1. Get a stm32f4 series board (I'm using a MKS Monster 8 board, which is a STM32F407VET6 board)
2. git clone & configure & build Marlin for this board
3. Send M42 I P74 S1 T1
4. Check the pin status by sending M43 P74

Version of Marlin Firmware

bugfix-2.1.x

Printer model

STM32F4 series board

Electronics

MKS Monster 8

LCD/Controller

None

Other add-ons

None

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

• A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

Marlin_config.zip

[Angelic47]

#26293 may also related to this issue.

[ellensp]

please add details of what pins your using I and J endstop so we can actually build your example config files.

[Angelic47]

Sorry I forget the pin configuration file. There are no real I and J endstops because this is configurated for an OpenPNP machine. Any pin can be used for compile. Attachments are modified version for Marlin/src/pins/stm32f4/pins_MKS_MONSTER8_common.h

[HeartOfGermany]

Important to know: The behavior seems a bit erratic. On STM32F103RE I am able to set PWM with M42 (Well actually it is analog DC output, but at least something that resembles my M42 set value and with "ignore firmware protection" flag enabled). But using the laser feature it is not creating PWM or analog out. So something is really off here.

I do not know where I read it, but FAN PWM on STM32F103RE was initially also not working for both Parts cooling and Controller fan. This however has been fixed somehow with updates, so maybe the version history could help to pin down where this was fixed for SKR Mini E3 V2 to see, what caused and fixed it. I am however absolutely unable to see any logical flaws in the firmware source, that would explain this behavior. It looks a lot like chaos at some parts, but afterall makes sense if you really dig into the code for what I can tell.

[Angelic47]

After a thorough investigation, I believe I have pinpointed the issue.

Tracking the definition of PWM_PIN, it seems to be defined in Marlin/src/HAL/STM32/fastio.h.
This macro actually points to digitalPinHasPWM(P), which is an Arduino function.

Let's take a look at the Arduino side.
Considering the vast codebase of Arduino, I will only highlight what I think is essential here.
If I'm not mistaken, digitalPinHasPWM(p) is defined in the file Arduino_Core_STM32/cores/arduino/pins_arduino.h, as follows:

// ...
#define digitalPinHasPWM(p)         (pin_in_pinmap(digitalPinToPinName(p), PinMap_TIM))
// ...

This code essentially checks if the current pin is listed in the PinMap_TIM array. Let's examine PinMap_TIM further:
The definition of PinMap_TIM, to accommodate various STM32 series, is found in different files.
For my MKS Monster8 board (actually a STM32F407VET6), the relevant definition is in Arduino_Core_STM32/blob/main/variants/STM32F4xx/F407V(E-G)T_F417V(E-G)T/PeripheralPins.c#L117:

// ...

//*** TIM ***

#ifdef HAL_TIM_MODULE_ENABLED
WEAK const PinMap PinMap_TIM[] = {
  {PA_0,       TIM2,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 1, 0)}, // TIM2_CH1
  {PA_0_ALT1,  TIM5,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM5, 1, 0)}, // TIM5_CH1
  {PA_1,       TIM2,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 2, 0)}, // TIM2_CH2
  {PA_1_ALT1,  TIM5,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM5, 2, 0)}, // TIM5_CH2
  {PA_2,       TIM2,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 3, 0)}, // TIM2_CH3
  {PA_2_ALT1,  TIM5,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM5, 3, 0)}, // TIM5_CH3
  {PA_2_ALT2,  TIM9,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF3_TIM9, 1, 0)}, // TIM9_CH1
  {PA_3,       TIM2,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 4, 0)}, // TIM2_CH4
  {PA_3_ALT1,  TIM5,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM5, 4, 0)}, // TIM5_CH4
  {PA_3_ALT2,  TIM9,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF3_TIM9, 2, 0)}, // TIM9_CH2
  {PA_5,       TIM2,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 1, 0)}, // TIM2_CH1
  {PA_5_ALT1,  TIM8,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF3_TIM8, 1, 1)}, // TIM8_CH1N
  {PA_6,       TIM3,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM3, 1, 0)}, // TIM3_CH1
  {PA_6_ALT1,  TIM13, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF9_TIM13, 1, 0)}, // TIM13_CH1
  {PA_7,       TIM1,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM1, 1, 1)}, // TIM1_CH1N
  {PA_7_ALT1,  TIM3,  STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM3, 2, 0)}, // TIM3_CH2
// ...

Due to length, I am only including a portion here. The complete contents of this array can be checked in the aforementioned file.

Now, here lies a problem.
Understandably, for STM32, almost all pins can serve as hardware PWM outputs driven by TIM, with only a few exceptions.
Arduino, following STM32 documentation, has listed all PWM pins and provided their respective mappings to TIM and TIM channels.
While this isn't an issue in itself, it's worth noting that STM32 does not actually have as many TIM channels. Meaning, many of these pins share the same TIM channel.
In STM32's design, the same TIM channel can select different pins, but the reverse is not true. Using two pins that share the same TIM channel for PWM outputs simultaneously can lead to setting conflicts.
Hence, using Arduino's digitalPinHasPWM directly does not allow specifying any pin for PWM output at any given moment.

Therefore, it's not hard to see why there might be issues with Marlin's M42 G-code.
Since nearly all pins are considered capable of PWM output, hal.set_pwm_duty is called.
As functions involving this feature increase, TIM channel conflicts are almost inevitable.
Moreover, per the M42 G-code's code, once Arduino's digitalPinHasPWM(p) confirms a pin's PWM capability, there's no parameter in M42 G-code to bypass PWM setting, even though it could simply output a digital HIGH or LOW.

Thus, I believe this issue is quite tricky to resolve...
A potentially good solution might be to add a parameter that forces the pin to output a digital HIGH or LOW instead of PWM.
As for other similar bugs related to PWM, I'm not sure if they stem from the same reason. There might also be other issues, but the TIM conflict is certainly a potential suspect.

[HeartOfGermany]

There are many conflicts in the codebase actually. As I use the SKR Mini E3 V2, the codebase actually refers to this HAL:
Marlin > src > STM32 > HAL.cpp

This is odd, as I would suspect it should be:

Marlin > src > STM32F1 > HAL.cpp

The STM32F1 HAL.cpp is completely greyed out and unused if I compile the code. I also cannot force it active as double definitions would break the code. I was unable to find the exact point, where the HAL.cpp actually gets selected just from my Board Definition in the config, so I an unsure what to search for. Can you take a look if you have a similar hickup in logic, where a HAL is selected that does not match your controller exactly at least accordint to its name?

[EvilGremlin]

Everything is correct. STM32F1 HAL is old one using libmaple and can only do F1 chips. STM32 HAL is new one and can do all chips.

[thisiskeithb]

the codebase actually refers to this HAL:

Marlin > src > STM32 > HAL.cpp

This is odd, as I would suspect it should be:

Marlin > src > STM32F1 > HAL.cpp

You would need to compile with one of the *_maple environments for the STM32F1 HAL to be used.

Note: Compiling with *_maple is deprecated, but it's still around since it seems to also work for many non-ST chips.

[HeartOfGermany]

@thisiskeithb I am actually unable to build the _maple variant as I get:

multiple definition of `MarlinHAL::set_pwm_frequency
multiple definition of `MarlinHAL::set_pwm_duty

-error. No idea why this might be. Odd, that there is a multiple definition error for PWM related functions. Coincidence?

[github-actions]

Greetings from the Marlin AutoBot!
This issue has had no activity for the last 90 days.
Do you still see this issue with the latest bugfix-2.1.x code?
Please add a reply within 14 days or this issue will be automatically closed.
To keep a confirmed issue open we can also add a "Bug: Confirmed" tag.

Disclaimer: This is an open community project with lots of activity and limited
resources. The main project contributors will do a bug sweep ahead of the next
release, but any skilled member of the community may jump in at any time to fix
this issue. That can take a while depending on our busy lives so please be patient,
and take advantage of other resources such as the MarlinFirmware Discord to help
solve the issue.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

[thinkyhead]

I happened to update your configs for bugfix-2.1.x while revisiting this old issue to see how it's doing, so here they are if for your reference. 2.1.3 beta is coming soon!

Configs.tar.gz