Marlin: [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?

[teemuatlut]

I'm fine with patching things for now and making a proper better thought-out solution later.

[marcio-ao]

I'll work on a PR, since I broke it :)

[tpruvot]

oh, about that...
ui.reset_status() is supposed to show "3D Printer Ready" on the LCD

settings.load() later can display some data on lcd and erase the Ready status

[SISLANGER]

// 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

causing identifier "SENSITIVE_PINS" is undefined

[marcio-ao]

Can you include the entire error message?

[SISLANGER]

define SENSITIVE_PINS { _X_PINS _Y_PINS _Z_PINS _X2_PINS _Y2_PINS _Z2_PINS _Z3_PINS _Z_PROBE _E0_PINS _E1_PINS _E2_PINS _E3_PINS _E4_PINS _E5_PINS _BED_PINS _H0_PINS _H1_PINS _H2_PINS _H3_PINS _H4_PINS _H5_PINS _PS_ON _HEATER_BED _FAN0 _FAN1 _FAN2 _FANC HAL_SENSITIVE_PINS }

Expands to:

{ _X_PINS _Y_PINS _Z_PINS _X2_PINS _Y2_PINS _Z2_PINS _Z3_PINS _Z_PROBE _E0_PINS _E1_PINS _E2_PINS _E3_PINS _E4_PINS _E5_PINS _BED_PINS _H0_PINS _H1_PINS _H2_PINS _H3_PINS _H4_PINS _H5_PINS _PS_ON _HEATER_BED _FAN0 _FAN1 _FAN2 _FANC HAL_SENSITIVE_PINS }
identifier "SENSITIVE_PINS" is undefined

[marcio-ao]

@SISLANGER: That does not seem related to the Trinamic change we have been discussing. Did anything else change? Or did you pull new code?

[SISLANGER]

it comes up as soon as I paste in

if HAS_TRINAMIC && HAS_LCD_MENU

init_tmc_section();

endif

[gloomyandy]

@SISLANGER please make sure you are using the latest version of Marlin so that everyone is in the same state.

[marcio-ao]

@SISLANGER: Let's see if the Travis build process catches this in the PR. I'm not seeing it in our local builds.

[SISLANGER]

OK, I will try an update also

[marcio-ao]

oh, about that...
ui.reset_status() is supposed to show "3D Printer Ready" on the LCD
settings.load() later can display some data on lcd and erase the Ready status

@tpruvot: I moved ui.reset_status() back to it's original location, but I am a little bit torn by this. Having something like "Default settings loaded" instead of "Printer ready" would be very helpful in the cases in which the EEPROM validation failed, but unfortunately this can't happen if the status is reset. Kind of annoying.

[tpruvot]

may need some status/state variable

[SISLANGER]

OK downloaded from https://github.com/MarlinFirmware/Marlin/tree/211563e533f418974577a6e7a36a156ddea64af2

which I believe is the very latest version. Just uploading without the fix just in case. Will then try fix

[SISLANGER]

I only wanted to print an 8 minute file LOL that was 7 hours ago

[InsanityAutomation]

@SISLANGER Welcome to bleeding edge! :-D

[Patag]

I just tested the fix and it runs fine. But X and Y axis display 101 whereas they are set to 100 in configuration_adv.h. Strange no ? I've previously use "Init EEPROM" menu, but no changes.

[marcio-ao]

@tpruvot: What message do you see on the LCD if ui.reset_status() just after ui.init? I am not seeing any messages other than the "Printer ready" message.

[tpruvot]

its only for those with the eeprom.dat on the sd...

that write the filename on read and write (could be skipped)

but the fact is... displaying the printer is ready on start of the init seems kinda wrong

[marcio-ao]

@tpruvot: It simplifies the code a lot if "Printer ready" is printed first, then if any of the initialization code runs into a problem, it simply replaces the message with something else. It all would happen fast enough so the user would only see the very last thing that went wrong, or "Printer ready" if everything went fine.

its only for those with the eeprom.dat on the sd... that write the filename on read and write (could be skipped)

I see. I guess we have to find where that filename is printed and see whether it really needs to be there.

[tpruvot]

https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/sd/cardreader.cpp#L475

[marcio-ao]

@tpruvot: And you are using an STM32F, I presume?

Removing that call would the message from everywhere where openFile is called, which may not be desirable. But there are only a handful of places where openFile is called, so it would simply be a matter of moving that LCD messages to the places that needs it. Personally, I like that approach better, since writing a message to the LCD seems like an odd side effect for a low-level function like openFile

[tpruvot]

yes, its common to any sdcard openfile as you can see

[SISLANGER]

Ok it works of sort, I have to M502, M500 to get in loaded in, but one of the figures for hybrid threshold is wrong says 47 should be 50. But I can live with that one for a while.

[marcio-ao]

@tpruvot: Yes, these are the places I see with openFile:

1. Marlin/src/HAL/HAL_STM32F1/persistent_store_sdcard.cpp
2. Marlin/src/sd/cardreader.h
3. Marlin/src/sd/cardreader.cpp
4. Marlin/src/gcode/sdcard/M23.cpp
5. Marlin/src/gcode/sdcard/M28_M29.cpp
6. Marlin/src/gcode/sdcard/M32.cpp

Not very many places. It won't be too much trouble to move the LCD message to all the places except the "persistent_store_sdcard.cpp", which would at least solve the immediate problem you were seeing with "Ready" message.

Ok it works of sort, I have to M502, M500 to get in loaded in, but one of the figures for hybrid threshold is wrong says 47 should be 50. But I can live with that one for a while.

@SISLANGER: That's peculiar, I feel like we have multiple issues floating around. May take a bit to sort through them all.

[SISLANGER]

@marcio-ao no worries as least I am not having to put them in every time I reset / turned off/on any more. Thank you for your hard work greatly appreciated, it really is.

[ghost]

I guess this is a temporary fudge to get things working again ? ... (is hoping)

Not having shadow variables is the proper way to go - what you see is what you get.

But yes, thank you @marcio-ao for your hard work. These things happen - all the time.

[teemuatlut]

hybrid threshold is wrong says 47 should be 50

Some of the settings get discretized when they get converted from one form to another.
When you ask for 50mm/s, the library calculates the closest corresponding TSTEP value and sends that to the driver. Then you get another level of error when converting back when you want to view the value.

[ghost]

Do they get rounded up/down as needed @teemuatlut rather than say just rounded down ?

[InsanityAutomation]

Slight side-bar @marcio-ao are you coming to RAPID next week?

[teemuatlut]

@doggyfan Rounded down as all the inaccuracies would be from division with integers.

https://github.com/MarlinFirmware/Marlin/blob/211563e533f418974577a6e7a36a156ddea64af2/Marlin/src/feature/tmc_util.h#L60-L62

[ghost]

okey doke.

Could you not do ..

constexpr uint16_t _tmc_thrs(const uint16_t msteps, const int32_t thrs, const uint32_t spmm) { return (12650000UL * msteps + (12650000UL / 2)) / (256 * thrs * spmm); }

To round up/down as needed (as an example) ?

[ghost]

sorry lol, this is what comes of DSP work for too long.

[SISLANGER]

The strange thing is it worked out the same figure correctly for the E1 so can't understand why it got it wrong for the second extruder.

[teemuatlut]

I took another look at your M122 earlier and that 50.29 pwm threshold should round to 50 in the LCD.
Maybe this will be fixed if I use local variables for the LCD...
Also tmc_init shouldn't set hybrid threshold as that is set from config store. Ugh...

[marcio-ao]

Slight side-bar @marcio-ao are you coming to RAPID next week?

@InsanityAutomation: Sadly, no. No travel scheduled for me :(

[gloomyandy]

@SISLANGER your m122 output showed 30.00 and 50.29 so are the settings the same? Looking at your config files you have different steps/mm for E and E1, so I would expect the threshold calculations to be different.

[SISLANGER]

@gloomyandy
Different steps yes (Different extruder types), 30.00 was a typo in the file changed it to 50 and E1 dropped to 47.

[marcio-ao]

@tpruvot: The HAL_STM32F1 persistent store code is doing something pretty nasty:

  bool PersistentStore::access_start() {
    if (!card.isDetected()) return false;
    int16_t bytes_read = 0;
    constexpr char eeprom_zero = 0xFF;
    card.openFile(eeprom_filename, true);
    bytes_read = card.read(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    if (bytes_read < 0) return false;
    for (; bytes_read < HAL_STM32F1_EEPROM_SIZE; bytes_read++)
      HAL_STM32F1_eeprom_content[bytes_read] = eeprom_zero;
    card.closefile();
    return true;
  }

  bool PersistentStore::access_finish() {
    if (!card.isDetected()) return false;
    card.openFile(eeprom_filename, false);
    int16_t bytes_written = card.write(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    card.closefile();
    return (bytes_written == HAL_STM32F1_EEPROM_SIZE);
  }

It is calling openFile, which is used by Marlin for printing GCODE, to write EEPROM data which has nothing to do with GCODE. I can see at least one instance where this would fail spectacularly: if a print is happening from the SD card and M500, M501 or M502 is issued either from file or from a terminal, the file will be closed, interrupting the print, and possibly worse.

Given that this code is pretty broken, I am fairly confident I won't break it more by suggesting that it be replaced with this instead:

  bool PersistentStore::access_start() {
    if (!card.isDetected()) return false;
    int16_t bytes_read = 0;
    constexpr char eeprom_zero = 0xFF;
    SdFile file;
    file.open(eeprom_filename, O_READ);
    bytes_read = file.read(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    if (bytes_read < 0) return false;
    for (; bytes_read < HAL_STM32F1_EEPROM_SIZE; bytes_read++)
      HAL_STM32F1_eeprom_content[bytes_read] = eeprom_zero;
    file.closefile();
    return true;
  }

  bool PersistentStore::access_finish() {
    if (!card.isDetected()) return false;
    SdFile file;
    file.open(eeprom_filename, O_CREAT | O_APPEND | O_WRITE | O_TRUNC)
    int16_t bytes_written = file.write(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    file.closefile();
    return (bytes_written == HAL_STM32F1_EEPROM_SIZE);
  }

The difference here is it will use low level routines to open a new file and close it, without (hopefully) wrecking Marlin's idea of what file is currently being printed. It also has the nice side effect of not printing anything to the LCD.

@tpruvot: Do you mind testing this modification and see if it works okay? If so, I can add it to my PR.

[tpruvot]

I didnt touch this part, it was just reindented during the last PR
else i see a O_APPEND which should not be there to write a new file..
and the weird loop with 0xff can be replaced by memset (or at least a direct = 0xFF)

[tpruvot]

rmm, doesnt seems to work...
what i tried (but doesnt load the settings) :

  static char eeprom_filename[] = "eeprom.dat";

  bool PersistentStore::access_start() {
    if (!card.isDetected()) return false;

    SdBaseFile file;
    if (!file.open(eeprom_filename, O_RDONLY))
      return false;

    int16_t bytes_read = file.read(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    if (bytes_read < 0) return false;
    for (; bytes_read < HAL_STM32F1_EEPROM_SIZE; bytes_read++)
      HAL_STM32F1_eeprom_content[bytes_read] = 0xFF;
    file.close();
    return true;
  }

  bool PersistentStore::access_finish() {
    if (!card.isDetected()) return false;

    SdBaseFile file;
    if (!file.open(eeprom_filename, O_CREAT | O_WRITE | O_TRUNC | O_APPEND))
      return false;

    int16_t bytes_written = file.write(HAL_STM32F1_eeprom_content, HAL_STM32F1_EEPROM_SIZE);
    //file.sync(); made in close
    file.close();
    return (bytes_written == HAL_STM32F1_EEPROM_SIZE);
  }

[tpruvot]

I think the issue is the missing "current folder" object named root in card object, but private.

Damn C++, seems weird to me to see code like that in SoCs :p

[tpruvot]

Ok, made it work, will do a PR with that (#14012)

[marcio-ao]

i see a O_APPEND which should not be there to write a new file..

@tpruvot: Yes, I figured as much, but it was in the original openFile so I kept it.

I think the issue is the missing "current folder" object named root in card object, but private.

Interesting. I thought there was a version of open that did not require the root object:

bool open(const char* path, uint8_t oflag = O_READ);

Although what you ended up doing should work fine as well.

[jmdearras]

Latest pull (5/16) all work except storing sensorless endstop values. Always restores as 0

[teemuatlut]

You mean with M501 or M502?
Does it affect LCD only?
What does M122 say?

[jmdearras]

I am testing with LCD, I will try serial this weekend. However, the endstop action was consistent with 0.

[AndyF211]

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(); }
^~~

I've been getting this same warning and I'm having the same issue on my SKR v1.3 on my Ender 3 (starting compiling for this board using Bugfix from 7 May). If I save the settings manually from the LCD, the settings seem to stay put. I also noticed that my Jerk settings on the LCD always revert back to 20 when I have them set to 10 in the firmware. Tried the latest Bugfix from 2 days ago and I still get the warning.

The 'LCD not being updated properly' and this 'warning' may not be related, but the warning probably needs it's own issue thread. The warning mentions the old 'TMC2208Stepper' library which I though I read somewhere was replaced by the new TMCStepper library (possible code remnant from the old library?). I installed the older library but I still get this warning.

[teemuatlut]

The TMC2208Stepper is a class name, not the library.
What drivers have you enabled?

[AndyF211]

Goes to show how much I know! : )

I've got the following:

define X_DRIVER_TYPE TMC2208

define Y_DRIVER_TYPE TMC2208

define Z_DRIVER_TYPE TMC2208

define E0_DRIVER_TYPE LV8729

[teemuatlut]

Monitoring enabled?
SW or HW Serial?

Or maybe it's just easier if you post your configs so I can compile =)

[AndyF211]

Here they are. I know you must be extremely busy nailing down other issues, so I really appreciate you taking the time.

These configs are for yesterday's release of the 2.0.X BugFix (transferred my configs). I was also getting the same error using the one dated 5/7/19. Aside from the aforementioned warning, I'm also now getting another warning that 'may' be related to the recent changes to the BLTOUCH code, but as we've already extablished, I'm no coder! : )

Thanks again, and thanks for all you do for the community!
Marlin (5-18-19).zip

[sreichholf]

This issue was fixed with #14008 for me (which has been merged on May 17).

[AndyF211]

I just compiled the latest Marlin 2.0.X Bugfix, (dated 22 May) and I still get the warning. I noticed a new version of Teemuatlu's TMC library (0.3.4), so I updated that before compiling with Atom (config attached). My control board is a 'BigTreeTech SKR V1.3'. I've got 2208's on X, Y, Z, and a LV8729 on E. Am I doing something wrong in my configs? Should I be concerned that my 2208's won't perform as expected based on this warning or should I just blow it off?

Here's the warning again:

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(); }
^~~

Config.zip

[teemuatlut]

It's an unused function warning and all it does it tell the developer that you've got a piece of code that is never called and thus unnecessary.

Should be fixed in #14074 as well as keeping the LCD variables up to date.

[AndyF211]

Copy that! Thanks!

[teemuatlut]

@SISLANGER Has your issue been resolved by the recent fixes?

[SISLANGER]

@teemuatlut thank you what's is the best way to update in platformio, rather than keep copying details from configuation.h, configuration_adh.h, pins_BIGTREE_SKR_V1.3.h

[SISLANGER]

@teemuatlut don't worry it looks like the comparrision method I use is still probably the best. Its just time consuming, when you got a lot on.

[teemuatlut]

I'd say it's largely a matter of opinion.
The way I do it is create a git commit with my settings and then do a fetch and rebase to update from the upstream code. But I know this method is likely not for everyone.

[github-actions[bot]]

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.