search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.03k stars 19.12k forks source link

[BUG] Laser PWM control does not work with SKR Mini E3 V2 at all #26293

Open HeartOfGermany opened 9 months ago

HeartOfGermany commented 9 months ago

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

Yes, and the problem still exists.

Bug Description

I tried both Marlin 2.1.2.1 and Marlin 2.1.x-bugfix. I have set the Platform IO version to the latest (V3.3.1), latest Autobuild Marlin.

I have defined PA1 as Laser PWM pin. However the pin state always is at 3.3V, no matter what the input state is (S0-S255). I have also deactivated M106 printing to ensure there is no conflict. I changed PA1 to 1 to avoid conflicts with the Pin Macro.

Manually setting the Pin using M42 results in an ANALOG output voltage. So M42 shows, that the Pin can do more than 0 and 1. It is officially a PWM pin, both according to PIO and the PDF of the STM32F103RE. I am not sure, why the pin is in an analog state and not outputting PWM, but at least M42 with overriding Firmware procetion is able to do more than 0 and 1 state.

M42 is not acceptable for laser engraving, as it has unpredictable delay. Trust me, I tried with chopping each travel move in 0.05mm steps and still the timing is not perfect and always delayed by around 1 cm travel move. What I need is a working inline mode.

I attached the configuration files: Configuration_adv.txt Configuration.txt

Please ignore the Temp Max which is strange - I adjusted thermistor_1.h to work with 400°C

Please note, that the configuration is supposed to work as 3D printer AND Laser engraver, so deactivating printer stuff is no solution unfortunately - and not a fix aswell.

My goal is, to print with LaserGRBL - with no modification to the gcode, if possible

Bug Timeline

All tested Marlin 2 versions so far - with 2.0.9 and old PIO not even M42 worked for Analog DC output

Expected behavior

Actual behavior

Steps to Reproduce

Version of Marlin Firmware

Marlin 2.1.2.1 AND current Bugfix

Printer model

Ender 3 basis, highly modified

Electronics

SKR Mini E3 V2

Add-ons

DC to PWM converter, as PA1 only outputs analog DC from 0-3.3V - I never saw any PWM signals

Bed Leveling

MBL Manual Bed Leveling

Your Slicer

Other (explain below)

Host Software

None

Don't forget to include

Additional information & file uploads

No response

ellensp commented 9 months ago

Provided Config files is not current bugfix 2.1.x, you have updated the serial number on a much older version of marlin.

You have many Configuration issues.

The most serious of which are:

define SERIAL_PORT 1 is in conflict with CR10_STOCKDISPLAY

You only seem to be using #define SERIAL_PORT -1 So all other serial ports should be disabled.

You cannot have #define HEATER_0_MAXTEMP 400 with thermistor type 1. Thermistor 1 has a max readable temperature of 320 (in bugfix, only 300 in older Marlins) . You also have HOTEND_OVERSHOOT=10. So the highest tempature you can set is 310

You also have a pin conflicts You have #define NUM_SERVOS 4 with #define SPINDLE_LASER_PWM_PIN PA1

These are in conflict

PIN: PA1  (A1)   M42 P1           SERVO0_PIN                             
 .                                 SPINDLE_LASER_PWM_PIN

If you need servo ports you need to select a non conflicting IO PIN either for the servo or the laser

HeartOfGermany commented 9 months ago

To address the points:

So you can see, there is no reason why it should not work. Every point has a reason. What I will try today is, I have updated PeripheralPins.c: {PA_1, TIM2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, AFIO_NONE, 2, 0)}, // TIM2_CH2 to {PA_1, TIM2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, AFIO_TIM2_ENABLE, 2, 0)}, // TIM2_CH2

Maybe this helps. There is no compile conflict apperently. I will report if this works.

ellensp commented 9 months ago

"This config is an updated 2.1.2.1". Bugfix no longer uses the macro BOTH or EITHER. Your Configs still have both of these

"Serial Port 1 was required"

No, You do not need two serial ports. you only enable the ports you need. Set #define SERIAL_PORT -1 and disable //#define SERIAL_PORT_2 -1

UART 1 pins are the same IO pis as used by the CR10_STOCKDISPLAY encoder. DO NOT ENABLE UART 1 on any Marlin serial port

"Is this a hardware limit? " yes the thermistor physically cannot read that high a temperature. It is not designed for it.

HeartOfGermany commented 9 months ago

All occourences of BOTH and EITHER are not called. They would simply not compile as function calls are not allowed for defines - this is what the error says. They have been left inside as these are unmet conditions and never checked. If they where checked, it would instantly throw errors. The configs as provided check the sanity check and throw no errors or config related warnings. This likely also misses the point. The issue is likely not the config at all. I think there is an issue with Platform IO. This is also the reason, why in early PIO versions not both fan headers where adjustable. But no one ever fixed the PWM issues for other pins. I am not sure what to search for, but today I will try with what I wrote aboth with the AFIO_TIM2_ENABLE instead of AFIO_NONE for PA1 and PA1 analog uncommented. I think this would be a good idea to try. It compiled without TIMER errors or any errors at all. I honestly cannot see, why the config scheme would matter that much, as all required settings are defined just fine. And the issue was with 2.1.2.1 aswell. And With previous versions too. I cannot rework the whole config everytime you decide to rename like 500 variable names for whatever reason. I have adjusted like 50-100 settings. This takes a whole day or longer. Just fixing name mismatches is much quicker. I mean, the config was properly written from the default 2.1.2.1. Where it also not worked.

"#define SERIAL_PORT_1 2" was used for the SKR Mini E3 V2 initially (Was from an old config) and I have migrated this setting to the latest version. I will try your recommendation. However Serial Port 2 made RX2/TX2 unusable for mods hence I changed it to 1. But good to know, that -1 will be sufficient.

I am not sure with what you mean with UART. I have no UART definition set anywhere. Not even in the 2 pin configuration headers used for SKR MINI E3 V2.

I am sure there is a limit to it. I will test what works and what doesnt. It is just a test and not related to the issue. It was added way after the issue already was a problem I had. I also do not consider to go to 400 as I do not have a full metal hotend and I do not like to breath quickly decomposing Teflon. Default max was 260 and 300 will be plenty.

ellensp commented 9 months ago

serial_port, serail_port_2 etc are marlin setting, you set it to a UART value, ie a physical UART on the chip are positive numbers. special devices like usb cdc serail use -1

ellensp commented 9 months ago

Your constantly missing the point

The first thing anyone attempting to help you would do is see what changes you have made to the config, by comparing it to the stock config.

Only to fine that EVERYTHING is different, because it is not a current config file.

This makes it extremely hard and frustrating to even attempt to assist you.

Here are actual bugfix configs, modified to compile and remove the servos Configuration.zip

So anyone else looking can see what you have actually enabled.

HeartOfGermany commented 9 months ago

Oh now I feel bad. I did not think about textdiff to compare... Yeah, amazing work. I hope you had a script to help with this effort because it looks like a ton of work.

But ellensp: did you see anything that could explain the issue with the PWM?

Please consider: At the beginning of this Board (SKR Mini E3 V2), with older Platform IO and Marlin there was the issue, that only 1 fan was controllable and the other just on or off. I think, they have changed something in Platform IO itself, as the PIO configs have HEAVILY changed, not even looking comparable by now. I do not think this was a marlin config thing at all, as the old configs already should have allowed PWM. I would think, that this fix actually comes from Platform IO for some reason. Not sure what they changed, as I do not know when it was fixed. What is your opinion on this theory? I am currently at work so I cannot check, but I think there is a chance to get it to work with switching the definitions in the following file:

Marlin/buildroot/share/PlatformIO/variants/MARLIN_F103Rx/PeripheralPins.c

HeartOfGermany commented 9 months ago

Okay, changing: {PA_1, TIM2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, AFIO_NONE, 2, 0)}, to {PA_1, TIM2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, AFIO_TIM2_ENABLE, 2, 0)}, Did not help. Now even the analog out is not working. I am unsure on what to do, as these function are apparently not well documented anywhere. Anyone any idea?

What is the exact difference between:

And can another timer like TIM1, TIM3 or TIM4 be used on pins like PA_1?

ellensp commented 9 months ago

There is nothing in the Configuration files that can cause this, and I can replicate that no PWM signal is produced

HeartOfGermany commented 9 months ago

Well I thought there might be some adjustment in the Platform IO / HAL pin configuration file that enabled the Analog output mode for PA1, as this pin is for Servo use typically. But that should not really matter, as the marlin configuration files should define how each pin is used.

For me at least it is not very transparent on how to configure the mode of each pin, as this typically is handled by the actual usecase like FAN, HEATER, THERMISTOR or whatever. There was a time, where SKR Mini E3 V2 had no fan control for both motherboard and parts cooling fan. I can only imagine, that this was fixed by modifying the HAL files itself, as the definiton of FAN for PC6 and PC7 never really changed in the marlin Configuration.h. I am not sure what the issue is. But I am sure that most people will have failed at the attempt to get a laser working at all on this board. Maybe on/of using the fan headers. But for me that is not viable, as the FAN is used to blow away the sud during cutting and changing headers is also not indended for me. And it is a 24V signal, so not good for my laser anyways. Also, I would think, that inline laser mode with G0/G1 or M3/M4 is much more performant and optimized, so always to be preferred, especially since this allows to directly read the LaserGRBL files, which do not output M106 commands.

I hope, that my Patreon subscription from tomorrow on is motivating someone to work on this. I guess it is well invested no matter what. Only less than 1k per month - not a lot of money for such a widely used firmware.

HeartOfGermany commented 8 months ago

Does anyone have any idea why this bug is happening and if I can just workaround it in some way or another? I'd love to have at least a hacky workaround which works for now.

HeartOfGermany commented 8 months ago

I have done some research:

In the file Marlin/Marlin/src/feature/spindle_laser.cpp there is the following content:

  #if ENABLED(HAL_CAN_SET_PWM_FREQ) && SPINDLE_LASER_FREQUENCY
    frequency = SPINDLE_LASER_FREQUENCY;
    hal.set_pwm_frequency(pin_t(SPINDLE_LASER_PWM_PIN), SPINDLE_LASER_FREQUENCY);
  #endif
  #if ENABLED(SPINDLE_LASER_USE_PWM)
    SET_PWM(SPINDLE_LASER_PWM_PIN);
    hal.set_pwm_duty(pin_t(SPINDLE_LASER_PWM_PIN), SPINDLE_LASER_PWM_OFF); // Set to lowest speed
  #endif

However the Configuration_adv.h has the following line:

//#define SPINDLE_LASER_FREQUENCY 2500 // (Hz) Spindle/laser frequency (only on supported HALs: AVR, ESP32, and LPC)

So there is no Frequency defined for any PWM. This is a problem if there is no fallback. I also see nothing that would indicate the emulation of any soft pwm.

In the file: Marlin/Marlin/src/module/temperature.cpp I found this: `

if ENABLED(FAST_PWM_FAN)

define SET_FAST_PWM_FREQ(P) hal.set_pwm_frequency(pin_t(P), FAST_PWM_FAN_FREQUENCY)

else

`

And this works. This also means, that the hal.set_pwm_frequency is not just limited to "AVR, ESP32, and LPC" as claimed. So the Configuration_adv.h needs an update, that this is also possible with the HAL for SMT32F1 for example.

What is also odd is the function:

  #if ENABLED(HAL_CAN_SET_PWM_FREQ) && SPINDLE_LASER_FREQUENCY
    static void refresh_frequency() { hal.set_pwm_frequency(pin_t(SPINDLE_LASER_PWM_PIN), frequency); }
  #endif

I cannot see this function (refresh_frequency()) from the file Marlin/src/feature/spindle_laser.h used anywhere. Did I oversee anything? Not really a required function imho, but it is odd to say the least.

The function apply_power(uint8_t opwr) is used in a few files, especially in Marlin/src/gcode/control/M3-M5.cpp and Marlin/src/module/stepper.cpp which makes sense. It calls the function _set_ocr in return.

  void SpindleLaser::_set_ocr(const uint8_t ocr) {
    #if ENABLED(HAL_CAN_SET_PWM_FREQ) && SPINDLE_LASER_FREQUENCY
      hal.set_pwm_frequency(pin_t(SPINDLE_LASER_PWM_PIN), frequency);
    #endif
    hal.set_pwm_duty(pin_t(SPINDLE_LASER_PWM_PIN), ocr ^ SPINDLE_LASER_PWM_OFF);
  }

Okay and now back to the issue: We call the hal to set the pwm duty, but as the config claims that the frequency is not supported, the hal.set_pwm_frequency is not defined. So it tries to run hal.set_pwm_duty(pin, value);.

Now let's take a look at Marlin/Marlin/src/HAL/STM32F1/fast_pwm.cpp

void MarlinHAL::set_pwm_duty(const pin_t pin, const uint16_t v, const uint16_t v_size/*=255*/, const bool invert/*=false*/) {
  const uint16_t duty = invert ? v_size - v : v;
  if (PWM_PIN(pin)) {
    timer_dev *timer;
    if (timer_freq[timer_and_index_for_pin(pin, &timer)] == 0)
      set_pwm_frequency(pin, PWM_FREQUENCY);
    const uint8_t channel = PIN_MAP[pin].timer_channel;
    timer_set_compare(timer, channel, duty);
    timer_set_mode(timer, channel, TIMER_PWM); // PWM Output Mode
  }
  else {
    pinMode(pin, OUTPUT);
    digitalWrite(pin, duty < v_size / 2 ? LOW : HIGH);
  }
}

So I cannot see, where PWM_FREQUENCY would be comming from actually. Some variants and HAL.h headers contain a definition for PWM_FREQUENCY, but I cannot find it in the STM32F1 context. It is commented out or rarely defined for other BIGTREE boards. I am not actually sure why there is no compiler error, or if I just do not get the obvious.

I am also not sure if Marlin/Marlin/src/gcode/control/M42.cpp is done in an ideal way.

Why is it, that extDigitalWrite(pin, pin_status); is done followed by hal.set_pwm_duty(pin, pin_status);?

I also do not get this logic:

  #ifndef OUTPUT_OPEN_DRAIN
    pinMode(pin, OUTPUT);
  #endif

Yet above, where the state is defined:

      #ifdef OUTPUT_OPEN_DRAIN
        case 5: pinMode(pin, OUTPUT_OPEN_DRAIN); break;
      #endif

I do not grasp why this state would ever be met and it is just confusing to read it. I would change it to:

      #ifdef OUTPUT_OPEN_DRAIN
        case 5: pinMode(pin, OUTPUT_OPEN_DRAIN); break;
      #else
        case 5: pinMode(pin, OUTPUT);
      #endif

In the consequence I would just remove the #ifndef OUTPUT_OPEN_DRAIN section to avoid the unnesessary confusion.

Another optimization:

First of all the M42 documentation needs adjustments, as Only T0-T3 are documented, but T4-T5 are already defined.

Also I would consider reworking the T1 (Output) a bit, as there are multiple possible options:

But there is no way to switch between PWM, Analog and Digital handling of this command. For PWM a frequency parameter would be great (or use the default, which oddly appears to be missing for my SKR Mini E3 V2.

Also I dislike this section:

  #if HAS_FAN
    switch (pin) {
      #define _CASE(N) case FAN##N##_PIN: thermalManager.fan_speed[N] = pin_status; return;
      REPEAT(FAN_COUNT, _CASE)
    }
  #endif

I would like to just do what I want with the I specified. I would rather suggest:

#if HAS_FAN && !parser.boolval('I')

I will continue debugging this issue when I have more time but there is way for improvement.