Refactor original solution from @leptun

- rename stall*guard to StallGuard (match name with the vendor)
- separate TMC2130 module from EEPROM (they do not need to know about each other at all)
- separate SGTHRS settings from motion - moved to globals like all other "global" parameters
- improved EEPROM storage for SGTHRS

src/config/config.h

@@ -83,7 +83,7 @@ static constexpr U_mm couplerToBowden = 3.5_mm; /// 3.5_mm /// FINDA Coupler scr
 // @@TODO this is very tricky - the same MMU, same PTFE,
 // just another piece of PLA (probably having more resistance in the tubes)
 // and we are at least 40mm off! It looks like this really depends on the exact position
-// We'll probably need to check for stallguard while pushing the filament to avoid ginding the filament
+// We'll probably need to check for StallGuard while pushing the filament to avoid ginding the filament
 static constexpr U_mm defaultBowdenLength = 427.0_mm; ///< ~427.0_mm - Default Bowden length. @TODO Should be stored in EEPROM. 392 a 784
 static constexpr U_mm minimumBowdenLength = 341.0_mm; ///< ~341.0_mm - Minimum bowden length. @TODO Should be stored in EEPROM.
 static constexpr U_mm maximumBowdenLength = 792.0_mm; ///< ~792.0_mm - Maximum bowden length. @TODO Should be stored in EEPROM.

src/hal/avr/eeprom.cpp

@@ -11,6 +11,11 @@ uint8_t EEPROM::ReadByte(EEPROM::addr_t addr) {
     return eeprom_read_byte((const uint8_t *)addr);
 }
 
+uint8_t EEPROM::ReadByte(EEPROM::addr_t addr, uint8_t defaultValue) {
+    uint8_t v = ReadByte(addr);
+    return v == 0xff ? defaultValue : v;
+}
+
 void EEPROM::UpdateByte(EEPROM::addr_t addr, uint8_t value) {
     eeprom_update_byte((uint8_t *)addr, value);
 }

src/hal/eeprom.h

@@ -15,6 +15,11 @@ public:
     static void UpdateByte(addr_t addr, uint8_t value);
     static uint8_t ReadByte(addr_t addr);
 
+    /// Convenience function to read a 1-byte value from EEPROM and check for unitialized EEPROM cells.
+    /// @returns 1-byte value read from the EEPROM.
+    /// In case the EEPROM has a default value at @addr, this function returns @defaultValue
+    static uint8_t ReadByte(addr_t addr, uint8_t defaultValue);
+
     static void WriteWord(addr_t addr, uint16_t value);
     static void UpdateWord(addr_t addr, uint16_t value);
     static uint16_t ReadWord(addr_t addr);

src/hal/progmem.h

@@ -22,5 +22,14 @@ static inline uint16_t read_word(const uint16_t *addr) {
 #endif
 }
 
+/// read a 8bit byte from PROGMEM
+static inline uint8_t read_byte(const uint8_t *addr) {
+#ifndef __AVR__
+    return *addr;
+#else
+    return (uint8_t)pgm_read_byte(addr);
+#endif
+}
+
 } // namespace progmem
 } // namespace hal

src/hal/tmc2130.cpp

@@ -1,7 +1,6 @@
 /// @file tmc2130.cpp
 #include "tmc2130.h"
 #include "../config/config.h"
-#include "../modules/permanent_storage.h"
 #include "../debug.h"
 
 namespace hal {
@@ -44,7 +43,7 @@ bool TMC2130::Init(const MotorParams &params, const MotorCurrents &currents, Mot
     WriteRegister(params, Registers::TPOWERDOWN, 0);
 
     ///Stallguard parameters
-    SetSGTHRS(params, mps::AxisSGTHRS::get((mm::Axis)params.axis));
+    SetSGTHRS(params);
     WriteRegister(params, Registers::TCOOLTHRS, config::tmc2130_coolStepThreshold);
 
     ///Write stealth mode config and setup diag0 output
@@ -79,8 +78,8 @@ void TMC2130::SetCurrents(const MotorParams &params, const MotorCurrents &curren
     WriteRegister(params, Registers::IHOLD_IRUN, ihold_irun);
 }
 
-void TMC2130::SetSGTHRS(const MotorParams &params, uint8_t sgthrs) {
+void TMC2130::SetSGTHRS(const MotorParams &params) {
-    uint32_t tmc2130_coolConf = (((uint32_t)sgthrs) << 16U);
+    uint32_t tmc2130_coolConf = (((uint32_t)params.sg_thrs) << 16U);
     WriteRegister(params, Registers::COOLCONF, tmc2130_coolConf);
 }
 

src/hal/tmc2130.h

@@ -23,7 +23,7 @@ struct MotorParams {
     bool dirOn; ///< forward direction
     gpio::GPIO_pin csPin; ///< CS pin
     gpio::GPIO_pin stepPin; ///< step pin
-    gpio::GPIO_pin sgPin; ///< stallguard pin
+    gpio::GPIO_pin sgPin; ///< StallGuard pin
     config::MRes mRes; ///< microstep resolution
     int8_t sg_thrs;
     uint8_t axis;
@@ -92,8 +92,11 @@ public:
     /// Set the current motor currents
     void SetCurrents(const MotorParams &params, const MotorCurrents &currents);
 
-    /// Set stallguard threshold
+    /// Set StallGuard threshold
-    void SetSGTHRS(const MotorParams &params, uint8_t sgthrs);
+    /// New SGTHRS must be already set in @params
+    /// Beware - there are no checks, new value is written into the TMC register immediately.
+    /// It is advised to prefer setting the SGTHRS via the Init() method.
+    void SetSGTHRS(const MotorParams &params);
 
     /// Return enabled state
     const bool Enabled() const {

src/logic/error_codes.h

@@ -37,7 +37,7 @@ enum class ErrorCode : uint_fast16_t {
     HOMING_FAILED = 0x8007, ///< generic homing failed error - always reported with the corresponding axis bit set (Idler or Selector) as follows:
     HOMING_SELECTOR_FAILED = HOMING_FAILED | TMC_SELECTOR_BIT, ///< E32903 the Selector was unable to home properly - that means something is blocking its movement
     HOMING_IDLER_FAILED = HOMING_FAILED | TMC_IDLER_BIT, ///< E33031 the Idler was unable to home properly - that means something is blocking its movement
-    STALLED_PULLEY = HOMING_FAILED | TMC_PULLEY_BIT, ///< E32839 for the Pulley "homing" means just stallguard detected during Pulley's operation (Pulley doesn't home)
+    STALLED_PULLEY = HOMING_FAILED | TMC_PULLEY_BIT, ///< E32839 for the Pulley "homing" means just StallGuard detected during Pulley's operation (Pulley doesn't home)
 
     FINDA_VS_EEPROM_DISREPANCY = 0x8008, ///< E32776 FINDA is pressed but we have no such record in EEPROM - this can only happen at the start of the MMU and can be resolved by issuing an Unload command
 

src/logic/feed_to_bondtech.cpp

@@ -64,7 +64,7 @@ bool FeedToBondtech::Step() {
             mm::motion.AbortPlannedMoves(); // stop pushing filament
             GoToPushToNozzle();
             //        } else if (mm::motion.StallGuard(mm::Pulley)) {
-            //            // stall guard occurred during movement - the filament got stuck
+            //            // StallGuard occurred during movement - the filament got stuck
             //            state = PulleyStalled;
         } else if (mm::motion.QueueEmpty()) { // all moves have been finished and the fsensor didn't switch on
             state = Failed;

src/modules/globals.cpp

@@ -2,6 +2,7 @@
 #include "globals.h"
 #include "../config/config.h"
 #include "permanent_storage.h"
+#include "../hal/progmem.h"
 
 namespace modules {
 namespace globals {
@@ -76,5 +77,15 @@ void Globals::SetMotorsMode(bool stealth) {
     // @@TODO store into EEPROM
 }
 
+uint8_t Globals::StallGuardThreshold(config::Axis axis) const {
+    // @@TODO this is not nice but we need some simple way of indexing the default SGTHRs
+    static constexpr uint8_t defaultAxisSGTHRs[3] PROGMEM = { config::pulley.sg_thrs, config::selector.sg_thrs, config::idler.sg_thrs };
+    return mps::AxisTMCSetup::get(axis, hal::progmem::read_byte(defaultAxisSGTHRs + axis));
+}
+
+void Globals::SetStallGuardThreshold(config::Axis axis, uint8_t sgthrs){
+    mps::AxisTMCSetup::set(axis, sgthrs); // store the value in EEPROM
+}
+
 } // namespace globals
 } // namespace modules

src/modules/globals.h

@@ -90,6 +90,10 @@ public:
     void ResetIdlerFeedrate() { idlerFeedrate_deg_s = config::idlerFeedrate.v; }
     void SetIdlerFeedrate_deg_s(uint16_t idlerFR_deg_s) { idlerFeedrate_deg_s = idlerFR_deg_s; }
 
+    /// @returns current StallGuard threshold for an axis
+    uint8_t StallGuardThreshold(config::Axis axis) const;
+    /// Stores the new StallGuard threshold for an axis into EEPROM (does not affect the current state of TMC drivers at all)
+    void SetStallGuardThreshold(config::Axis axis, uint8_t sgthrs);
 private:
     /// Sets the active slot, usually after some command/operation.
     /// Also updates the EEPROM records accordingly

src/modules/motion.cpp

@@ -2,6 +2,7 @@
 #include "motion.h"
 #include "../panic.h"
 #include "../debug.h"
+#include "permanent_storage.h"
 
 // TODO: use proper timer abstraction
 #ifdef __AVR__
@@ -56,6 +57,8 @@ bool Motion::InitAxis(Axis axis) {
     // disable the axis and re-init the driver: this will clear the internal
     // StallGuard data as a result without special handling
     Disable(axis);
+
+    // Init also applies the currently pre-set StallGuard threshold into the TMC driver
     return axisData[axis].drv.Init(axisParams[axis].params, axisParams[axis].currents, axisParams[axis].mode);
 }
 
@@ -83,6 +86,10 @@ void Motion::StallGuardReset(Axis axis) {
     axisData[axis].drv.ClearStallguard();
 }
 
+void Motion::PlanStallGuardThreshold(config::Axis axis, uint8_t sg_thrs) {
+    axisParams[axis].params.sg_thrs = sg_thrs;
+}
+
 void Motion::PlanMoveTo(Axis axis, pos_t pos, steps_t feed_rate, steps_t end_rate) {
     dbg_logic_fP(PSTR("Move axis %d to %u"), axis, pos);
 

src/modules/motion.h

@@ -96,12 +96,16 @@ public:
     /// Set the same mode of TMC/motors operation for all axes. @see SetMode
     void SetMode(MotorMode mode);
 
-    /// @returns true if a stall guard event occurred recently on the axis
+    /// @returns true if a StallGuard event occurred recently on the axis
     bool StallGuard(Axis axis);
 
-    /// clear stall guard flag reported on an axis
+    /// clear StallGuard flag reported on an axis
     void StallGuardReset(Axis axis);
 
+    /// Sets (plans) StallGuard threshold for an axis (basically the higher number the lower sensitivity)
+    /// The new SGTHRS value gets applied in Init(), it is _NOT_ written into the TMC immediately in this method.
+    void PlanStallGuardThreshold(Axis axis, uint8_t sg_thrs);
+
     /// Enqueue a single axis move in steps starting and ending at zero speed with maximum
     /// feedrate. Moves can only be enqueued if the axis is not Full().
     /// @param axis axis affected

src/modules/movable_base.cpp

@@ -10,6 +10,9 @@ void MovableBase::PlanHome() {
     InvalidateHoming();
 
     // switch to normal mode on this axis
+
+    // Update StallGuard threshold
+    mm::motion.PlanStallGuardThreshold(axis, mg::globals.StallGuardThreshold(axis));
     mm::motion.InitAxis(axis);
     mm::motion.SetMode(axis, mm::Normal);
     mm::motion.StallGuardReset(axis);
@@ -21,6 +24,7 @@ void MovableBase::PlanHome() {
 }
 
 MovableBase::OperationResult MovableBase::InitMovement() {
+    mm::motion.PlanStallGuardThreshold(axis, mg::globals.StallGuardThreshold(axis));
     if (motion.InitAxis(axis)) {
         PrepareMoveToPlannedSlot();
         state = Moving;

src/modules/permanent_storage.cpp

@@ -360,14 +360,11 @@ void DriveError::setH(uint8_t highByte) {
     ee::EEPROM::UpdateByte(EEOFFSET(eepromBase->eepromDriveErrorCountH), highByte - 1);
 }
 
-uint8_t AxisSGTHRS::get(mm::Axis axis) {
+uint8_t AxisTMCSetup::get(config::Axis axis, uint8_t defaultValue) {
-    uint8_t sg_thrs = ee::EEPROM::ReadByte(EEOFFSET(eepromBase->sg_thrs[axis]));
+    return ee::EEPROM::ReadByte(EEOFFSET(eepromBase->sg_thrs[axis]), defaultValue);
-    if (sg_thrs & 0x80)
-        sg_thrs = mm::axisParams[axis].params.sg_thrs;
-    return sg_thrs;
 }
 
-void AxisSGTHRS::set(mm::Axis axis, uint8_t val) {
+void AxisTMCSetup::set(config::Axis axis, uint8_t val) {
     ee::EEPROM::UpdateByte(EEOFFSET(eepromBase->sg_thrs[axis]), val);
 }
 

src/modules/permanent_storage.h

@@ -1,7 +1,7 @@
 /// @file permanent_storage.h
 #pragma once
 #include "../hal/eeprom.h"
-#include "motion.h"
+#include "../config/axis.h"
 
 namespace modules {
 
@@ -99,16 +99,13 @@ private:
     static void setH(uint8_t highByte);
 };
 
-/// @brief Read and increment drive errors
+/// Axis TMC persistent setup
-///
+class AxisTMCSetup {
-/// (Motor power rail voltage loss)
-class AxisSGTHRS {
 public:
-    static uint8_t get(mm::Axis axis);
+    static uint8_t get(config::Axis axis, uint8_t defaultValue);
-    static void set(mm::Axis axis, uint8_t val);
+    static void set(config::Axis axis, uint8_t val);
 };
 
-
 } // namespace permanent_storage
 } // namespace modules
 

src/registers.cpp

@@ -28,6 +28,12 @@
 
   __Bold = Status__
 
+  ---------------------------------------------------------------------------------
+  Access type:
+  - Read: the register can be only read
+  - Write: the register can be read and written to. The written change is not persistent (applies only until reset of the MMU)
+  - Write Persistent: the register can be read and written to and the written values is stored in EEPROM (persistent across restarts)
+
   ---------------------------------------------------------------------------------
   How can you use the M707/M708 gcodes?
   - Serial terminal like Putty.
@@ -50,7 +56,7 @@
 | 0x01h 01 | uint8    | project_minor              | 00h 0        | ffh 255     | Project Minor Version                    | Read only    | M707 A0x01 | N/A
 | 0x02h 02 | uint8    | project_revision           | 00h 0        | ffh 255     | Project Revision                         | Read only    | M707 A0x02 | N/A
 | 0x03h 03 | uint8    | project_build_number       | 00h 0        | ffh 255     | Project Build Number                     | Read only    | M707 A0x03 | N/A
-| 0x04h 04 | uint16   | MMU_errors                 | 0000h 0      | ffffh 65535 | MMU Errors                               | Read / Write | M707 A0x04 | M708 A0x04 Xnnnn
+| 0x04h 04 | uint16   | MMU_errors                 | 0000h 0      | ffffh 65535 | MMU Errors                               | Read / Write Persistent | M707 A0x04 | M708 A0x04 Xnnnn
 | 0x05h 05 | uint8    | Current_Progress_Code      | ffh 255      | ffh 255     | Emtpy                                    | Read only    | M707 A0x05 | N/A
 | ^        | ^        | ^                          | 00h 0        | ^           | OK                                       | ^            | ^          | ^
 | ^        | ^        | ^                          | 01h 1        | ^           | EngagingIdler                            | ^            | ^          | ^
@@ -146,9 +152,9 @@
 | 0x14h 20 | uint16   | Pulley_slow_feedrate       | 0000h 0      | 0014h 20    | unit mm/s                                | Read / Write | M707 A0x14 | M708 A0x14 Xnnnn
 | 0x15h 21 | uint16   | Selector_homing_feedrate   | 0000h 0      | 001eh 30    | unit mm/s                                | Read (Write) | M707 A0x15 | (M708 A0x15 Xnnnn)
 | 0x16h 22 | uint16   | Idler_homing_feedrate      | 0000h 0      | 0109h 265   | unit deg/s                               | Read (Write) | M707 A0x16 | (M708 A0x16 Xnnnn)
-| 0x17h 23 | uint8    | Pulley_sg_thrs__R          | 00h 0        | 08h 8       |                                          | Read (Write) | M707 A0x17 | M708 A0x17 Xnn
+| 0x17h 23 | uint8    | Pulley_sg_thrs__R          | 00h 0        | 08h 8       |                                          | Read / Write Persistent | M707 A0x17 | M708 A0x17 Xnn
-| 0x18h 24 | uint8    | Selector_sg_thrs_R         | 00h 0        | 03h 3       |                                          | Read (Write) | M707 A0x18 | M708 A0x18 Xnn
+| 0x18h 24 | uint8    | Selector_sg_thrs_R         | 00h 0        | 03h 3       |                                          | Read / Write Persistent | M707 A0x18 | M708 A0x18 Xnn
-| 0x19h 25 | uint8    | Idler_sg_thrs_R            | 00h 0        | 05h 6       |                                          | Read (Write) | M707 A0x19 | M708 A0x19 Xnn
+| 0x19h 25 | uint8    | Idler_sg_thrs_R            | 00h 0        | 05h 6       |                                          | Read / Write Persistent | M707 A0x19 | M708 A0x19 Xnn
 | 0x1ah 26 | uint16   | Get Pulley position        | 0000h 0      | ffffh 65535 | unit mm                                  | Read only    | M707 A0x1a | N/A
 | 0x1bh 27 | uint16   | Set/Get_Selector_slot      | 0000h 0      | ffffh 65535 | unit slot [0-4/5] 5=park pos             | Read / Write | M707 A0x1b | M708 A0x1b Xn
 | 0x1ch 28 | uint16   | Set/Get_Idler_slot         | 0000h 0      | ffffh 65535 | unit slot [0-4/5] 5=disengaged           | Read / Write | M707 A0x1c | M708 A0x1c Xn
@@ -334,18 +340,18 @@ static const RegisterRec registers[] /*PROGMEM*/ = {
 
     // 0x17 Pulley sg_thrs threshold RW
     RegisterRec(
-        []() -> uint16_t { return mps::AxisSGTHRS::get(mm::Axis::Pulley); },
+        []() -> uint16_t { return mg::globals.StallGuardThreshold(config::Pulley); },
-        [](uint16_t d) { mm::motion.DriverForAxis(mm::Axis::Pulley).SetSGTHRS(mm::axisParams[mm::Axis::Pulley].params, d); mps::AxisSGTHRS::set(mm::Axis::Pulley, d); },
+        [](uint16_t d) { mg::globals.SetStallGuardThreshold(config::Pulley, d); },
         1),
     // 0x18 Selector sg_thrs RW
     RegisterRec(
-        []() -> uint16_t { return mps::AxisSGTHRS::get(mm::Axis::Selector); },
+        []() -> uint16_t { return mg::globals.StallGuardThreshold(mm::Axis::Selector); },
-        [](uint16_t d) { mm::motion.DriverForAxis(mm::Axis::Selector).SetSGTHRS(mm::axisParams[mm::Axis::Selector].params, d); mps::AxisSGTHRS::set(mm::Axis::Selector, d); },
+        [](uint16_t d) { mg::globals.SetStallGuardThreshold(mm::Axis::Selector, d); },
         1),
     // 0x19 Idler sg_thrs RW
     RegisterRec(
-        []() -> uint16_t { return mps::AxisSGTHRS::get(mm::Axis::Idler); },
+        []() -> uint16_t { return mg::globals.StallGuardThreshold(mm::Axis::Idler); },
-        [](uint16_t d) { mm::motion.DriverForAxis(mm::Axis::Idler).SetSGTHRS(mm::axisParams[mm::Axis::Idler].params, d); mps::AxisSGTHRS::set(mm::Axis::Idler, d); },
+        [](uint16_t d) { mg::globals.SetStallGuardThreshold(mm::Axis::Idler, d); },
         1),
 
     // 0x1a Get Pulley position [mm] R

tests/unit/logic/load_filament/test_load_filament.cpp

@@ -195,7 +195,7 @@ void FailedLoadToFindaResolveManual(uint8_t slot, logic::LoadFilament &lf) {
     // its moves (which is correct), but we don't have enough cycles to home the selector afterwards
     // - basically it will just start homing
     // Moreover, the Idler is to disengage meanwhile, which makes the simulation even harder.
-    // Therefore we just tick the stallguard of the Selector and hope for the best
+    // Therefore we just tick the StallGuard of the Selector and hope for the best
     //
     // With the introduction of dual-side homing, the simulation gets even harder,
     // so let's assume the MMU does its job -> prefer simulating selector homing properly and check the machine's state afterwards

tests/unit/logic/stubs/homing.cpp

@@ -10,7 +10,7 @@
 
 void SimulateIdlerAndSelectorHoming(logic::CommandBase &cb) {
 #if 0
-    // do 5 steps until we trigger the simulated stallguard
+    // do 5 steps until we trigger the simulated StallGuard
     for (uint8_t i = 0; i < 5; ++i) {
         main_loop();
         cb.Step();
@@ -57,7 +57,7 @@ void SimulateIdlerAndSelectorHoming(logic::CommandBase &cb) {
 }
 
 void SimulateIdlerHoming(logic::CommandBase &cb) {
-    // do 5 steps until we trigger the simulated stallguard
+    // do 5 steps until we trigger the simulated StallGuard
     for (uint8_t i = 0; i < 5; ++i) {
         main_loop();
         cb.Step();
@@ -91,7 +91,7 @@ void SimulateIdlerHoming(logic::CommandBase &cb) {
 }
 
 void SimulateSelectorHoming(logic::CommandBase &cb) {
-    // do 5 steps until we trigger the simulated stallguard
+    // do 5 steps until we trigger the simulated StallGuard
     for (uint8_t i = 0; i < 5; ++i) {
         main_loop();
         cb.Step();
@@ -164,7 +164,7 @@ bool SimulateFailedHomeFirstTime(logic::CommandBase &cb) {
         return false;
 
     {
-        // do 5 steps until we trigger the simulated stallguard
+        // do 5 steps until we trigger the simulated StallGuard
         for (uint8_t i = 0; i < 5; ++i) {
             main_loop();
             cb.Step();
@@ -221,7 +221,7 @@ bool SimulateFailedHomeSelectorRepeated(logic::CommandBase &cb) {
         return false;
 
     {
-        // do 5 steps until we trigger the simulated stallguard
+        // do 5 steps until we trigger the simulated StallGuard
         for (uint8_t i = 0; i < 5; ++i) {
             main_loop();
             cb.Step();

tests/unit/logic/stubs/stub_motion.cpp

@@ -36,6 +36,10 @@ void TriggerStallGuard(Axis axis) {
     axes[axis].stallGuard = true;
 }
 
+void Motion::PlanStallGuardThreshold(Axis axis, uint8_t sg_thrs){
+    // do nothing for now
+}
+
 void Motion::PlanMoveTo(Axis axis, pos_t pos, steps_t feed_rate, steps_t end_rate) {
     axes[axis].plannedMoves.push_back(pos);
     if (!axisData[axis].enabled)

tests/unit/modules/stubs/stub_eeprom.cpp

@@ -34,6 +34,11 @@ uint8_t EEPROM::ReadByte(EEPROM::addr_t offset) {
     return EE[offset];
 }
 
+uint8_t EEPROM::ReadByte(EEPROM::addr_t offset, uint8_t defaultValue) {
+    REQUIRE(offset < EE.size());
+    return EE[offset] == 0xff ? defaultValue : EE[offset];
+}
+
 void EEPROM::WriteWord(EEPROM::addr_t offset, uint16_t value) {
     REQUIRE(offset < EE.size() - 1);
     *reinterpret_cast<uint16_t *>(&EE[offset]) = value;