M919 - Get or Set Trinamic Chopper Times

[magnificu]

This pull request adds the possibility to temporarily overwrites the TMC chopper times using a gcode command:
e.g.
M919 X Y O3 V4 S1

This means update chopper times for X and Y axis
O3 - set time_off = 3
V4 - set hysteresis_end = 4
S1 - set hysteresis_start = 1

in case there is no parameter e.g. M9999 it reports the current chopper times for all axis

Requirements

TMC drive.
I test it only with TMC5160

Benefits

With this option the user can tune the chopper times using gcode commands and when he is happy with the results he can write the tuned values for time_off, hysteresis_end and hysteresis_start into the firmware Configuration_adv.h

In the future this functionality can be improved e.g. by saving/changing these values in EEPROM.

Configurations

I test it with TMC5160, SPI
config_magni3D.zip

Related Issues

#23252

[XDA-Bam]

I had a quick look at the code. If I didn't overlook anything, it checks if set values are in range, but does not handle null explicitly - that is, if a user e.g. sets M9999 X Y O N4 S1. In that case, Marlin would probably crash with a null pointer exception. I would suggest to check this on a low level in tmc_util.h, to make sure no future G-code or function can accientally cause a NPE there.

Looking at Marlin and RepRap G-codes, M919 seems to be unused and close to other TMC-related G-codes like M911, M914 and M915. Maybe it would be a suitable G-code identifier.

[magnificu]

The variables are uint8_t and int8_t, if there is a parameter that is not given the actual value is 0 not null.
I will update the gcode number to M919 and test the case you suggested M919 X Y O N4 S1

[magnificu]

I did a test as you suggested and it does not crash.
I updated the pull request.

Here you can find some images.
test_images.zip

Is actually writing the default CHOPPER_TIMING from Configuration_adv.h, in my case is
CHOPPER_DEFAULT_24V { 4, 2, 1 }
(see the last image)
That's why I did this initialization in "M919.cpp"

static constexpr chopper_timing_t chopper_timing = CHOPPER_TIMING;
time_off = chopper_timing.toff;
hysteresis_end = chopper_timing.hend;
hysteresis_start = chopper_timing.hstrt;

[teemuatlut]

Conceptually this should be just fine and a good addition but I think the code quality can be improved.
I can take a better look later today.

[thinkyhead]

Changed parameter N to V and made some other adjustments….

[magnificu]

I think the 4 should be 6. Can you please check?

      #if LINEAR_AXES >= 4
        case K_AXIS:
          #if AXIS_IS_TMC(K)
            TMC_SET_CHOPPER_TIME(K);
          #endif
          break;
      #endif

[magnificu]

Without changing the code, I just fetched the latest changes to my fork and now for some reason the automatic checks are failing...
Is someone still taking care of this code review?

[ellensp]

Network issue

Run pip install -U https://github.com/platformio/platformio-core/archive/develop.zip
Collecting https://github.com/platformio/platformio-core/archive/develop.zip
  Downloading https://github.com/platformio/platformio-core/archive/develop.zip (429 kB)
ERROR: Exception:
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/pip/_vendor/urllib3/response.py", line 438, in _error_catcher
    yield
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/pip/_vendor/urllib3/response.py", line 519, in read
    data = self._fp.read(amt) if not fp_closed else b""
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/pip/_vendor/cachecontrol/filewrapper.py", line 62, in read
    data = self.__fp.read(amt)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/http/client.py", line 465, in read
    n = self.readinto(b)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/http/client.py", line 509, in readinto
    n = self.fp.readinto(b)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/socket.py", line 589, in readinto
    return self._sock.recv_into(b)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/ssl.py", line 1071, in recv_into
    return self.read(nbytes, buffer)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/ssl.py", line 929, in read
    return self._sslobj.read(len, buffer)
socket.timeout: The read operation timed out

[magnificu]

@teemuatlut
Thank you for your feedback.
The intention of this commit was my way of trying to give something back to the community with the hope that this work will help other users as well.
Please feel free to update the code and make it suit the programming standards. I used as a base the M906

[magnificu]

@thinkyhead great work

I noticed that you changed the axis letters I, J, K to U, V, W

• Parameters:
• XYZUVWE...

This description of the parameters does not match with the following code in M919.
The axis "I" is very confusing since same letter is used for the index.

      #if LINEAR_AXES >= 4
        case I_AXIS:
          #if AXIS_IS_TMC(I)
            TMC_SET_CHOPPER_TIME(I);
          #endif
          break;
      #endif

      #if LINEAR_AXES >= 5
        case J_AXIS:
          #if AXIS_IS_TMC(J)
            TMC_SET_CHOPPER_TIME(J);
          #endif
          break;
      #endif

      #if LINEAR_AXES >= 6
        case K_AXIS:
          #if AXIS_IS_TMC(K)
            TMC_SET_CHOPPER_TIME(K);
          #endif
          break;
      #endif

[thinkyhead]

I've tweaked the parameter behavior a bit, so that if you leave out the "I" parameter then all stepper drivers for the flagged axes will be updated. And, if you leave out the "T" parameter when there is an "E" parameter, then all E stepper drivers will be updated. The previous behavior was kind of inconsistent, and not very well documented.

[thinkyhead]

Internally, the extra axes are referred to as I, J, K. However, the default G-code parameters for these axes are U, V, and W A, B, and C. If a G-code is using any potential axis names for other parameters, that collision can be checked for at compile-time. For example…

#if HAS_CLASSIC_JERK && (AXIS4_NAME == 'J' || AXIS5_NAME == 'J' || AXIS6_NAME == 'J')
  #error "Can't set_max_jerk for 'J' axis because 'J' is used for Junction Deviation."
#endif

[magnificu]

I noticed that in Configuration_adv.h CHOPPER_TIMING can be overwritten for each axis by:
CHOPPER_TIMING_X
CHOPPER_TIMING_X2
CHOPPER_TIMING_Y
CHOPPER_TIMING_Y2
CHOPPER_TIMING_Z
CHOPPER_TIMING_Z2
CHOPPER_TIMING_Z3
CHOPPER_TIMING_Z4
CHOPPER_TIMING_E
CHOPPER_TIMING_E1
CHOPPER_TIMING_E2
CHOPPER_TIMING_E3
CHOPPER_TIMING_E4
CHOPPER_TIMING_E5
CHOPPER_TIMING_E6
CHOPPER_TIMING_E7

I think would be more correct to use those values for the ct initialization depending on the i (axis) values
In case no value is given for O, P, S the original values from CHOPPER_TIMING_? are kept.

[thinkyhead]

I think would be more correct to use those values

Yeah, I was gonna say….
I'll see if I can whip that together.

[magnificu]

I tried to "collect" all default chopper times in an array int ct_array[] = {CHOPPER_TIMING, CHOPPER_TIMING_X, CHOPPER_TIMING_X2, etc}, but they are not all necessarily defined in Configuration_adv.h
I was thinking to use it as ct_array[i] and make the code cleaner...

[magnificu]

@thinkyhead
I think the commit to Alter 'I' and 'T' behavior is not necessarily good because it limits flexibility and can cause hardware damage.

M919 was thought to temporarily fine tune the chopper times for each axis independent.
Even if the same drivers & motors are used they respond slightly different due to manufacturing tolerances.
If the user wants to set different chopper times for e.g. Z, Z2, Z3 this would not be possible with the altered behavior since
M919 Z I2 O4 P2 S1 will set Z3 and overwrite Z2 and Z as well with no possibility to avoid that.
The old behavior was more flexible.

M906 with the altered behavior can cause hardware damage in case the user have different drivers & motors for the Z, Z2 and Z3 axis.
The old behavior was safer.

[thinkyhead]

It was intended that M919 Z I2 O4 P2 S1 should only set Z3. If you wanted to set the first Z driver only, the form would be M919 Z I0. When the I parameter is left out, that would specify all Z drivers. I'll triple-check the implementation to make sure it is working in that manner.

[thinkyhead]

This PR is somehow corrupted and can't be edited, so it has been recreated at the PR linked above.