Laser Refactor

[shizacat]

Hello.

Description

I request improvement in code of laser. Was to make on finite state machime conception.
it have four state: ON, OFF, and transitional - TO_ON, TO_OFF.

Benefits

• All events have processed in one place (apply_power)
• Power value set only throught one function (apply_power)
• Fixed bug - don't work wait when laser on/off. (SPINDLE_LASER_POWERUP_DELAY, SPINDLE_LASER_POWERDOWN_DELAY)
• Slightly rename function (ocr_set_power) and added comments.

[thinkyhead]

As I understand it there are situations when the spindle ENA pin ought to be left in the ON state even though the PWM is being set to 0 so that the capacitors are able to remain charged up. There may also be some rationale for setting the power value without applying it to the PWM right away.

A good first step would be to go through all the existing spindle/laser code and add lots of commentary that explains exactly what is being done and why. If there are questions about some things, we can refer back to previous Pull Requests and post questions tagging the authors.

[thinkyhead]

• Please take a look at Inline Laser Power Improvements and fixes #22690 and consider working with @descipher to document the existing code and figure out the total refactor that is required to improve this subsystem and make it well-behaved.
• Please give consideration to the reversible Spindle and not just the laser. Pay particular attention to the reasoning behind unitPower, power (here renamed ocr_power), and menuPower, when these values are set, and when these values are subsequently converted to "OCR" power and applied to the hardware ENA and PWM pins.
• Pay very careful attention to the situations where unitPower is applicable, versus when ocr_power is applicable.
• Consider situations when the machine is idle and not generating movement blocks (and therefore not applying the OCR power to the spindle/laser from the Stepper ISR block processor).
• Consider what may be required to accommodate future functionality, such as receiving a list of power levels (i.e., a grayscale raster line) and interpolating that list over a single linear move.

[shizacat]

Concerning

spindle ENA pin ought to be left in the ON state even though the PWM is being set to 0

Other words M3 S0 command
I suggest this solution (concept):

Code Block

// ocr_power - have next roles: current out power for PWM; current angle for SERVO; enable/disable ENA_PIN; and state FSM for laser/spindel  (0 - disable; > 0 - enable)

// Control ENA_PIN in mode PWM. If defined ENA_PIN can be disabled throught M5 only. If it isn't defined when call M3 S0 ENA_PIN will be disable.
#define SPINDLE_LASER_PWN_SEPARATE_PIN

void SpindleLaser::ocr_set_power(const uint8_t opwr, const bool on = true);

/**
 * Set cutter ocr_power value for PWM, Servo, and on/off pin.
 *
 * @param opwr Power value. Range 0 to MAX. When 0 disable spindle/laser.
 * @param on  Only for PWM. Separate control ENA_PIN,
 *            if true - enable pin; false - disable pin (default true)
 */
void SpindleLaser::ocr_set_power(const uint8_t opwr, const bool on) {
  static uint8_t last_power_applied = 0;

  switch (get_event(opwr)) {
    case SpindleLaserEvent::ON:
      if (opwr == last_power_applied) break;
      last_power_applied = ocr_power = opwr;

      #if ENABLED(SPINDLE_LASER_PWM)
        ocr_set(ocr_power);
        isReady = true;
      #endif

      #if ENABLED(SPINDLE_SERVO)
        MOVE_SERVO(SPINDLE_SERVO_NR, ocr_power);
      #else
        #if BOTH(SPINDLE_LASER_PWM, SPINDLE_LASER_PWN_SEPARATE_PIN)
          ena_pin_set(on);
          // isReady = on; - ???
        #else
          ena_pin_set(true);
          isReady = true;
        #endif
      #endif
      break;

    case SpindleLaserEvent::TO_ON:
      last_power_applied = ocr_power = opwr;

      #if ENABLED(SPINDLE_LASER_PWM)
        ocr_set(ocr_power);
        isReady = true;
      #endif

      #if ENABLED(SPINDLE_SERVO)
        MOVE_SERVO(SPINDLE_SERVO_NR, ocr_power);
      #else
        #if BOTH(SPINDLE_LASER_PWM, SPINDLE_LASER_PWN_SEPARATE_PIN)
          ena_pin_set(on);
          // isReady = on; - ???
        #else
          ena_pin_set(true);
          isReady = true;
        #endif
      #endif

      power_delay(true);
      break;

    case SpindleLaserEvent::OFF: break;

    case SpindleLaserEvent::TO_OFF:
      last_power_applied = ocr_power = opwr;

      #if ENABLED(SPINDLE_LASER_PWM)
        isReady = false;
        ocr_set(0);
      #endif

      #if ENABLED(SPINDLE_SERVO)
        MOVE_SERVO(SPINDLE_SERVO_NR, ocr_power);
      #else
        isReady = false;
        ena_pin_set(false);
      #endif

      power_delay(false);
      break;
  }
}

Now
PWM enable: call ocr_set_power( S value ) - behave pin only dependents SPINDLE_LASER_PWN_SEPARATE_PIN
PWN disable call ocr_set_power( value ) 0 - disable, >0 enable
M5
ocr_set_power(0, false) - disable in any case

[descipher]

Welcome aboard @shizacat

I have, over the last 30 days walked the cutter code in an effort to fully understand it, I have also observed issues in the same.
Some of the challenges I have encountered are not the code itself.
I have found that there are many different features added that are not in concert.
Some features only focus on one finite part of the overall function while others
consider a higher level of functional co-existence.

Here are some examples of what I have found:

The "cutter" has a multitude of operating requirements depending on what the CNC hardware target use case will be.
The capability of that hardware is diverse and any thing I wanted to change had the issue of negative impacting already active use cases.
For example in a "state machine view" pins do not really exist but controlling a particular use case will employ pins to do so.
So now the abstraction challenge surfaces.
If I have Solid State Laser hardware then the enable pin is just the PWM output. (Some targets do not even have PWM capability!).
But if I have a CO2 laser "cutter" a separate enable pin is required.
The code needs to accommodate both existing active use cases or it will break a multitude of them.
On many CO2 lasers the physical enable pin is used in multiple scenarios, usually for safety.
The enable pin commonly activates a CO2 laser power HV supply. (also in various ways!)
Now looking at the state machine to pin control abstraction mapping to hardware target mapping.
When I only invoke a PWM output power control state I still need to now define a additional enable
output events even though there may or may not be an associated pin to work with.

Then there is the Spindle/Servo cutter targets. These are generally less complex and have their own set of requirements.
Things like an ocr value of 0 may not be an off state or we need a minimum speed with a delay to be enforced before any movements and then there is reversing states.
Another target is the Pen based positional Servo drawing CNC, fortunately it's very simple.

What becomes clear to me is that the G-CODE processing has limitations and should be the first area of concern.
G-CODE's are processed serially with buffering and are not always in sync with the cartesian planner movements.
The planner code calculates block movement of G-CODE's such as G0,1,2,3 and 5 or Extruders etc. are processed and queued, then the block moves are actioned by the stepper ISR process.
M3/4/5 G-CODE's are not processed in the planner or stepper, we must wait for the queued move to be completed and then we can allow an M3/4 or 5 to be inserted.
However we can queue an M3/4 unitPower into the next planner G-CODE move.

Within any cutter G-CODE power input we currently have 4 unitPower inputs to deal with. OCR 0-255, Servo angle 0-180, Percent 1-100, and RPM, the latter two need to be converted to either OCR or in the absence of PWM an output logic of on/off. We also have to scale the min/max limits and any inversion requirements. Also some G-CODE generation software products like LaserGRBL use a format of 0-1000. (compatibility modes could be useful) FYI GRBL is the basis for the planner and stepper code in Marlin.

An even more complex state is when we add movement, power and tools.
Like when the target is a SCARA robot with changeable tools etc.
The state would need to change in flight based on the tool, power level differences and movement types.
We aren't there yet but we need to consider that going forward in any design constructs.

All these elements need careful consideration.

Currently my approach is to use operational Mode Flags and this method was proven successful in GRBL.
Right now I'm doing menu testing and unitPower testing on my PR. There are some issues to address with my changes in those areas. I have not examined or tested a state machine method addon, but we can certainly explore it and see how it would improve the current code. Timing is very tricky so I'm not sure that it would be successful. The stepper ISR is very sensitive, I found that I can trigger a watchdog in some power processing scenarios.

[shizacat]

@descipher Thank you )

Ohhh, How all difficult )

laser/spindel module - LPM

I looked at your 'PR', it is very big (it touch many side of firmware).
It seems to me that they should be solved gradually.
Your PR does not touch the kernel LPM, and it is very poorly written, which fix my PR.
Inline block should work above LPM, and LPM doesn't need to known about Inline, menu, M/G comand, safety pin, so on.
His goal is simple - control laser/spindel.

This PR doesn't change behave LPM. Its goal is to make code LPM maintainable, readable, and testable.
And sort of how it turned out...
This PR slightly conflict with your. May be will it be approved, and you slightly modify your code?
Otherwise, what to do next with this PR?

Thoughts:

• Reverse - his behavior will be easily implemented with what is already there. Just need to know what )
• Safety pin - is a separate external module, he don't link to the laser
• Separate ena pin - there is already a sketch above
• Units of measurement - in it need to figure it out, but this is an abstraction layer and it will not touch the laser module. Unless this can be attributed to changing the parameter to 16 bit for set pwm.

[descipher]

I looked at your 'PR', it is very big (it touch many side of firmware).

Should only be 7 files, I messed up the PR with a commit from a different system which was not pointing to the correct position.
Just checking with Scott for guidance on fixing that without further pain .. lol

Thoughts:

• Reverse - his behavior will be easily implemented with what is already there. Just need to know what )

At a minimum:
Currently when SPINDLE_CHANGE_DIR and SPINDLE_FEATURE are defined and a G-Code of M3 or M4 is processed the state machine should implement the following:

detect dir change
turn off if not the same dir
wait for spindle stop delay time set by SPINDLE_LASER_POWERDOWN_DELAY
set speed/power
change dir via set_reverse(flag)
turn on
wait for spindle start time set by SPINDLE_LASER_POWERUP_DELAY
update state

Please note the flag isReady is really not appropriate. I believe it is related to menu based controls and from what I gather is used to prevent applying power from the menu at the wrong time e.g. a spindle that currently on being reversed while in forward etc. The state machine would need to handle that.

At some point the state machine could also work with a cutter_mode flag method like the approach I used.

• Safety pin - is a separate external module, he don't link to the laser

This is integrated as the enable pin in almost all cases, for example on the popular K40 laser. The PSU enable pin is pulled high with a pullup resister and this kills the power. The same enable pin is interrupted serially by safety switches etc.

• Separate ena pin - there is already a sketch above

Yes, however the enable should be blocked when a fault or other lockout is active. e.g. cutter_mode = -1 or Error state.
The state machine should shutdown outputs during an error state.

• Units of measurement - in it need to figure it out, but this is an abstraction layer and it will not touch the laser module. Unless this can be attributed to changing the parameter to 16 bit for set pwm.

Yes this is abstracted and the state machine is not aware of it. The spindle_laser module does have routines for this and the state machine can just apply power that is already converted. This is the Snnnnn G-Code power parameter processing and yes it could have a future option for a 16 bit PWM but not currently.

[descipher]

Take note that cutter.power is not necessarily OCR based and that renaming brings confusion with the ocr calls.

[descipher]

FYI
void SpindleLaser::ocr_set_power(const uint8_t opwr, const bool on) {
static uint8_t last_power_applied = 0; <- this makes no sense in the current code, this will always set it to 0 and the last state is always 0 before being tested. I will move this to a public and initialize it to zero there.

I cleaned up some of my PR so you can now see just the actual file changes. Will do some history cleanup later.

[shizacat]

@descipher Thank you
I added new state for direction.
Draw FSM: https://drive.google.com/file/d/1OXvSYsoHccdlwEa9ZOA20Dpy0WLN3lsk/view?usp=sharing

[descipher]

The purpose of isReady is clear after some elimination tests. It is used to determine if the display should reveal the active power output level. When isReady is false no value is presented to the user. We should add this information to the comments.
I suspect it was defined to handle inverted state as active e.g. 0 is on thus isReady = true vs 0 so the output pin cannot be the flags reference.

[descipher]

@shizacat What hardware do you have to test with?

[thinkyhead]

If it is possible to apply the improvements outlined in this PR to the latest Marlin code, then please update this PR. If the changes are no longer compatible or required, then we can simply close this PR.