grblHAL / core

grblHAL core code and master Wiki
Other
305 stars 74 forks source link

Stepper driver startup current #400

Open dresco opened 7 months ago

dresco commented 7 months ago

Was just taking a closer look at driver_setup(), with respect to the stepper enable pins & startup current draw in the Trinamic drivers..

I'd updated the H7 driver in line with the recent F4 changes, but I don't believe this is working as intended.

I think the the DIGITAL_OUT() has to remain after the HAL_GPIO_Init(), else it has no effect on the physical pin state (at least that is what I'm observing on the H7).

What I see currently is that GPIO_Init() sets the pin output low (typically enabling the stepper driver), and it stays that way until deenergized later from grbl_enter().

With the DIGITAL_OUT line (including StepperEnable), moved back below HAL_GPIO_Init() - then at least the output would be set high almost immediately after the output is initialised?

(Appreciate this initial DIGITAL_OUT pays no attention to the $4 inversion setting, but presumably that's a whole different can of worms).


Recent update to the H7 driver below for reference;

--- a/Src/driver.c
+++ b/Src/driver.c
@@ -1943,12 +1943,13 @@ static bool driver_setup (settings_t *settings)

     for(i = 0 ; i < sizeof(outputpin) / sizeof(output_signal_t); i++) {
         if(outputpin[i].group != PinGroup_StepperPower) {
+
+            if(outputpin[i].group == PinGroup_MotorChipSelect || outputpin[i].group == PinGroup_MotorUART || outputpin[i].group == PinGroup_StepperEnable)
+                DIGITAL_OUT(outputpin[i].port, outputpin[i].bit, 1);
+
             GPIO_Init.Pin = outputpin[i].bit = 1 << outputpin[i].pin;
             GPIO_Init.Mode = outputpin[i].mode.open_drain ? GPIO_MODE_OUTPUT_OD : GPIO_MODE_OUTPUT_PP;
             HAL_GPIO_Init(outputpin[i].port, &GPIO_Init);
-
-            if(outputpin[i].group == PinGroup_MotorChipSelect || outputpin[i].group == PinGroup_MotorUART)
-                DIGITAL_OUT(outputpin[i].port, outputpin[i].bit, 1);
         }
     }
terjeio commented 7 months ago

I set the output state early on purpose to avoid a glitch on the pin (going from high-Z input directly to high). It was done to avoid a current surge in the stepper drivers on startup. Writing to the output register before setting direction is allowed and the pin will be set to the state held in the output register when direction is set. From the PRM (7.3.10):

When the I/O port is programmed as output:
The output buffer is enabled:
– Open drain mode: A “0” in the Output register activates the N-MOS whereas a “1” 
in the Output register leaves the port in Hi-Z (the P-MOS is never activated)
– Push-pull mode: A “0” in the Output register activates the N-MOS whereas a “1” in 
the Output register activates the P-MOS
terjeio commented 7 months ago

This is the issue that triggered the change.

dresco commented 7 months ago

This is the issue that triggered the change.

Thanks, yes that is exactly the scenario that had me looking closer at this...

Writing to the output register before setting direction is allowed and the pin will be set to the state held in the output register when direction is set. From the PRM (7.3.10):

I see, that makes total sense then. However, it doesn't seem to work on the H7... :(

Looking more closely, it seems the BSSR write doesn't work if not already in output mode, as the corresponding bit in ODR is not set, and the pin is subsequently driven low on HAL_GPIO_Init().

It does work if I set the relevant bit in ODR directly (instead of using DIGITAL_OUT). I wonder if this behaviour is the same for the F7?

terjeio commented 7 months ago

I wonder if this behaviour is the same for the F7?

I'll check when I am back home, going tomorrow.

dresco commented 7 months ago

I'll check when I am back home, going tomorrow.

Thanks, fwiw this is what I have currently;

diff --git a/Src/driver.c b/Src/driver.c
index abee334..45e42df 100644
--- a/Src/driver.c
+++ b/Src/driver.c
@@ -1944,8 +1944,9 @@ static bool driver_setup (settings_t *settings)
     for(i = 0 ; i < sizeof(outputpin) / sizeof(output_signal_t); i++) {
         if(outputpin[i].group != PinGroup_StepperPower) {

+            // Set the initial state of the pin when it is enabled (note: need to use ODR instead of BSRR)
             if(outputpin[i].group == PinGroup_MotorChipSelect || outputpin[i].group == PinGroup_MotorUART || outputpin[i].group == PinGroup_StepperEnable)
-                DIGITAL_OUT(outputpin[i].port, outputpin[i].bit, 1);
+                outputpin[i].port->ODR |= 1 << outputpin[i].pin;

             GPIO_Init.Pin = outputpin[i].bit = 1 << outputpin[i].pin;
             GPIO_Init.Mode = outputpin[i].mode.open_drain ? GPIO_MODE_OUTPUT_OD : GPIO_MODE_OUTPUT_PP;
terjeio commented 7 months ago

I wonder if this behaviour is the same for the F7?

Yes, it is. Your code above works.

dresco commented 7 months ago

Am also wondering whether there should be $ parameter settings for default current per axis (before any M906 changes), do you have any thoughts on this?

When I was looking at the Marlin code (looking for clues re the reported 5160 driver problems on the H7 board), I noticed that it sets a default current limit of 800mA for 5160 drivers.

Out of the box, the EZ5160 Pro driver modules have a default current setting that seems a bit sketchy without active cooling - according to their own product page.

image

My test board pulls a solid 1.8A @ 24V just turning a single NEMA17 motor in this default state..

M122
[TRINAMIC]
                      X
Driver          TMC5160
Set current           0
RMS current        2960
Peak current       4186
Run current       30/31
Hold current      15/31
...
terjeio commented 7 months ago

Am also wondering whether there should be $ parameter settings for default current per axis

In addition to the $14x settings? Default values are set here. Could it be that you need to reset driver/plugin settings with $RST=&since Set current is reported as 0?

dresco commented 7 months ago

In addition to the $14x settings? Default values are set here. Could it be that you need to reset driver/plugin settings with $RST=&since Set current is reported as 0?

Hah! No, that is exactly what I was suggesting - I'd just not noticed it existed already :blush:

They do always seem to revert to 0 on a settings reset though, will investigate that..

dresco commented 7 months ago

Hmm, they seem to be explicitly set to zero here, is that expected?

terjeio commented 7 months ago

Oops, this may be an unintended side-effect from core changes I made earlier. The core defaults were initially for I2C potentiometers - I'll have to fix it...