-
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.
Duet WiFi hardware designer and firmware engineer
Please do not ask me for Duet support via PM or email, use the forum
http://www.escher3d.com, https://miscsolutions.wordpress.com -
@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 F60So 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
I think the problem is in the IterateDrivers Function in the Platform.cppthe 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 MB6XDMaxAxesPlusExtruders = 25 GetTotalAxes() = 3 in this case XYZ MaxExtruders = 16So 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 secI'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.
-
@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.
Maybe you can try, and we can see if this will fix the problem for you too.
-
@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 fallno 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 fallthe 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 driversM112 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 F60without driver timings:
with driver timings (same job file):
-
@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.
-
@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.
Duet WiFi hardware designer and firmware engineer
Please do not ask me for Duet support via PM or email, use the forum
http://www.escher3d.com, https://miscsolutions.wordpress.com -
@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
-
@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 .L555Duet WiFi hardware designer and firmware engineer
Please do not ask me for Duet support via PM or email, use the forum
http://www.escher3d.com, https://miscsolutions.wordpress.com -
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.
-
@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.
closed M112 doesnt stop all movement #927
