Add percent format to cutter power

[descipher]

Requirements

None

Description

Adds a % padding to the Laser/Spindle Power menu display value without conversion, defines a MENU data TYPE of percent_3 and it formats to d%.

Benefits

Clear understanding of what the display value represents when manually changing the Spindle/Laser power from the menu with configuration_adv.h-> #define CUTTER_POWER_UNIT PERCENT"

Configurations

#define LASER_FEATURE or SPINDLE_FEATURE
#define CUTTER_POWER_UNIT PERCENT
#define REPRAP_DISCOUNT_SMART_CONTROLLER or any LCD display

Related Issues

[descipher]

Was hoping this would be merged soon, I have another pull ready but I need to delete this branch on my end to clean up an issue.

[sjasonsmith]

Are we concerned that changing this default may cause unexpected behavior for people upgrading?

If they set power to 100, previously that would be 39.2% power, and it will now be 100% power.

I am not very familiar with laser workflows to know how likely of a problem this would be.

If we want to change the default without the risk of unexpected over-powered lasers, we migth have to rename the constant and add a SanityCheck to force people's attention to it.

[descipher]

The display will be in %, there is no issue keeping it at PWM. I would think anyone who can compile and upgrading should be aware of what mode they require for M3 commands.

[sjasonsmith]

I am thinking about the way I upgrade my configurations. I rebase my configurations on the latest bugfix. If a previous default was good enough for me, it will not cause a merge conflict if it changes. I will only see a conflict if I had actually changed the value.

[descipher]

FYI, Smoothie firmware defaults to %, GRBL default is PWM so it's a toss up. Maybe we should keep it as PWM for safety then.

[sjasonsmith]

That is probably the safest things to do. To double-check, the display is always in percent, no matter that this is set to?

[descipher]

Ok, I set it to PWM. The display is dynamic, if it set at PWM you get 0-255.

[descipher]

There are edits outside of my testing, I see the issue, the test for PWM or PERCENT is nested and hiding it. going to run it through VS and then update it.

[sjasonsmith]

You refer to CUTTER_UNIT_IS?

I found that when I was exploring, trying to understand if this changed only the display, or also G-Code processing.

[descipher]

The display is what it changed, however it pulls it from the existing code.

[shitcreek]

Since Marlin is based off of GRBL, we think we should keep the same default.

[descipher]

That's odd, Checking ...

[sjasonsmith]

It looks like changing that setting highlighted a bug:

[Test mega1280] Spindle, MESH_BED_LEVELING, closed loop, Power Monitor, and LCD...
In file included from Marlin/src/MarlinCore.cpp:154:0:
Marlin/src/feature/spindle_laser.h:146:12: error: #error "???"
           #error "???"
            ^
Marlin/src/feature/spindle_laser.h: In static member function 'static cutter_power_t SpindleLaser::power_to_range(cutter_power_t)':
Marlin/src/feature/spindle_laser.h:148:7: error: expected primary-expression before ')' token
       ));

Prior to merging this, it seems we will want to make sure we get some better representation for the various options into our CI test builds. Spindle/Laser are probably already pretty under-represented there.

Ideally we would find some existing test configs to add to, rather than adding new ones. I don't know if that will be possible here. You could add some yourself or we can figure something out prior to a merge.

[descipher]

So there is a bug in the original 2.0.x branch.
#if CUTTER_UNIT_IS(PWM255)
0
#elif CUTTER_UNIT_IS(PERCENT)
1
#elif CUTTER_UNIT_IS(RPM)
2
#else
#error "???" <-- not sure what's up with this?
#endif
));

[descipher]

So what this reveals is there must be a define of one of the three and code will die without a valid default. Not a real issue but something to note.

[descipher]

Agreed there is a need for more test scope around CNC and Laser capability. I will get more involved with that going forward.

[shitcreek]

So there is a bug in the original 2.0.x branch.
#if CUTTER_UNIT_IS(PWM255)
0
#elif CUTTER_UNIT_IS(PERCENT)
1
#elif CUTTER_UNIT_IS(RPM)
2
#else
#error "???" <-- not sure what's up with this?
#endif
));

There is a sanity check for this, however, whoever added the bit for a servo forgot to add a case for the servo.
Should probably add something like this:

configuration_adv.h

//#define SPINDLE_SERVO         // A servo converting an angle to spindle power
#ifdef SPINDLE_SERVO
  #define SPINDLE_SERVO_NR   0  // Index of servo used for spindle control
  #define SPINDLE_SERVO_MIN 10  // Minimum angle for servo spindle
  **#define SPINDLE_SERVO_MAX 180 // Maximum angle for servo spindle**
#endif

spindle_laser.h:

// Correct power to configured range
    static inline cutter_power_t power_to_range(const cutter_power_t pwr) {
      return power_to_range(pwr, (
        #if CUTTER_UNIT_IS(PWM255)
          0
        #elif CUTTER_UNIT_IS(PERCENT)
          1
        #elif CUTTER_UNIT_IS(RPM)
          2
        **#elif ENABLED(SPINDLE_SERVO)
          3**
        #else
          #error "???"
        #endif
      ));
    }
    static inline cutter_power_t power_to_range(const cutter_power_t pwr, const uint8_t pwrUnit) {
      if (pwr <= 0) return 0;
      cutter_power_t upwr;
      switch (pwrUnit) {
        case 0:                                                 // PWM
          upwr = cutter_power_t(
              (pwr < pct_to_ocr(min_pct)) ? pct_to_ocr(min_pct) // Use minimum if set below
            : (pwr > pct_to_ocr(max_pct)) ? pct_to_ocr(max_pct) // Use maximum if set above
            :  pwr
          );
          break;
        case 1:                                                 // PERCENT
          upwr = cutter_power_t(
              (pwr < min_pct) ? min_pct                         // Use minimum if set below
            : (pwr > max_pct) ? max_pct                         // Use maximum if set above
            :  pwr                                              // PCT
          );
          break;
        case 2:                                                 // RPM
          upwr = cutter_power_t(
              (pwr < SPEED_POWER_MIN) ? SPEED_POWER_MIN         // Use minimum if set below
            : (pwr > SPEED_POWER_MAX) ? SPEED_POWER_MAX         // Use maximum if set above
            : pwr                                               // Calculate OCR value
          );
          break;
        **case 3:                                                 // SERVO
          upwr = cutter_power_t(
              (pwr < SPINDLE_SERVO_MIN) ? SPINDLE_SERVO_MIN     // Use minimum if set below
            : (pwr > SPINDLE_SERVO_MAX) ? SPINDLE_SERVO_MAX         // Use maximum if set above
            : pwr                                               // Calculate OCR value
          );
          break;**
        default: break;
      }
      return upwr;
    }

  #endif // SPINDLE_LASER_PWM

[descipher]

Yes I agree, it looks like unfinished servo angle control defines, we can complete it.

[descipher]

After reflecting on the Servo Angle missing elements, it does not make sense to change it ATM. A servo is positional in respect to angle and while it may use a power factor we do not know if that is the intent. I think we can leave that alone until understand the intent.

[sjasonsmith]

The SPINDLE_SERVO feature is intended for use with a "Hobby Servo ESC", basically an Electronic Speed Controller that takes a typical servo signal as an input. Setting it as a percentage could certainly work, but more variables would likely be needed to do so correctly. It probably is best to leave it for somebody utilizing the feature to expand upon.

For example, some ESC's might treat the min pulse as off, while for others that would maximum reverse speed, with a centered pulse representing off.

[descipher]

We are on the same page. Agreed it is something better left for an active user of the feature rather than mucking with it.

…

________________________________ From: Jason Smith <notifications@github.com> Sent: Tuesday, December 15, 2020 10:05:59 PM To: MarlinFirmware/Marlin <Marlin@noreply.github.com> Cc: Mike La Spina <mike.laspina@shaw.ca>; Author <author@noreply.github.com> Subject: Re: [MarlinFirmware/Marlin] Add percent format to cutter power (#20410) The SPINDLE_SERVO feature is intended for use with a "Hobby Servo ESC", basically an Electronic Speed Controller that takes a typical servo signal as an input. Setting it as a percentage could certainly work, but more variables would likely be needed to do so correctly. It probably is best to leave it for somebody utilizing the feature to expand upon. For example, some ESC's might treat the min pulse as off, while for others that would maximum reverse speed, with a centered pulse representing off. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#20410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACR4XP3CRVPTAZJR47FNREDSVAW2PANCNFSM4US5VFGQ>.

[sjasonsmith]

@shitcreek do you have any remaining concerns on this one? At this point it seems to be just a cosmetic change in the menus, and doesn't change any behavior if you aren't using PERCENT as your power format.

[descipher]

Confirmed this change only impacts the Menu value presentation when the user sets PERCENT as the power unit. You send M3 S0 to 100 when in PERCENT unit power and thus I think we should see that mode on the display. Otherwise you will see 0-100 vs 0-100%. With the change it is clear that you are working with percent units.