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

Draft: Add new eddy current probe support with tap functionality #6785

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vvuk
Copy link

@vvuk vvuk commented Jan 15, 2025

I've been working on updated eddy current sensor support for a week or so now, with a goal of supporting both homing and accurate z-offset setting via "tap" ("touch" etc.) like functionality. This works, though I'm still gathering testers (my sample size is 2 currently of people it works for, including myself). I'm opening this draft PR to get early feedback.

This is implemented as separate "ng" versions of ldc1612 code and eddy current sensor code. I ended up rewriting large parts of these, and I wanted to make sure that the current eddy current support can continue to be used as-is to make it easy for people to switch back and forth (especially during development).

Homing is similar to current probe_eddy_current. However, as there's no temperature calibration or compensation, homing is "coarse" at print temps. Tap is used to set an accurate physical offset at print temperatures.

"Tap" is based on an accumulated sum of a simple average of the derivative of a windowed moving average of the sensor frequencies (say that 10 times fast). I spent a while with a Jupyter notebook and a bunch of data, and this seems to give a very reliable signal:

image

Another plot with tap detection starting and ending bars (all the way to the right; orange start, green end at which point it's triggered):

image

The code generates these interactive plots in /tmp/tap.html if plotly is installed in klippy's venv at tap time.

Post-tap, the sensor's reading is taken at toolhead Z=2 (or whatever the home trigger height is set, because that's also used as the probe height), and the difference between that value and 2 is saved as a "tap offset" which is then added to sensor readings for post-tap bed mesh (as I type this out I realize there's a bug here, actually; will fix). The idea being that post-tap, the steppers + z offset is the most accurate physical distance, so I can figure out what the offset is from the sensor's reading to the true value. Then, for bed mesh, I assume that in bed-mesh-level variance ranges, the sensor will still accurately enough detect +/- 0.2-0.3mm from that point, even if the absolute value isn't correct.

There's also functionality for testing sensor ranges at different drive currents; BTT Eddy's coil design in particular I think leads to a much less useful sensor range, and I end up needing to use a higher drive current for "tap" than I do for home to get useful physical ranges.

I've also written up way too many words for documentation.

Also, if someone who knows more about the physical behaviour of stepper motors when there's contact involved between nozzle/build plate wants to verify that I'm doing the right thing for calculating the true Z offset, it would be appreciated. I've talked myself into this, which could be entirely off base:

  • The toolhead's position at the tap start time is true Z=0 (plus a configurable constant value, typically to "back it off" a bit)
  • When the drip move is stopped, the steppers will have been commanded to a tiny bit beyond that point, even though there's physical contact with the build plate
  • I don't believe the Z steppers will have skipped at this point, instead the force of nozzle against build plate is spread out amongst the entire mechanical assembly
  • So an accurate z offset is purely the toolhead's position (well, the negative of) at the tap start time.
  • The toolhead lifting will just "undo" the motion of the steppers, again without skipping, and will just release that force that was spread out. Meaning that touching the build plate will not cause any Z stepper skips or otherwise incorrect toolhead Z values.

I would appreciate any feedback, as well some idea of whether this would be considered for merging or not. This PR also includes a simple MIT-licensed printf -- I added a way to send back some runtime debug logging messages back to klippy to preserve my sanity :) This can be stripped out, or I'm happy to move this to a separate PR.

@gueee
Copy link

gueee commented Jan 16, 2025

Hi Vladimir!
I've got 6 Cartographers and two Mellow Fly Pros to test and would be happy to help you testing and pushing your project along.

@Sineos
Copy link
Collaborator

Sineos commented Jan 16, 2025

Thanks a lot for this PR. Just some "non-code" comments as food for thought:

  • Probably you are aware of https://github.com/KevinOConnor/klipper-dev/tree/work-ldctap-20240430
  • From a user perspective, I'd consider the fragmentation between BTT Eddy, Cartographer, Beacon, etc., a major issue.
    • It forces the user to install third-party code, causing extra effort and marking Klipper as "dirty."
    • It potentially creates issues on main-line Klipper updates, as suddenly the third-party code no longer works.

Personally, I think that some effort to consolidate the interfaces and functionality between these products would greatly benefit the users (and potentially the developers as well in terms of maintenance and effort).

Of course, this depends on the willingness and ability of the different actors, e.g., @dalegaard, @mattthebaker, @ksavinash9, @wohe, @bigtreetech, @FLYmaker, @jaysuk, to align on a common approach.

@dalegaard
Copy link
Contributor

Hi,

Kevin has made his stance on this stuff abundantly clear and I'm not interested in being involved in the Klipper project. Please refrain from highlighting me.

Best regards,
Lasse

@vvuk
Copy link
Author

vvuk commented Jan 16, 2025

Probably you are aware of https://github.com/KevinOConnor/klipper-dev/tree/work-ldctap-20240430

Yep! I chatted briefly with Kevin before starting down this path. I started building on top of that branch but things evolved pretty rapidly. (I also 100% realize that "rewrite the whole thing" isn't great -- a lot of that was due to me needing to more deeply understand the code, and a lot was code cleanup along the way so that I could have an easier time debugging/diagnosing issues.)

From a user perspective, I'd consider the fragmentation between BTT Eddy, Cartographer, Beacon, etc., a major issue.

Yeah, absolutely. It's one of the reasons I started doing this -- I looked at the code for eddy, carto, etc. and they were all doing similar things in slightly dissimilar ways... and other things in wildly different ways, but unnecessarily so. I have BTT Eddys, Mellow Fly, Cartographer here that I'm planning on testing with myself. The code here should support any LDCxxx sensor (and technically really any sensor that provides some kind of value that can be smoothly mapped to a height -- I have one of these "Bed Distance Sensor" probes that I haven't looked into how they work).

There's a lot that can and should be shared so that improvements are uniform. There's still value in the different products, as coil design significantly impacts precision and range.

@vvuk
Copy link
Author

vvuk commented Jan 16, 2025

A few notes about the approach I took...

Calibration:

  • I originally did calibration via polynomial fitting, increasing in degree until a RMSE threshold was crossed. This worked, but was never really perfect and even a 0.005mm divergence matters for e.g. bed mesh. I reverted back to smoothing out the data and interpolating values. It's still worth exploring fitting, perhaps with some better data conditioning.
  • TODO: test doing multiple calibration moves and average out the data.
  • TODO: expand out the eddy-ng PROBE_ACCURACY (vs. the standard PROBE_ACCURACY) to do a ManualProbe, and then check sensor height reading vs true-Z at a few different heights. This will give a better idea of true accuracy, especially in the 1mm-3mm range which (IMO) is the most important: bed mesh, QGL, etc. would be done at 2.0mm (or centered around whatever the "home height" is).

For tap detection, I looked at the following:

  • Simple sum-of-frequency-change for last N sensor readings, and then waiting for that value to start dropping rapidly, usually expressed as a % drop. This can detect a tap, but for it to be reliable the % has to be very high meaning that tap gets detected much later (50ms or so in my test cases at 3mm/s)
  • 1D Canny edge detection. Very late detection, needs a lot of smoothing to not give false triggers.
  • Higher-order Daubechies wavelet transform (suggestion from someone who knows way more about stats/signals than I do) -- might be promising
  • TODO: Use higher accuracy finite difference coefficients for derivative approximation -- this looks promising, and I'm going to explore this for the derivative calculations
  • TODO: make some of the window sizes configurable, and possibly even implement a few different runtime-selectable detection approaches (for testing, mainly).
  • TODO: right now the tap speed defaults to 3mm/s. This is quite slow, and while it works for me, it does make the touch very gentle and doesn't work well for some testers. I need to gather some data at faster speeds. I've got a Sacrificial Ender (tm) that I won't mind toolhead crashes and whatnot to test with, probably over the weekend.

I'll clean up and share the Jupyter notebook I've been using for data analysis; right now every tap dumps data to /tmp/tap-samples.csv that can be imported and looked at.

General implementation:

  • I see the whitespace test failing :) I've got lots of lines over 80 characters, because I'm not working on a VT100. We can reformat down the line :)
  • Right now it's a full klipper fork, despite just a few added files. I'm going to create a separate repo that's a separately-installable set of files and a simple shell script to patch some of the compilation things. (Klipper could actually benefit from an EXTRA_SOURCES in the makefiles to add things to src-y without needing to edit makefiles.)

@vvuk
Copy link
Author

vvuk commented Jan 19, 2025

(Note: this branch lags behind my vlad/eddy-ng branch in my repo -- this branch has just the eddy-related things, so I manually update it every few days. If anyone's interested in testing, please use https://github.com/vvuk/klipper/tree/vlad/eddy-ng if possible)

@pellcorp
Copy link

Seems like this would have a much greater chance of being merged if the changes were the minimum required to make it work and of course were changes to the existing files rather than new, also what is with the printf files is that just for convenience? I would love to consider this for Simple AF project I have a ton of btt eddy users and probably some carto users who would be up for it

@pellcorp
Copy link

In our testing we also noticed eddy does not like to be used colder than when it was calibrated, is this something your changes are proposing to fix?

@vvuk
Copy link
Author

vvuk commented Jan 24, 2025

Seems like this would have a much greater chance of being merged if the changes were the minimum required to make it work and of course were changes to the existing files rather than new

The majority of the work is new, and changes to existing files would be extensive. There isn't much point in looking at a diff (I mean you can diff e.g. ldc1612.py and ldc1612_ng.py, and there will be some common/shared code, but not much). If this gets merged it should be a super set of the previous code.

also what is with the printf files is that just for convenience

Yep, like I mentioned at the start, it's there for some debugging prints, but is disabled/turned off in this PR. Because of the way this PR was made it's still in here, but I'll update things to remove it shortly.

In our testing we also noticed eddy does not like to be used colder than when it was calibrated, is this something your changes are proposing to fix?

Temperature is pretty irrelevant with the approach I'm taking, except if the extremes are such that the coil simply does not function as needed in those temperatures (i.e. requires drive current or other adjustments).

@pellcorp
Copy link

Seems like this would have a much greater chance of being merged if the changes were the minimum required to make it work and of course were changes to the existing files rather than new

The majority of the work is new, and changes to existing files would be extensive. There isn't much point in looking at a diff (I mean you can diff e.g. ldc1612.py and ldc1612_ng.py, and there will be some common/shared code, but not much). If this gets merged it should be a super set of the previous code.

also what is with the printf files is that just for convenience

Yep, like I mentioned at the start, it's there for some debugging prints, but is disabled/turned off in this PR. Because of the way this PR was made it's still in here, but I'll update things to remove it shortly.

In our testing we also noticed eddy does not like to be used colder than when it was calibrated, is this something your changes are proposing to fix?

Temperature is pretty irrelevant with the approach I'm taking, except if the extremes are such that the coil simply does not function as needed in those temperatures (i.e. requires drive current or other adjustments).

I think you would have way more luck getting this merged it if changed the existing code rather than added new, but of course I am just an interested bystander :-)

We get the Error during homing probe: Eddy current sensor error if we use the probe colder than we calibrated it, so for example if someone calibrates the eddy at 35c and tries to use it for bed mesh at 30c they will often (almost always in my experience) get the eddy current sensor error. In my project Simple AF (its a way to support Creality K1s / K1Ms with eddy, carto etc) we actually have a lot of hacks to work around this.

Anyway I will probably be creating a branch of my own klipper fork with the changes from this PR and asking some Simple AF users to have a play with it, we will let you know our experience :-)

It would be really awesome if this could be merged at some point.

vvuk added 2 commits January 26, 2025 16:07
Upstream commit 482d2798d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants