From 02332018ecf05eb798450e8e35005bd97b46bfc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gu=C3=B0ni=20M=C3=A1r=20Gilbert?= Date: Sun, 30 Jul 2023 13:54:05 +0000 Subject: [PATCH] tmc2130: Fix bug with vSense bit and current values If vSense changes at runtime due to Run current being changed. Then we must always shift the Hold current correctly. Whether the vSense is changing 1 -> 0 or 0 ->1 Change in memory (with TMC2130_SERVICE_CODES_M910_M918): Flash: +76 bytes SRAM: 0 bytes --- Firmware/Marlin_main.cpp | 12 ++++---- Firmware/power_panic.cpp | 12 ++++---- Firmware/tmc2130.cpp | 25 +++++++--------- Firmware/tmc2130.h | 62 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index f3c722e52..982fd413c 100644 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -1085,8 +1085,8 @@ void setup() #ifdef TMC2130 if(FarmOrUserECool()) { //increased extruder current (PFW363) - currents[E_AXIS].iRun = TMC2130_CURRENTS_FARM; - currents[E_AXIS].iHold = TMC2130_CURRENTS_FARM; + currents[E_AXIS].setiRun(TMC2130_CURRENTS_FARM); + currents[E_AXIS].setiHold(TMC2130_CURRENTS_FARM); } #endif //TMC2130 @@ -8064,8 +8064,8 @@ Sigma_Exit: } float cur_mA = code_value(); uint8_t val = tmc2130_cur2val(cur_mA); - currents[i].iHold = val; - currents[i].iRun = val; + currents[i].setiHold(val); + currents[i].setiRun(val); tmc2130_setup_chopper(i, tmc2130_mres[i]); } } @@ -8140,7 +8140,7 @@ Sigma_Exit: { for (uint8_t axis = 0; axis < NUM_AXIS; axis++) { if (code_seen(axis_codes[axis])) { - currents[axis].iHold = code_value_uint8(); + currents[axis].setiHold(code_value_uint8()); tmc2130_setup_chopper(axis, tmc2130_mres[axis]); } } @@ -8164,7 +8164,7 @@ Sigma_Exit: { for (uint8_t axis = 0; axis < NUM_AXIS; axis++) { if (code_seen(axis_codes[axis])) { - currents[axis].iRun = code_value_uint8(); + currents[axis].setiRun(code_value_uint8()); tmc2130_setup_chopper(axis, tmc2130_mres[axis]); } } diff --git a/Firmware/power_panic.cpp b/Firmware/power_panic.cpp index 8e0897e4b..adf3a033d 100644 --- a/Firmware/power_panic.cpp +++ b/Firmware/power_panic.cpp @@ -61,11 +61,11 @@ void uvlo_() { // Minimise Z and E motor currents (Hold and Run) #ifdef TMC2130 - currents[Z_AXIS].iHold = 20; - currents[Z_AXIS].iRun = 20; + currents[Z_AXIS].setiHold(20); + currents[Z_AXIS].setiRun(20); tmc2130_setup_chopper(Z_AXIS, tmc2130_mres[Z_AXIS]); - currents[E_AXIS].iHold = 20; - currents[E_AXIS].iRun = 20; + currents[E_AXIS].setiHold(20); + currents[E_AXIS].setiRun(20); tmc2130_setup_chopper(E_AXIS, tmc2130_mres[E_AXIS]); #endif //TMC2130 @@ -227,8 +227,8 @@ static void uvlo_tiny() { disable_e0(); #ifdef TMC2130 - currents[Z_AXIS].iHold = 20; - currents[Z_AXIS].iRun = 20; + currents[Z_AXIS].setiHold(20); + currents[Z_AXIS].setiRun(20); tmc2130_setup_chopper(Z_AXIS, tmc2130_mres[Z_AXIS]); #endif //TMC2130 diff --git a/Firmware/tmc2130.cpp b/Firmware/tmc2130.cpp index 498b982d3..035a3db34 100755 --- a/Firmware/tmc2130.cpp +++ b/Firmware/tmc2130.cpp @@ -419,7 +419,7 @@ void tmc2130_home_enter(uint8_t axes_mask) tmc2130_wr(axis, TMC2130_REG_GCONF, TMC2130_GCONF_NORMAL); tmc2130_wr(axis, TMC2130_REG_COOLCONF, (((uint32_t)tmc2130_sg_thr_home[axis]) << 16)); tmc2130_wr(axis, TMC2130_REG_TCOOLTHRS, __tcoolthrs(axis)); - currents[axis].iRun = tmc2130_current_r_home[axis]; + currents[axis].setiRun(tmc2130_current_r_home[axis]); tmc2130_setup_chopper(axis, tmc2130_mres[axis]); tmc2130_wr(axis, TMC2130_REG_GCONF, TMC2130_GCONF_SGSENS); //stallguard output DIAG1, DIAG1 = pushpull } @@ -517,15 +517,15 @@ static constexpr bool getIntpolBit([[maybe_unused]]const uint8_t axis, const uin } static void SetCurrents(const uint8_t axis) { - uint8_t iHold = currents[axis].iHold; - const uint8_t iRun = currents[axis].iRun; + uint8_t iHold = currents[axis].getiHold(); + const uint8_t iRun = currents[axis].getiRun(); // Make sure iHold never exceeds iRun at runtime if (iHold > iRun) { iHold = iRun; // Update global array such that M913 reports correct values - currents[axis].iHold = currents[axis].iRun; + currents[axis].setiHold(iRun); // Let user know firmware modified the value SERIAL_ECHO_START; @@ -554,11 +554,8 @@ static void SetCurrents(const uint8_t axis) { void tmc2130_setup_chopper(uint8_t axis, uint8_t mres) { - // Update the currents, this will re-calculate the Vsense flag - currents[axis] = MotorCurrents(currents[axis].iRun, currents[axis].iHold); - // Initialise the chopper configuration - ChopConfU chopconf = ChopConfU(currents[axis].vSense, mres); + ChopConfU chopconf = ChopConfU(currents[axis].getvSense(), mres); chopconf.s.intpol = getIntpolBit(axis, mres); chopconf.s.toff = tmc2130_chopper_config[axis].toff; // toff = 3 (fchop = 27.778kHz) @@ -573,24 +570,24 @@ void tmc2130_setup_chopper(uint8_t axis, uint8_t mres) void tmc2130_set_current_h(uint8_t axis, uint8_t current) { // DBG(_n("tmc2130_set_current_h(axis=%d, current=%d\n"), axis, current); - currents[axis].iHold = current; + currents[axis].setiHold(current); tmc2130_setup_chopper(axis, tmc2130_mres[axis]); } void tmc2130_set_current_r(uint8_t axis, uint8_t current) { // DBG(_n("tmc2130_set_current_r(axis=%d, current=%d\n"), axis, current); - currents[axis].iRun = current; + currents[axis].setiRun(current); tmc2130_setup_chopper(axis, tmc2130_mres[axis]); } void tmc2130_print_currents() { printf_P(_n("tmc2130_print_currents()\n\tH\tR\nX\t%d\t%d\nY\t%d\t%d\nZ\t%d\t%d\nE\t%d\t%d\n"), - currents[0].iHold, currents[0].iRun, - currents[1].iHold, currents[1].iRun, - currents[2].iHold, currents[2].iRun, - currents[3].iHold, currents[3].iRun + currents[0].getiHold(), currents[0].getiRun(), + currents[1].getiHold(), currents[1].getiRun(), + currents[2].getiHold(), currents[2].getiRun(), + currents[3].getiHold(), currents[3].getiRun() ); } diff --git a/Firmware/tmc2130.h b/Firmware/tmc2130.h index fe60f0516..e09e027c3 100644 --- a/Firmware/tmc2130.h +++ b/Firmware/tmc2130.h @@ -60,14 +60,70 @@ typedef struct extern tmc2130_chopper_config_t tmc2130_chopper_config[NUM_AXIS]; struct MotorCurrents { - bool vSense; ///< VSense current scaling - uint8_t iRun; ///< Running current - uint8_t iHold; ///< Holding current + // Refresh the vSense flag + // If the vSense flag changes then both Run and Hold current values + // must be shifted accordingly. This is done especially to handle + // the edge case where only either of the current values are changed at runtime. + // See M911 and M912 + void refreshCurrentScaling() { + // IMPORTANT: iRun must have range 0 to 63 (2^6) so we can properly + // update the current scaling back and forth + + // Detect new vSense value + const bool newvSense = (iRun < 32); + if (vSense != newvSense) { + // Update currents to match current scaling + if (vSense) { + // vSense was 1 [V_FS = 0.32V] but is changing to 0 [V_FS = 0.18V] + // Half both current values to be in sync with current scale range + iHold >>= 1; + iRun >>= 1; + } else { + // vSense was 0 [V_FS = 0.18V], but is changing to 1 [V_FS = 0.32V] + // double the Hold current value + // iRun is expected to already be correct so no shift needed. + // Keep in mind, only a change in iRun can change vSense. + iHold <<= 1; + } + + // Update vSense + vSense = newvSense; + } else if (!vSense) { + // No change in vSense, but vSense = 0, which means we must scale down the iRun value + // from range [0, 63] to range [0, 31] + iRun >>= 1; + } + } constexpr inline __attribute__((always_inline)) MotorCurrents(uint8_t ir, uint8_t ih) : vSense((ir < 32) ? 1 : 0) , iRun((ir < 32) ? ir : (ir >> 1)) , iHold((ir < 32) ? ih : (ih >> 1)) {} + + inline uint8_t getiRun() const { return iRun; } + inline uint8_t getiHold() const { return iHold; } + inline uint8_t getvSense() const { return vSense; } + + void __attribute__((noinline)) setiRun(uint8_t ir) { + iRun = ir; + + // Refresh the vSense bit and take care of updating Hold/Run currents + // accordingly + refreshCurrentScaling(); + } + + void __attribute__((noinline)) setiHold(uint8_t ih) { + iHold = vSense ? ih : ih >> 1; + // Note that iHold cannot change the vSense bit. If iHold is larger + // than iRun, then iHold is truncated later in SetCurrents() + } + + private: + // These members are protected in order to ensure that + // the struct methods are used always to update these values at runtime. + bool vSense; ///< VSense current scaling + uint8_t iRun; ///< Running current + uint8_t iHold; ///< Holding current }; extern MotorCurrents currents[NUM_AXIS];