Movement after M112 E-Stop!

droftarts

@timschneider @hpiz Thanks for all your investigative work, I've flagged this to @dc42.

Ian

dc42

Hi all, so does this issue occur only when using external drivers, for example with a 6XD? ~~Does it occur only in SBC mode?~~

@timschneider which source code branch are you looking at? The current 3.5-dev and 3.4-dev source code has this for Move::Exit:

void Move::Exit() noexcept
{
	StepTimer::DisableTimerInterrupt();
	rings[0].Exit();
#if SUPPORT_ASYNC_MOVES
	rings[1].Exit();
#endif
#if SUPPORT_LASER || SUPPORT_IOBITS
	delete laserTask;
	laserTask = nullptr;
#endif
	moveTask.TerminateAndUnlink();
}

That should be sufficient to stop all movement.

@hpiz which firmware version are you using?

PS - when Emergency Stop is pressed in DWC, it sends M112 and then after a little while it sends M999 to reset the Duet. At the reset the driver Enable signal may go to Enabled again, depending on its polarity. But by then the movement queue is empty because RRF has re-initialised.

timschneider

@dc42
sorry I was referring to the commit where the Move Task was added.
But you give the right hint, I was looking in the wrong direction, as it can bee seen from the oscilloscope that the stop of the step generation is delayed. So I guess, another code before Move::Exit is delayed or the very first line StepTimer::DisableTimerInterrupt is slow.

I'll be back in the office on monday, then I can do the above tests with Duet 3 6HC.

hpiz is using 3.4.6 - from the debug log

power up + 00:00:00 [info] Running: Duet 3 MB6XD v1.01 or later: 3.4.6 (2023-07-21 14:11:58)

hpiz

@dc42 Hi, I have a Duet 2 I could test 3.4.6 with to see steps on my analog scope. Not sure if that helps...

dc42

All, when you see this behaviour is it just the remainder of the current move that continues to execute after a pause? The Move::Exit code empties the movement queues but I'm wondering whether the step interrupt might be getting re-enabled and perhaps that might cause the current move to complete.

hpiz

@dc42 From my experience, it would seem it is just continuing with the moves it was already performing. For example, I don't really see any behavior difference between the following two GCode jobs...

G90
G21


G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

M112


G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

vs

G90
G21


G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

G1 Y500 Z100 F4000
G1 Y600 Z0 F4000

M112

The M112 seems to be executed as it's read into the buffer, around the second movement loop, it then tries to complete the third loop after a brief stop (0.25 secs), nothing after M112 seems to ever try to execute, so that's good. Not sure if this matters, but the continued moves seem to move beyond where they should, which in my mind could only happen if more steps are sent than should. I'm gonna test my Duet 2 ethernet today (will post back maybe 2 hours from now) and see if it's got the same step continuation.

As far as the code goes, I wish I could help more but I mostly make websites in C#. Though, I will keep trying.

hpiz

@dc42 My Duet 2 does not produce pulses after M112 running 3.4.6

Note: The scope is in trigger mode

Duet2: M112 in job

Duet2: Using DWC Endstop

dc42

Thanks all, I'll be away at FormNext for a week but I'll look into this as a matter of urgency when I return.

timschneider

@dc42 we have to thank you for your very fast response and in the middle of the preperation for the Formnext!

@dc42 said in Movement after M112 E-Stop!:

All, when you see this behaviour is it just the remainder of the current move that continues to execute after a pause?

To check that, I did another test. Setup as above with SBC. Jobfile the following:

G90
G21

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

So every move is around 1 sec (F60 is within the jerk so i guess there is not much acceleration). Based on that, the delay should be less than 1-1.5s, but in fact, it is again around 3.5s (3.46s) as in all the samples above, regardless of the movement length and speed.

So from my understanding, it is not only the remainder of the current move.

timschneider

@timschneider
I think the problem is in the IterateDrivers Function in the Platform.cpp

the following code snippets are based on the 3.4-dev branch

The else if branch will disable all extruders, but the problem is, that it will start working from GetTotalAxes() to MaxAxesPlusExtruders. But there are only MaxExtruders allowed.
That means in numbers for the MB6XD

MaxAxesPlusExtruders = 25
GetTotalAxes() = 3 in this case XYZ
MaxExtruders  = 16

So from 3 - 24 it will work with the else if branch

size_t LogicalDriveToExtruder(size_t drive) noexcept { return MaxAxesPlusExtruders - 1 - drive; }

25 - 1 - 3 = 21
25 - 1 - 4 = 20
...
25 - 1 - 24 = 0

DriverId extruderDrivers[MaxExtruders] -> array of 16 elements

so the following code will read some random data for the elements from 3 - 8
const DriverId id = extruderDrivers[LogicalDriveToExtruder(axisOrExtruder)];

and this code will sometimes end up in the remoteFunc e.g. in our case send can message and wait for a reply

if (id.IsLocal())
{
	localFunc(id.localDriver);
}
else
{
	remoteFunc(id);
}

but there will be no reply and this will delay the e-stop (stop step generation and clear the movement system) around 3 - 3.5 sec

I've changed the IterateDrivers the following:

// Function to identify and iterate through all drivers attached to an axis or extruder
void Platform::IterateDrivers(size_t axisOrExtruder, function_ref<void(uint8_t)> localFunc, function_ref<void(DriverId)> remoteFunc) noexcept
{
	if (axisOrExtruder < reprap.GetGCodes().GetTotalAxes())
	{
		for (size_t i = 0; i < axisDrivers[axisOrExtruder].numDrivers; ++i)
		{
			const DriverId id = axisDrivers[axisOrExtruder].driverNumbers[i];
			if (id.IsLocal())
			{
				localFunc(id.localDriver);
			}
			else
			{
				remoteFunc(id);
			}
		}
	}
	else if (axisOrExtruder >= (MaxAxesPlusExtruders - MaxExtruders) && axisOrExtruder < MaxAxesPlusExtruders)
	{
		const DriverId id = extruderDrivers[LogicalDriveToExtruder(axisOrExtruder)];
		if (id.IsLocal())
		{
			localFunc(id.localDriver);
		}
		else
		{
			remoteFunc(id);
		}
	}
}

But this will only address the delay - I think this is a good time to add a secondary layer to prevent the EnableSignale from getting high again. (Because of the delay, prepare moves where re-enableing the drivers again)

platform.cpp

 src/Platform/Platform.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/Platform/Platform.cpp b/src/Platform/Platform.cpp
index c8f7793c..fdd6f2d3 100644
--- a/src/Platform/Platform.cpp
+++ b/src/Platform/Platform.cpp
@@ -424,7 +424,7 @@ Platform::Platform() noexcept :
 #if HAS_MASS_STORAGE
 	logger(nullptr),
 #endif
-	board(DEFAULT_BOARD_TYPE), active(false), errorCodeBits(0),
+	board(DEFAULT_BOARD_TYPE), active(false), preventDriverEnable(true), errorCodeBits(0),
 	nextDriveToPoll(0),
 	lastFanCheckTime(0),
 #if SUPPORT_PANELDUE_FLASH
@@ -861,6 +861,7 @@ void Platform::Init() noexcept
 	DuetExpansion::DueXnTaskInit();								// must initialise interrupt priorities before calling this
 #endif
 	active = true;
+	preventDriverEnable = false;
 }
 
 // Reset the min and max recorded voltages to the current values
@@ -2439,7 +2440,7 @@ void Platform::IterateDrivers(size_t axisOrExtruder, function_ref<void(uint8_t)>
 			}
 		}
 	}
-	else if (axisOrExtruder < MaxAxesPlusExtruders)
+	else if (axisOrExtruder >= (MaxAxesPlusExtruders - MaxExtruders) && axisOrExtruder < MaxAxesPlusExtruders)
 	{
 		const DriverId id = extruderDrivers[LogicalDriveToExtruder(axisOrExtruder)];
 		if (id.IsLocal())
@@ -2572,7 +2573,7 @@ void Platform::DisableOneLocalDriver(size_t driver) noexcept
 // Enable the local drivers for a drive. Must not be called from an ISR, or with interrupts disabled.
 void Platform::EnableDrivers(size_t axisOrExtruder, bool unconditional) noexcept
 {
-	if (unconditional || driverState[axisOrExtruder] != DriverStatus::enabled)
+	if (unconditional || (driverState[axisOrExtruder] != DriverStatus::enabled && !preventDriverEnable))
 	{
 		driverState[axisOrExtruder] = DriverStatus::enabled;
 		const float requiredCurrent = motorCurrents[axisOrExtruder] * motorCurrentFraction[axisOrExtruder];

platform.h

 src/Platform/Platform.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/Platform/Platform.h b/src/Platform/Platform.h
index c3f006b4..4f2b2196 100644
--- a/src/Platform/Platform.h
+++ b/src/Platform/Platform.h
@@ -528,7 +528,7 @@ public:
 
 	uint32_t GetSteppingEnabledDrivers() const noexcept { return steppingEnabledDriversBitmap; }
 	void DisableSteppingDriver(uint8_t driver) noexcept { steppingEnabledDriversBitmap &= ~StepPins::CalcDriverBitmap(driver); }
-	void EnableAllSteppingDrivers() noexcept { steppingEnabledDriversBitmap = 0xFFFFFFFFu; }
+	void EnableAllSteppingDrivers() noexcept { if (!preventDriverEnable) { steppingEnabledDriversBitmap = 0xFFFFFFFFu; } }
 
 #ifdef DUET3_MB6XD
 	bool HasDriverError(size_t driver) const noexcept;
@@ -643,6 +643,8 @@ public:
 	const GpInputPort& GetGpInPort(size_t gpinPortNumber) const noexcept
 		pre(gpinPortNumber < MaxGpInPorts) 	{ return gpinPorts[gpinPortNumber]; }
 
+	void SetPreventDriverEnable() noexcept { preventDriverEnable = true; }
+
 #if MCU_HAS_UNIQUE_ID
 	const UniqueId& GetUniqueId() const noexcept { return uniqueId; }
 	uint32_t Random() noexcept;
@@ -742,6 +744,7 @@ private:
 #endif
 
 	bool active;
+	bool preventDriverEnable;
 	uint32_t errorCodeBits;
 
 	void InitialiseInterrupts() noexcept;

RepRap.cpp

 src/Platform/RepRap.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/Platform/RepRap.cpp b/src/Platform/RepRap.cpp
index 5d5a28aa..168619c4 100644
--- a/src/Platform/RepRap.cpp
+++ b/src/Platform/RepRap.cpp
@@ -1004,6 +1004,7 @@ void RepRap::EmergencyStop() noexcept
 	else
 #endif
 	{
+		platform->SetPreventDriverEnable(); // prevent any conditional call to EnableDrivers
 		platform->DisableAllDrivers();				// disable all local and remote drivers - need to do this to ensure that any motor brakes are re-engaged
 
 		switch (gCodes->GetMachineType())

With these changes, there will be no delay at all after the estop is pressed in DWC and processed by RRF.

timschneider

@hpiz so I've recompiled a clean version of the 3.4-dev branch with only the IterateDrivers fix pull request on github.

This is not a production version and only for testing purpose, it is based on the current 3.4-dev branch and only for MB6XD. Use on your own risk.

Duet3Firmware_MB6XD.bin

Maybe you can try, and we can see if this will fix the problem for you too.

timschneider

@dc42 the missing MB6HC test.

MB6HC in standalone mode

following config

; Enable network
M552 P0.0.0.0 S1
    
;CNC Mode
M453                            ; Select CNC device mode
M669 K0 S2 T1

M584 X5 Y0.0:0.1:0.2:0.3 Z4             ; set drive mapping

M92 X160.00 Y320.00 Z640.00     ; set steps per mm
M566 X1800.00 Y180.00 Z120.00     ; set maximum instantaneous speed changes (mm/min)
M203 X15000.00 Y12000.00 Z5000.00 I5  ; set maximum speeds (mm/min)
M201 X400.00 Y200.00 Z200.00        ; set accelerations (mm/s^2)

;M564 H0                         ; Allow stepper movement before homing
; Axis Limits
M208 X0 Y0 Z0 S1                ; set axis minima
M208 X600 Y1200 Z170 S0         ; set axis maxima

;Machine Initialization
;M18                          ; disable all steppers
;M17                          ; enable z stepper so it doesn't fall

no delay.

with the following config (added stepper driver timings)

; Enable network
M552 P0.0.0.0 S1
    
;CNC Mode
M453                            ; Select CNC device mode
M669 K0 S2 T1

M569 R0 P0 S0 T3:3:6:0
M569 R0 P1 S0 T3:3:6:0
M569 R0 P2 S0 T3:3:6:0
M569 R0 P3 S0 T3:3:6:0
M569 R0 P4 S0 T3:3:6:0
M569 R0 P5 S0 T3:3:6:0

M584 X5 Y0.0:0.1:0.2:0.3 Z4             ; set drive mapping

M92 X160.00 Y320.00 Z640.00     ; set steps per mm
M566 X1800.00 Y180.00 Z120.00     ; set maximum instantaneous speed changes (mm/min)
M203 X15000.00 Y12000.00 Z5000.00 I5  ; set maximum speeds (mm/min)
M201 X400.00 Y200.00 Z200.00        ; set accelerations (mm/s^2)

;M564 H0                         ; Allow stepper movement before homing
; Axis Limits
M208 X0 Y0 Z0 S1                ; set axis minima
M208 X600 Y1200 Z170 S0         ; set axis maxima

;Machine Initialization
;M18                          ; disable all steppers
;M17                          ; enable z stepper so it doesn't fall

the delay is there again. I think it will fill the following struct (slowDriversBitmap and slowDriverStepTimingClocks), and will cause IterateDrivers to send can messages.

	DriverId extruderDrivers[MaxExtruders];					// the driver number assigned to each extruder
#ifdef DUET3_MB6XD
... disabled for MB6HC
#else
	uint32_t slowDriversBitmap;								// bitmap of driver port bits that need extended step pulse timing
	uint32_t slowDriverStepTimingClocks[4];					// minimum step high, step low, dir setup and dir hold timing for slow drivers

M112 in job file, because there is no enable signal turned on and off for the interal / smart drivers.

jobfile

G90
G21

G1 Y0 F60
G1 Y3 F60

G1 Y0 F60
G1 Y3 F60

M112

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

G1 Y0 F60
G1 Y1 F60

without driver timings:

with driver timings (same job file):

hpiz

@timschneider said in Movement after M112 E-Stop!:

Duet3Firmware_MB6XD.bin

You sir, are a wizard. I spent probably 2 hours looking at the code and IterateDrives was not something I even looked at, I kept going back to the DisableAllDrivers in the platform, which I guess was in the ballpark for the enable signal, but I was mostly looking for step loop hangs.

I loaded up your firmware and after 6 tests (M112 in job, driver error, and DWC EStop) I was unable to get any motion after M112. Bravo, I think I'm gonna keep your firmware until dc42 takes the pull request and releases. I still have a lot of work to do on my machine, although I've taken like a two week break.

dc42

@timschneider thanks for your detailed analysis!

I think the correct fix to IterateDrivers is to change the else if condition to this:

else if (axisOrExtruder < MaxAxesPlusExtruders && LogicalDriveToExtruder(axisOrExtruder) < reprap.GetGCodes().GetNumExtruders())

This will ensure that no action is taken if IterateDrivers is called with a logical drive number that is too high to be an axis but too low to be an extruder.

I agree that an additional layer of protection is needed to guard against CAN delays so I will review your solution, although I think there may already be an 'active' flag that we could use for this purpose.

EDIT: I've reviewed your code that add the preventDriverEnable flag and I don't think it will have the desired effect; because function EnableAllSteppingDrivers doesn't actually enable drivers, it just clears the "don't step" flag that gets set during homing of an axis that has multiple motors with separate endstops. So instead I've changed Move::MoveLoop to self-terminate the Move task when emergency stop has been commanded, thereby preventing further calls to DDA::Prepare.

These changes are now committed to the 3.5-dev branch of RRF. Please test them on your machine.

timschneider

@dc42
I agree that this "else if" check is much better, more future proof and less error prone, just note the performance degradation due to calling LogicalDriveToExtruder twice. I am not aware if this can be solved at compile time.

@dc42 said in Movement after M112 E-Stop!:

I've changed Move::MoveLoop to self-terminate the Move task when emergency stop has been commanded, thereby preventing further calls to DDA::Prepare.

I wouldn't have dared to even consider the change, but in the end it should be safe - and that looks safe to me.

I'll compile and check each part alone and post results.

Tim

dc42

@timschneider thanks. The LoigicalDriveToExtruder function is inlined, so I expect the compiler to optimise the two calculations into one.

Edit: yes the second call is optimised away. Here's the start of the else-if:

4652              	.L545:
4653 0082 1F29     		cmp	r1, #31
4654 0084 FAD8     		bhi	.L544
4655 0086 C1F11F01 		rsb	r1, r1, #31
4656 008a D3F8F832 		ldr	r3, [r3, #760]
4657 008e 8B42     		cmp	r3, r1
4658 0090 F4D9     		bls	.L544
4659 0092 00EB4100 		add	r0, r0, r1, lsl #1
4660 0096 B0F8843A 		ldrh	r3, [r0, #2692]
4661 009a C3F30724 		ubfx	r4, r3, #8, #8
4662 009e ADF81030 		strh	r3, [sp, #16]	@ movhi
4663 00a2 FFF7FEFF 		bl	_ZN12CanInterface13GetCanAddressEv
4664 00a6 8442     		cmp	r4, r0
4665 00a8 0AD0     		beq	.L555

timschneider

@dc42

I think it is a success!

without the IterateDrivers fix there is a small delay 1.34s in the step generation shut down, but the drivers are not re-enabled.

with both patches 'no' delay at all.

dc42

@timschneider, many thanks for your thorough testing and for diagnosing the cause of the problem. I'll mark https://github.com/Duet3D/RepRapFirmware/issues/927 as fixed in 3.5. If/when we release 3.4.7 the fix will be in that version too. I've already back-ported the fix to the 3.4-dev source code.

dc42 created this issue in Duet3D/RepRapFirmware

Movement after M112 E-Stop!

closed M112 doesnt stop all movement #927