Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FT Motion updates #27349

Merged

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Aug 12, 2024

Proposed updates to FT Motion from @ulendoalex to address miscellaneous reported issues. There may be some extraneous code reversions that we can sort out as we clean this up, test, and discuss.

Based on S2AUlendo/Marlin-migration-test

@narno2202
Copy link
Contributor

So you want to add Ulendo's CaaS (M494) but in this case we need an accelerometer and a PI. Why not installing Klipper? :)
Don' forget that some parts of this code, breaks some basic functionality (probing for example).

@thinkyhead
Copy link
Member Author

thinkyhead commented Aug 13, 2024

So you want….

This is a set of proposed changes submitted by Ulendo, which I needed to reorganize as a starting point to sort them out. I'm starting out by posting them more-or-less as-written, and it will be up to us to keep the good parts, fix the broken parts, and throw out any unnecessary changes here.

I haven't made any specific choices yet, except probably to merge the time-based endstop monitoring. I need to tweak endstops.cpp to handle the changes I made to that time-and-direction-tracking.

I don't think the M494 addition is actually intended by Ulendo, but it was included in the code that was submitted to me for review, so it is included here too for completeness.

@narno2202
Copy link
Contributor

Regarding endstops management, the code was already proposed in #26848 then removed because of bugs. I deliberately didn't include M494 when I submit #26848 as an external device is needed to tweak the shaper (Raspberry PI).
Current bugfix is running fine on my printer with sensroless homing or mechanical endstops (all the axes) and various kinds of probes : BLTouch, CRTouch, SmartTouch and Biqu Microprobe, the last with endstop interrupt enabled (this is perhaps a good solution).

@thinkyhead thinkyhead force-pushed the Marlin-migration-test-drop branch 2 times, most recently from 244bdec to 52bc3db Compare August 13, 2024 22:14
@thisiskeithb
Copy link
Member

Current bugfix is running fine on my printer

Too bad that isn't true for everyone 😄 We have several open FT Motion bugs right now.

the last with endstop interrupt enabled (this is perhaps a good solution).

This is incompatible with LPC-based configs and is now sanity checked (#27302), so it's not universal.

@narno2202
Copy link
Contributor

@thisiskeithb , not sure that sounding is a bug. For sensorless homing, I'm still thinking of a setting problem (it was that for me). For LPC motherboard, in the datasheet LPC has external interrupt pins as I'm not an electronic engineer, so I think OK, it will work :)

@thinkyhead thinkyhead force-pushed the Marlin-migration-test-drop branch 2 times, most recently from 4e2d7ac to ef268b5 Compare August 14, 2024 18:35
@narno2202
Copy link
Contributor

@thinkyhead , @thisiskeithb , I have a question regarding endstops.
How an endstop could be triggered without a physical contact?
This is what happens with Biqu Microprobe when homing the Z axis. As it works with standard motion, the probe is well wired and not defective. False triggering could be due to electrical interference. Is It possible in this case and if not is there another cause?

@thinkyhead
Copy link
Member Author

How an endstop could be triggered without a physical contact?

Lack of a pullup/pulldown can leave a pin floating, making it more susceptible to EMF induction, and in general this is more of a problem for NO switches versus NC switches. Keeping the endstop isolated from the stepper motor wires can help, as can adding a tiny capacitor or even a tiny resistor. A ferrite core would also help.

@narno2202
Copy link
Contributor

@thinkyhead , many thanks for your answer. As my motherbord have pullups on all endstops pins, I suppose (fear) that Microprobe failure is related to excessive sensibility per hardware design to EMF generated by FTM processing.
Do you think that a kind of Endstop Noise Threshold only for Z homing when Microprobe is present could be helpful?

@thinkyhead
Copy link
Member Author

thinkyhead commented Aug 16, 2024

This is now reduced to a small number of effective changes:

  • Use the calculated move duration to decide when to check endstops. (@ulendoalex)
  • Rework of baby-stepping within FT Motion handling. (@ulendoalex)
  • Remove FT_BIT_START since it may be obviated by the move duration. (@thinkyhead)
  • Add ftMotion.axis_is_moving and ftMotion.motor_direction to retain existing endstop logic. (@thinkyhead)
  • FT Motion uses *_AXIS to index XYZ direction, so account for that in endstop logic. (@thinkyhead)
  • Bring back blockProcRdy = false when stepper.abort_current_block. Is this needed? (@ulendoalex)

The main thing we're concerned about is to ensure homing and probing will work. A move duration may be very very long, but generally less than a minute, so we just want to make sure our time checking logic isn't impacted by the clock turning over, overflowing, or being too small a type to track longer durations.

Referring to the original code linked in the OP, it wasn't clear what parts were reverted on purpose and what parts were reverted by mistake due to generating the code by diffing against older code. We favor using the existing states in the Stepper class to track axis-moved and axis-direction so these states properly reflect the DIR / MOVED states set most recently by FT Motion. With that in mind, I took great care to sort out what should be kept, so we should be very close in that respect. Only one or two questions remain.

I'll be back after a break to install this on the BIQU BX for testing, and it shouldn't take too long to see whether it's working or not. Once we get past this exciting challenge, it's straight on to figuring out how to eliminate those loud overtones. Fun!

@narno2202
Copy link
Contributor

@thinkyhead , I just test it. Sensorless homing is OK (still OK). Z homing with BLTouch and Bed Leveling with HS mode are OK (still OK). The diagonal move when homing with G28 (homing all axes) is ultra fast now. Biqu Microprobe still fails as before. I do not notice any difference with the current bugfix as this PR is a revival of old code that has already worked for me. Regarding homing and probing, there are too few reports of reel bugs to have an exact idea of the problem : which probes, which kind of endstops really fail.

@narno2202
Copy link
Contributor

@thinkyhead , in ft_motion.cpp

if (stepper.abort_current_block) {
    if (sts_stepperBusy) return;          // Wait until motion buffers are emptied
    discard_planner_block_protected();
    reset();
    blockProcRdy = false;                 // Set queueing to look for next block.
    stepper.abort_current_block = false;  // Abort finished.
  }

blockProcReady is already set to false in reset()

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 17, 2024

The diagonal move when homing with G28 (homing all axes) is ultra fast now.

That's a recent bug that exists outside of this PR.

I just ran a couple prints on my BX today with ecfff50 and noticed the same issue and planned to track down which commit it was after the last round of prints was complete.

The linked Issue below could also be a symptom of recent changes since they are having troubles with fast homing (or random failing):

@thinkyhead
Copy link
Member Author

That's a recent bug that exists outside of this PR.

I'll be sure to address that in my testing … also on the BX.

@narno2202
Copy link
Contributor

@thinkyhead , finally, I have found the solution for Biqu Microprobe and FT_MOTION for Z homing. The only way to make it fine, is to decrease Z_CURRENT_HOME. Mine was 630 mA for Z_CURRENT and I set it to 300mA for Z_CURRENT_HOME. As I don't if there is an universal threshold, a comment should be added in Configuration.h and a test in SanityCheck.h. Do you want me to propose a PR or can it be added to this one?

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 17, 2024

I have found the solution for Biqu Microprobe and FT_MOTION for Z homing. The only way to make it fine, is to decrease Z_CURRENT_HOME. Mine was 630 mA for Z_CURRENT and I set it to 300mA for Z_CURRENT_HOME. As I don't if there is an universal threshold, a comment should be added in Configuration.h and a test in SanityCheck.h. Do you want me to propose a PR or can it be added to this one?

That seems really odd that Z homing current needs to be reduced/sanity checked with only this probe & motion type. Are you sure it's not something else?

@narno2202
Copy link
Contributor

@thisiskeithb , as I said in a previous post, the microprobe was triggered without any physical contact soon after it was deployed when the motion starts. The microprobe works fine with FT_MOTION disabled. I suspect electromagnetic interference generated by FTM processing. I try to adapt ENDSTOP_NOISE_THRESHOLD for Z homing when FT_MOTION is enabled but it failed miserably as my other experiments with FT_MOTION code. Then I remembered a discussion with @vovodroid about sensorless homing. If failure is related to EMF, current plays a role, so I tested modifying Z_CURRENT_HOME and it works. Can you test it?

@narno2202
Copy link
Contributor

@vovodroid , there is a misunderstanding, I was only talking about Z homing with the Biqu Microprobe. If your sensorless homing problem is solved, please give it a try. And if not, you can home X and Y axis with FT_MOTION disabled, enable it and then home Z.

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 21, 2024

The diagonal move when homing with G28 (homing all axes) is ultra fast now.

I tracked the bug down to 89b0143.

Resetting to right before that commit (i.e. 0790a9d), stops the really fast X/Y travel move to Z_SAFE_HOMING_X_POINT/Z_SAFE_HOMING_Y_POINT during G28.


Issuing a G28 after a fresh flash & EEPROM reset will not exhibit the bug. This bug manifests itself when you have the "Emit to G-code" option enabled in PrusaSlicer (and I assume its derivatives) since those limits are higher than many config's defaults:

image

Without starting a full print, I can reproduce this on my BIQU BX and Prusa Bear after flashing & resetting the EEPROM by sending the following commands:

G28
M203 X500 Y500 Z10 E120 ; pulled from slicer g-code
G28

As noted above, reverting back to 0790a9d fixes this broken behavior.

edit: @rq3 confirmed that this also fixes delta homing/G33 behavior over in #27354 (comment)

@thinkyhead
Copy link
Member Author

this also fixes delta homing/G33 behavior

@thisiskeithb — Maybe it's enough to revert one line from that commit. In motion.cpp (line 1128) try reverting to this and see if it makes the move more in line with expectations. A 'blocking' move would have previously preferred XY_PROBE_FEEDRATE which defaults to the average of X and Y (rather than, e.g., the larger of X and Y).

- const feedRate_t xy_feedrate = fr_mm_s ?: feedRate_t(PLANNER_XY_FEEDRATE_MM_S);
+ const feedRate_t xy_feedrate = fr_mm_s ?: feedRate_t(XY_PROBE_FEEDRATE_MM_S);

@thinkyhead
Copy link
Member Author

@thinkyhead , finally, I have found the solution for Biqu Microprobe and FT_MOTION for Z homing. The only way to make it fine, is to decrease Z_CURRENT_HOME. Mine was 630 mA for Z_CURRENT and I set it to 300mA for Z_CURRENT_HOME. As I don't if there is an universal threshold, a comment should be added in Configuration.h and a test in SanityCheck.h. Do you want me to propose a PR or can it be added to this one?

A #warning in Warnings.cpp may suffice to remind users to tune the homing current carefully if there is any trouble. We can also add a section to the website Troubleshooting page concerning this one.

As this PR is confirmed working just fine on our own setups, I'll merge it for wider testing. A round of bug fixes is in order over the coming weeks, as we will soon make a pre-release version just for testers to bang on.

@thinkyhead thinkyhead merged commit 934ac24 into MarlinFirmware:bugfix-2.1.x Aug 24, 2024
63 checks passed
@thinkyhead thinkyhead deleted the Marlin-migration-test-drop branch August 24, 2024 04:42
@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 24, 2024

Maybe it's enough to revert one line from that commit.

Grabbing the latest bugfix-2.1.x with e2d8b2f fixes the super fast x/y homing move, but I'm waiting for @rq3 over in #27354 to see if it fixes homing/G33 on deltas.

All fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants