Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] M914 incorrect handling of "I" parameter resulting in incorrect TMC driver Bump Sensitity settings #24012

Closed
rondlh opened this issue Apr 9, 2022 · 5 comments

Comments

@rondlh
Copy link
Contributor

rondlh commented Apr 9, 2022

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

Yes, and the problem still exists.

Bug Description

Marlin handles the "I" parameter incorrectly for the M914 command (Set TMC Bump sensitivity).
For example:
M914 I1 X10 should set the X2 sensitivity, but it actually sets the X sensitivity.
M914 I2 X10 should not do anything, but it sets the X2 sensitivity.

The bug is located in M911-M914.cpp

  /**
   * M914: Set StallGuard sensitivity.
   */
  void GcodeSuite::M914() {

    bool report = true;
    const uint8_t index = parser.byteval('I');
    LOOP_NUM_AXES(i) if (parser.seen(AXIS_CHAR(i))) {
      const int16_t value = parser.value_int();
      report = false;
      switch (i) {
        #if X_SENSORLESS
          case X_AXIS:
            if (index < 2) stepperX.homing_threshold(value);
            TERN_(X2_SENSORLESS, if (!(index & 1)) stepperX2.homing_threshold(value));
            break;
        #endif

Below a possible solution (similar to how M913 handles the "I" parameter. It sets the I-value to -1 if not supplied, which adjust the sensitivity for both X and X2 if present, and correctly associates I0 with X and I1 with X2.

  /**
   * M914: Set StallGuard sensitivity.
   */
  void GcodeSuite::M914() {

    bool report = true;
const int8_t index = parser.byteval('I', -1);
    LOOP_NUM_AXES(i) if (parser.seen(AXIS_CHAR(i))) {
      const int16_t value = parser.value_int();
      report = false;
      switch (i) {
        #if X_SENSORLESS
          case X_AXIS:
TERN_(X_SENSORLESS,  if (index < 0 || index == 0) stepperX.homing_threshold(value);
TERN_(X2_SENSORLESS, if (index < 0 || index == 1) stepperX2.homing_threshold(value));
            break;
        #endif

The same issues applies to Y, and Z!

Bug Timeline

Don't know

Expected behavior

Settings according to documentation.
I0 relates to X, Y, Z etc.
I1 relates to X2, Y2, Z2 etc.

Actual behavior

I1 relates to X, Y, Z
I2 relates to X2, Y2, Z2

Steps to Reproduce

Check feedback of "M914 I1 X33"

Version of Marlin Firmware

Marlin 2.0.9.3 latest bugfix

Printer model

Custom

Electronics

MKS Monster8 V1.0

Add-ons

BTT TFT35 V3.0

Bed Leveling

ABL Bilinear mesh

Your Slicer

Cura

Host Software

Other (explain below)

Additional information & file uploads

Using BTT TFT35 V3.0 as host software.

@rondlh rondlh changed the title [BUG] M914 incorrect handling if "I" parameter resulting in incorrect TMC driver Bump Sensitity settings [BUG] M914 incorrect handling of "I" parameter resulting in incorrect TMC driver Bump Sensitity settings Apr 9, 2022
@rondlh
Copy link
Contributor Author

rondlh commented Apr 13, 2022

Can somebody please confirm this issue?

@thinkyhead
Copy link
Member

See #11248 and #11249 which standardized the meaning of these parameters across a few G-codes. The documentation may still be out of date, and the M503 report may also still need to be updated to reflect the change.

  • No "I" parameter, I with no value, or I0: both X and X2
  • I1 for only the first stepper driver (e.g., X)
  • I2 for only the second stepper driver (e.g., X2)

The logic of M914 appears to match this rework:

case X_AXIS:
  if (index < 2) stepperX.homing_threshold(value);
  TERN_(X2_SENSORLESS, if (!(index & 1)) stepperX2.homing_threshold(value));
  break;
  • When the index < 2 (0 or 1) the first X driver will be affected.
  • When the index is even (0 or 2) the second driver will be affected.

@thinkyhead
Copy link
Member

I've patched bugfix-2.0.x to bring M913 in line with the rest, and fixed the report for M914. Please test and let us know whether or not the changes work for your setup.

@rondlh
Copy link
Contributor Author

rondlh commented Apr 23, 2022

I've patched bugfix-2.0.x to bring M913 in line with the rest, and fixed the report for M914. Please test and let us know whether or not the changes work for your setup.

The changes work for my setup, thanks!

@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants