[BUG] (TMC2208 hybrid_threshold not updating board)

[SISLANGER]

Description

TMC2208 hybrid_threshold not updating on board always shows 0 with TMC2208 UART on SKR V1.3

Steps to Reproduce

SEE DESC

Expected behavior: Show updated values or any values other than 0

Actual behavior: [What actually happens] only shows set values of 0

[ghost]

Could you maybe post the output from an M122 command ? .. assuming you've enabled TMC_DEBUG

Maybe also zip and post your two config files ?

[teemuatlut]

See if your drivers can communicate first http://marlinfw.org/docs/hardware/tmc_drivers.html#troubleshooting

[SISLANGER]

 

M122 X Y Z E E1 Enabled false false false false false Set current 800 800 800 800 800 RMS current 795 795 795 795 795 MAX current 1121 1121 1121 1121 1121 Run current 25/31 25/31 25/31 25/31 25/31 Hold current 12/31 12/31 12/31 12/31 12/31 CS actual 12/31 12/31 12/31 12/31 12/31 PWM scale 14 14 14 14 14 vsense 1=.18 1=.18 1=.18 1=.18 1=.18 stealthChop true true true true true msteps 16 16 16 16 16 tstep max max max max max pwm threshold 78 78 66 64 164 [mm/s] 100.25 100.04 30.21 30.00 50.29 OT prewarn false false false false false OT prewarn has been triggered false false false false false off time 3 3 3 3 3 blank time 24 24 24 24 24 hysteresis -end -1 -1 -1 -1 -1 -start 1 1 1 1 1 Stallguard thrs DRVSTATUS X Y Z E E1 stst X X X X X olb ola s2gb s2ga otpw ot 157C 150C 143C 120C s2vsa s2vsb Driver registers: X 0xC0:0C:00:00 Y 0xC0:0C:00:00 Z 0xC0:0C:00:00 E 0xC0:0C:00:00 E1 0xC0:0C:00:00     Testing X connection... OK Testing Y connection... OK Testing Z connection... OK Testing E connection... OK Testing E1 connection... OK

[ghost]

Looks OK.

When you say the threshold value shows as '0', do you mean on an LCD display ? .. if so, what LCD display do you have ?

[SISLANGER]

Archive.zip

@doggyfan @teemuatlut

[sreichholf]

I can confirm this issue on an Ender 3 with SKR 1.3 and CR10_STOCKDISPLAY. All hybrid threshold values show "0" on LCD.

I have left //STEALTHCHOP_E disabled, though I do have 4x2208 as it doesn't work anyways.

[SISLANGER]

@doggyfan yes all other values show fine, except the hybrid_threshold which I can hear and see the changes when I change it live during a print.

[SISLANGER]

I get the following on compelling

Marlin/src/feature/tmc_util.cpp:146:28: warning: 'TMC_driver_data get_driver_data(TMC2208Stepper&)' defined but not used [-Wunused-function] static TMC_driver_data get_driver_data(TMC2208Stepper &st) { ^~~~~~~ Marlin/src/feature/tmc_util.cpp:143:23: warning: 'uint32_t get_pwm_scale(TMC2208Stepper&)' defined but not used [-Wunused-function] static uint32_t get_pwm_scale(TMC2208Stepper &st) { return st.pwm_scale_sum(); } ^~~~~

[SISLANGER]

I can confirm this issue on an Ender 3 with SKR 1.3 and CR10_STOCKDISPLAY. All hybrid threshold values show "0" on LCD.

I have left //STEALTHCHOP_E disabled, though I do have 4x2208 as it doesn't work anyways.

OK so its not just board specific I know I have seen them in the settings to start with so maybe a bug, maybe a conflict of settings

[ghost]

I don't get that compile warning, but yes I too can confirm something has gone wrong with the Marlin code at some point. I too am getting '0' with my four TMC2208 thresholds on the LCD with the SKR 1.3 board - external 12V power to board and TMC2208's.

M122 looks OK too though.

00: X Y Z Z2 Enabled false false false false Set current 1000 1000 1000 1000 RMS current 994 994 994 994 MAX current 1402 1402 1402 1402 Run current 17/31 17/31 17/31 17/31 Hold current 8/31 8/31 8/31 8/31 CS actual 8/31 8/31 8/31 8/31 PWM scale 10 10 10 10 vsense 0=.325 0=.325 0=.325 0=.325 stealthChop true true true true msteps 16 16 16 16 tstep max max max max pwm threshold 29 29 38 38 [mm/s] 136.31 136.31 13.00 13.00 OT prewarn false false false false off time 4 4 4 4 blank time 24 24 24 24 hysteresis -end 2 2 2 2 -start 1 1 1 1 Stallguard thrs
DRVSTATUS X Y Z Z2 stst X X X X olb
ola
s2gb
s2ga
otpw
ot
157C
150C
143C
120C
s2vsa
s2vsb
Driver registers: X 0xC0:08:00:00 Y 0xC0:08:00:00 Z 0xC0:08:00:00 Z2 0xC0:08:00:00

Testing X connection... OK Testing Y connection... OK Testing Z connection... OK Testing Z2 connection... OK ok

[gloomyandy]

Please test with a very recent version of Marlin. There have been changes to the menu code in the last day or so that impact the way that some values are displayed and edited.

[SISLANGER]

@gloomyandy it was a recent fresh bugfix-2.0.x from a week or so ago

[ghost]

@gloomyandy just compiled with the latest code on github and still no change. All thresholds are showing '0' in the LCD menu.

So at some point something has gone a miss in the code. I don't yet know the menu code well enough to track it down at the moment, though I'm just looking through the code to see how it works in that section.

[Patag]

Any relation with this #13990 ?

[gloomyandy]

@SISLANGER a week or so ago is way too old, this change was like 2 days ago I think. But it looks like there is still a problem with the latest code.

@doggyfan if you change the threshold using the LCD (so change the value from zero), does that new value show up in an M122?

[SISLANGER]

@gloomyandy I had a look at the changes and wasn't related to the board or issue

[ghost]

@doggyfan if you change the threshold using the LCD (so change the value from zero), does that new value show up in an M122?

Yes it does, if I edit the threshold on the LCD menu it shows up in the M122 response. So it's editing it, just not displaying it correctly in the LCD menu.

Strangley, the LCD menu setting is remembered if I exit the TMC menu and go back into it.

[gloomyandy]

@doggyfan and @Patag it looks very much like the same issue that is causing #13990

[ghost]

yep, okey doke @gloomyandy

I'd download previous code commits and see exactly where the problem occured if only I could get on with github.

I have windows 7, git and tortoisegit installed. How do I download previous commits I wonder ?

This way I can find the exact code change that has caused this.

[SISLANGER]

@doggyfan If possible could you let me know the fix or solution when you get a chance. Thank you

[ghost]

Yes I will do that for you @SISLANGER with pleasure. I just need to find out how to download previous Marlin commits from github first.

[SISLANGER]

Thank you @doggyfan Is this it https://github.com/MarlinFirmware/Marlin/tree/15357af67ceb74b14606eba9fbb75d20914f8909

[ghost]

No, need to go back further than that as that version has the problem too.

How did you select that tree ? .. I need to be able to do this myself.

[SISLANGER]

@doggyfan

Sorry I'm just getting used to GitHubI found a list under commits

https://github.com/MarlinFirmware/Marlin/compare/bugfix-2.0.x

[ghost]

OK, I'm getting there slowly.

This problem was created between the 2nd May and the 4th May. I'm just homing in on the exact commit that has caused this.

[SISLANGER]

Cool

[ghost]

Yes sorry @SISLANGER, it was that one that's caused this problem.

https://github.com/MarlinFirmware/Marlin/tree/15357af67ceb74b14606eba9fbb75d20914f8909

The previous commit is fine.

A lot of other changes were made in the commit that's caused this, not just the backlash code that was added/changed.

[SISLANGER]

No worries @doggyfan have fun ;-)

[ghost]

I'm not yet familiar enough with Marlin to fix this. Whoever made those changes (of which is many) needs to be brought in to fix it I think.

[SISLANGER]

@doggyfan Thank you for trying

@thinkyhead @marcio-ao

Any possibility you can help on this bug / issue

[teemuatlut]

Good job tracking down the commit. Helps a ton!

At a quick glance I see ui.init was moved to before settings.load. ui.init is responsible for setting up the LCD along withe TMC LCD section. settings.load calls the function that initiates the TMC drivers and their configuration.

Revert that and see if that helps any.

[SISLANGER]

@teemuatlut sorry on what file

[teemuatlut]

Marlin.cpp on setup()

[ghost]

Yes, reverting that ui boot-up position does indeed fix the problem @teemuatlut.

But I suspect we can't just move it back without concequences to other parts of the code. Seems the ui needs to be done first.

Really, the very first thing that should be setup is the ui/LCD, because it's the only feedback for the user when things go wrong at boot-up.

If it were me, I'd have the entire boot-up sequence displayed on the LCD as it goes through it all. That way the user gets to see where bad problems occur.

[marcio-ao]

It looks like there is conflicting requirements that need to sorted through. When I moved "ui.init()" before "settings.load()", I did my best to check whether it would introduce any problems, but I must have failed to see this one.

Anyway, the reason I made the change is that our implementation of the ExtUI is storing settings in the EEPROM related to things like brightness, touch register configuration, etc. The following sequence of things happen:

1. ui.init() is called, which calls the ExtUI to initializes the display hardware to the point in which it can begin accepting commands.
2. settings.load is now called, which reads data from the EEPROM and calls the ExtUI with EEPROM data pertaining to the UI.
3. The ExtUI takes this data and loads it into the registers on the graphics chip.

The reason for the re-org is that if 2 and 3 happens before 1, then the graphics chip isn't initialized so it cannot receive the settings.

It sounds like the TMC code is expecting things to happen in another order. I'll read through this thread and see if I can understand what is going on.

[marcio-ao]

I guess in going through the comments it isn't clear to me why the TMC code would require the EEPROM to be read before the UI is initialized. @teemuatlut: Could you perhaps clarify?

[sreichholf]

I just did a quick "flyover" but in init_tmc_section() (which is at the en dof MarlinUI::init) init_lcd_variables() is called for each tmc stepper. That in return requires the settings to be initialized: as it sets the initial values from
planner.settings.axis_steps_per_mm[spmm_id]

So init_tmc_section has to be moved behind loading the settings (I'm not familiar enough with Marlin to suggset a proper fix "just so", sorry).

[marcio-ao]

I suspect the issue is with the function init_tmc_section() which is called at the end of ui.init(). It looks like it shadows the TMC state into separate state variables in RAM. Before the re-ordering, that shadowing happens after the EEPROM had been loaded and after the initial state had been loaded into the TMC drivers. After the re-ordering, it happens before the EEPROM has been loaded, so it shadows the default values rather than EEPROM values.

The fix to this appears to be simple, the call to init_tmc_section() could be placed at the end of _load() in configuration_store.cpp.

[marcio-ao]

@sreichholf: Yes, looks like you identified the problem a split second before I did :)

[marcio-ao]

In fact, the ExtUI already implements a method for doing stuff after the EEPROM has been loaded. We could put init_tmc_section in the same location to make the relationship clear:

  bool MarlinSettings::load() {
    if (validate()) {
      const bool success = _load();
      #if ENABLED(EXTENSIBLE_UI)
        ExtUI::onConfigurationStoreRead(success);
      #endif
      #if HAS_TRINAMIC && HAS_LCD_MENU
        init_tmc_section();
      #endif
      return success;
    }
    reset();
    #if ENABLED(EEPROM_AUTO_INIT)
      (void)save();
      SERIAL_ECHO_MSG("EEPROM Initialized");
    #endif
    return true;
  }

[marcio-ao]

Actually, it probably needs to be done in case of failure as well:

  bool MarlinSettings::load() {
    if (validate()) {
      const bool success = _load();
      #if ENABLED(EXTENSIBLE_UI)
        ExtUI::onConfigurationStoreRead(success);
      #endif
      #if HAS_TRINAMIC && HAS_LCD_MENU
        init_tmc_section();
      #endif
      return success;
    }
    reset();
    #if ENABLED(EEPROM_AUTO_INIT)
      (void)save();
      SERIAL_ECHO_MSG("EEPROM Initialized");
    #endif
    #if HAS_TRINAMIC && HAS_LCD_MENU
        init_tmc_section();
    #endif
    return true;
  }

[marcio-ao]

Okay, third revision, here is what I am leaning towards:

In "configuration_store.cpp", replace "bool MarlinSetting::load()" with:

  bool MarlinSettings::load() {
    bool success = validate();
    if (success)
      success = _load();
    else {
      reset();
      #if ENABLED(EEPROM_AUTO_INIT)
        (void)save();
        SERIAL_ECHO_MSG("EEPROM Initialized");
      #endif
    }
    #if ENABLED(EXTENSIBLE_UI)
      ExtUI::onConfigurationStoreRead(success);
    #endif
    #if HAS_TRINAMIC && HAS_LCD_MENU
      init_tmc_section();
    #endif
    return success;
  }

In "ultralcd.cpp", remove the following from void MarlinUI::init():

  #if HAS_TRINAMIC && HAS_LCD_MENU
    init_tmc_section();
  #endif

[teemuatlut]

Perhaps the whole TMC init configuration should be moved to its own section. The overall order should be something along lines of

1. Init UART/SPI
2. Configure static driver parameters
3. Configure variables that are EEPROM supported
4. Init TMC LCD <==> Copy some of the values into shadow variables

[marcio-ao]

My only concern with my above suggestion is that now it means init_tmc_section will be called at startup, but also anytime the user runs M501. Presumably this would need to happen anyway, to update the shadow variables, but I don't know how this was handled before.

[teemuatlut]

I never really liked having the initialization tied to M50x calls. Restoring the config doesn't need to send all the configuration parameters to the driver, just the ones that can be modified with gcodes. The steps 2, 3 and 4 also need to happen with Power::power_on because the settings don't "stick" if there is no VMOT power.

[marcio-ao]

@teemuatlut: Just a thought, rather than keeping shadow variables and having to deal with keeping their state correct, couldn't you just copy them on entry to TMC menu? The advantage of that is that the correct settings will always used in the LCD interface, regardless of whether some other code changed the state elsewhere.

This is the approach I take in my ExtUI. I keep no copies of the machine state, except when the enters a screen where there that setting needs to be changed.

[teemuatlut]

Sure, that would be ideal as I believe that would save a bit of SRAM, but I really dislike dealing with the monster that is the LCD section of Marlin and the macros needed references to variables. I didn't think of using local variables or just didn't spend the time to figure it out.

EDIT: Would that also mean the variables are read from the drivers on each LCD update cycle when the menu is selected?

[SISLANGER]

@marcio-ao is that the current fix revision 3

[marcio-ao]

I never really liked having the initialization tied to M50x calls. Restoring the config doesn't need to send all the configuration parameters to the driver, just the ones that can be modified with gcodes.

I agree. In a way, it seems like the TMC drivers have the same issues that I ran into with the graphics chip. They are some external bits of hardware with their own settings registers that need to be copied from the EEPROM. I suggest that "main.cpp" do the following:

• As early as possible, initialize the TMC drivers with default values.
• Call ui.init()
• Call settings.load();
• From within Configuration_store.cpp, load whatever values are read from EEPROM into TMC chips as well as the shadow variables.

EDIT: Would that also mean the variables are read from the drivers on each LCD update cycle when the menu is selected?

Ideally these values would be copied only when transitioning into and out of the menus, although the UltraLCD code doesn't make it easy to do this, since it lacks entry and exit routines. In my ExtUI, I explicitly have "onEntry" and "onExit" events where I shuffle things in and out of shadow variables.

[marcio-ao]

How about keeping things simple. For now, we could change `Marlin.cpp' to have the following init sequence:

  // UI must be initialized before EEPROM
  // (because EEPROM code calls the UI).
  ui.init();
  ui.reset_status();

  ...

  // Load data from EEPROM if available (or use defaults)
  // This also updates variables in the planner, elsewhere
  (void)settings.load();

  #if HAS_TRINAMIC && HAS_LCD_MENU
    init_tmc_section();
  #endif

I feel like this restores things to as closely as they were to prior to commit 15357af67ceb74b14606eba9fbb75d20914f8909 without introducing any more corner cases to test. Thoughts?