Add ldc1612 support for "METHOD=scan" probing

[KevinOConnor]

This is an alternate implementation to the "scan" mode introduced in PR #6558 . The main difference is that this PR refactors the internal probe classes with a goal to "flatten" some of those interactions between classes.

This PR is on top of PR #6605 .

@Arksine - the main goal of this PR is to demonstrate a possible internal interface between the classes for the "detailed scan" mode. That is, the toolhead movements still go through ProbePointsHelper, but now PrinterEddyProbe can detect a "scan" request and substitute a different probe implementation. Also, the same sample gathering code is used for both "descend until trigger" and "scan probing" modes. I understand your PR had improved user options, improved documentation, and other improvements. Happy to change this PR further, or go with an alternate implementation.

Let me know your thoughts.
-Kevin

[Arksine]

Hi Kevin,

Having done a basic review of the code it looks good to me. I'll do some testing and an in depth review as soon as I am able. With regard to additional options and documentation, I can add that in a follow-up PR that adds "rapid" probing to bed_mesh. Speaking of which, I think I can reuse most of your code with a small modification to the EddyScanningProbe class:

--- a/klippy/extras/probe_eddy_current.py
+++ b/klippy/extras/probe_eddy_current.py
@@ -346,12 +346,16 @@ class EddyScanningProbe:
         self._gather = EddyGatherSamples(printer, sensor_helper,
                                          calibration, z_offset)
         self._sample_time_delay = 0.50
-        self._sample_time = 0.100
+        self._sample_time = gcmd.get_float("SAMPLE_TIME", 0.100, above=0.0)
+        self._mode = gcmd.get("SCAN_MODE", "detailed")
     def run_probe(self, gcmd):
         toolhead = self._printer.lookup_object("toolhead")
         printtime = toolhead.get_last_move_time()
-        toolhead.dwell(self._sample_time_delay + self._sample_time)
-        start_time = printtime + self._sample_time_delay
+        if self._mode == "detailed":
+            toolhead.dwell(self._sample_time_delay + self._sample_time)
+            start_time = printtime + self._sample_time_delay
+        else:
+            start_time = printtime - self._sample_time / 2
         self._gather.note_probe(start_time, start_time + self._sample_time)
     def pull_probed_results(self):
         results = self._gather.pull_probed()

I believe with this change we can keep the scanning code in probe_eddy_current.py as bed_mesh can reuse the probe session interface when generating the rapid probe path. If you think this is appropriate I can make this change in the follow-up PR, otherwise I can use the add_client() method and grab the samples directly as originally proposed.

[KevinOConnor]

I believe with this change we can keep the scanning code in probe_eddy_current.py as bed_mesh can reuse the probe session interface when generating the rapid probe path.

Yeah, if we can reuse the code, I think that is a good idea. (As an aside, the code will also need to be updated to use toolhead.register_lookahead_callback().)

What do you think about using METHOD=rapid (or similar) instead of adding a new user-facing SCAN_MODE setting? It's not a big deal either way though.

Thanks.
-Kevin

[Arksine]

What do you think about using METHOD=rapid (or similar) instead of adding a new user-facing SCAN_MODE setting? It's not a big deal either way though.

That makes sense, removing the extra parameter is cleaner. Factoring in the need to register the lookahead callback, the new diff should look something like this:

diff --git a/klippy/extras/probe_eddy_current.py b/klippy/extras/probe_eddy_current.py
index 4bd4c4224..d47dfc780 100644
--- a/klippy/extras/probe_eddy_current.py
+++ b/klippy/extras/probe_eddy_current.py
@@ -261,9 +261,10 @@ class EddyGatherSamples:
             results.append(pos)
         del self._probe_times[:]
         return results
-    def note_probe(self, start_time, end_time):
-        toolhead = self._printer.lookup_object("toolhead")
-        nom_pos = toolhead.get_position()
+    def note_probe(self, start_time, end_time, nom_pos=None):
+        if nom_pos is None:
+            toolhead = self._printer.lookup_object("toolhead")
+            nom_pos = toolhead.get_position()
         self._probe_times.append((start_time, end_time, nom_pos))
 
 # Helper for implementing PROBE style commands (descend until trigger)
@@ -346,14 +347,29 @@ class EddyScanningProbe:
         self._gather = EddyGatherSamples(printer, sensor_helper,
                                          calibration, z_offset)
         self._sample_time_delay = 0.50
-        self._sample_time = 0.100
+        self._sample_time = gcmd.get_float("SAMPLE_TIME", 0.100, above=0.0)
+        self._method = gcmd.get("METHOD", "scan")
+        self._pending_positions = []
     def run_probe(self, gcmd):
         toolhead = self._printer.lookup_object("toolhead")
-        printtime = toolhead.get_last_move_time()
-        toolhead.dwell(self._sample_time_delay + self._sample_time)
-        start_time = printtime + self._sample_time_delay
-        self._gather.note_probe(start_time, start_time + self._sample_time)
+        self._pending_positions.append(toolhead.get_position())
+        toolhead.register_lookahead_callback(self._lookahead_cb)
+        if self._method == "scan":
+            toolhead.dwell(self._sample_time_delay + self._sample_time)
+    def _lookahead_cb(self, printtime):
+        if not self._pending_positions:
+            return
+        pos = self._pending_positions.pop(0)
+        if self._method == "scan":
+            start_time = printtime + self._sample_time_delay
+        else:
+            start_time = printtime - self._sample_time / 2
+        self._gather.note_probe(
+            start_time, start_time + self._sample_time, pos
+        )
     def pull_probed_results(self):
+        toolhead = self._printer.lookup_object("toolhead")
+        toolhead.wait_moves()
         results = self._gather.pull_probed()
         # Allow axis_twist_compensation to update results
         for epos in results:
@@ -390,7 +406,7 @@ class PrinterEddyProbe:
         return self.cmd_helper.get_status(eventtime)
     def start_probe_session(self, gcmd):
         method = gcmd.get('METHOD', 'automatic').lower()
-        if method == 'scan':
+        if method in ('scan', 'rapid'):
             z_offset = self.get_offsets()[2]
             return EddyScanningProbe(self.printer, self.sensor_helper,
                                      self.calibration, z_offset, gcmd)

I'm not sure if toolhead.wait_moves() is the best approach before pulling the results, or if it would be better to wait until self._pending_positions is empty.

[KevinOConnor]

Yeah, that should work.

I just realized though, that my code has a systemic inaccuracy that should be fixed. The "nominal position" should really be calculated from the stepper motor positions - not the toolhead/trapq positions. For example, if one is using a Z with rotation_distance=8, full_steps_per_rotation=200, and microsteps=16 then there is a step_distance of 0.0025. But, if one were to move to a Z position that isn't an exact multiple of that amount it could result in a systemic bias to the probe results. This wasn't an issue for "descend until trigger" mode because the toolhead position is recalculated at the end of the trigger, but in scan (and rapid scan) modes, the actual stepper Z may deviate from the requested Z. Probably not a huge precision issue on most printers, but could become an issue in some setups.

I'll update this branch to calculate the "nominal Z" from the stepper motor positions. The one upside of this change is that we wont have to track the nominal toolhead position upfront - it can be calculated in EddyGatherSamples from the stepper movement history and start/end_times.

I'm not sure if toolhead.wait_moves() is the best approach before pulling the results

Just "thinking out lout", simplest may be to call toolhead.get_last_move_time() in pull_probed_results() as that will flush the lookahead and ensure all register_lookahead_callback() calls are completed.

Cheers,
-Kevin

[KevinOConnor]

I reworked and rebased this branch.

Notes:

• This branch is still relative to the underlying probe refactoring (PR Internal rework of PrinterProbe class #6605).
• This branch now includes the sensor_bulk size change (for 2-byte response ids) as described at Support 2-byte message ids in host/mcu protocol #6613.
• The scan mode code now calculates the low-level kinematic position from the steppers - which should provide a more precise position.
• The code (both scan and descend-until-trigger) will now average the frequencies and then calculate a Z height from that average (as opposed to averaging the calculated Z heights for each sample). I think this should be slightly more accurate.
• I also tested a variant of @Arksine's "rapid_scan" snippet above. I included it on this branch, but I'll drop it from this series if it is in the way.

-Kevin

[c0psrul3]

testing this branch and found that i'm able to successfully do BED_MESH_CALIBRATE

• in detailed mode: successful
• in scan mode: error "probe_eddy_current sensor not in valid range"
• in rapid mode: error "probe_eddy_current sensor not in valid range"

any ideas here?

[KevinOConnor]

Thanks for testing.

If you're using one of the scan modes, you'll need to make sure the horizontal_move_z field is relatively low - try a command like: bed_mesh_calibrate method=scan horizontal_move_z=1.5 (And, you'll need to make sure the bed is relatively flat so that the nozzle does not strike the bed while moving at that low Z height.)

If that fails, run M112 immediately after the unexpected result, and then attach the full Klipper log file here.

-Kevin

[c0psrul3]

@KevinOConnor thanks, that seems to have solved my issue.
for usability, do you think there's anything that can be done to meet n00b user expectations? default values?
allowing horizontal_move_z to be set in probe_eddy_current config section? just ideas.

thank you again for the guidance.

[c0psrul3]

testing the different methods (scan works great) and got this error:
Probe triggered prior to movement
from command: BED_MESH_CALIBRATE METHOD=rapid PROFILE=rapid-test-1 horizontal_move_z=1.5

[Arksine]

I have had a chance to test the latest version if this branch and refactor bed_mesh to accommodate these changes. Overall it looks good to me, however I do have some observations:

1. For the rapid_scan method, I noticed that for samples where there is a direction change, the kinematic XY positions returned by EddyGatherSamples may slightly deviate from the requested position. This can cause an error in the probe_finalize() when bed_mesh attempts to validate faulty regions and/or when it transposes the results into a Z matrix. That said, I can increase the tolerance in bed mesh to accommodate the deviation, as long a we know it won't be too large. In my local work I have changed bed mesh to allow up to .5mm deviation on X and/or Y.

2. For the scan method, registering lookahead callbacks would likely speed up the probing procedure a bit, although it works fine as things stand. This change would likely also introduce the deviations noted above.

for usability, do you think there's anything that can be done to meet n00b user expectations? default values?
allowing horizontal_move_z to be set in probe_eddy_current config section? just ideas.

@c0psrul3 The [bed_mesh] section has a horizontal_move_z option that can be set to apply a default.

testing the different methods (scan works great) and got this error:
Probe triggered prior to movement
from command: BED_MESH_CALIBRATE METHOD=rapid PROFILE=rapid-test-1 horizontal_move_z=1.5

The correct method name is rapid_scan. The error you received was the result of the probing procedure falling back to the automatic method, ie: standard probing. FWIW, when you test rapid_scan you may encounter an error due to the issue I noted in #1 above.

[c0psrul3]

@Arksine thank you for correcting me, as i obv did not read the diff close enough.
i was referring to the fact that horizontal_move_z default is 5 and does not indicate much on how to resolve it.

@KevinOConnor rapid_scan immediately gives me error Probe triggered prior to movement, but does appear to work with horizontal_move_z=2 however the scan process then does hop between scans.
so, with BED_MESH_CALIBRATE METHOD=rapid_scan PROFILE=rapid-test-1 horizontal_move_z=2 the mesh appears to look nearly identical to the mesh from method=scan

klippy.log.20240609-1109.txt

[Arksine]

@KevinOConnor rapid_scan immediately gives me error Probe triggered prior to movement, but does appear to work with horizontal_move_z=2 however the scan process then does hop between scans.
so, with BED_MESH_CALIBRATE METHOD=rapid_scan PROFILE=rapid-test-1 horizontal_move_z=2 the mesh appears to look nearly identical to the mesh from method=scan

According to your log you are on commit aa507c8. The lastest commit in this branch after Kevin's last push is 2ac8242. You likely need to fetch and reset (or checkout detached) to update to the latest code.

The rapid_scan should be one continuous movement.

[c0psrul3]

@Arksine thanks for pointing this out 👍 rapid_scan appears to work, went through the motions, but got error message:
bed_mesh: Invalid y-axis table length
however, METHOD=scan appears to still work without issue.

klippy.log.20240609-1651.txt

[KevinOConnor]

Thanks for testing and reviewing.

I noticed that for samples where there is a direction change, the kinematic XY positions returned by EddyGatherSamples may slightly deviate from the "requested position".

Yeah - the latest code is calculating the x, y, and z position from the stepper motors. There are a few reasons the resulting "stepper position" may deviate from the requested position:

1. Due to minor rounding differences in floating point numbers.
2. Due to microstep coarseness. (For example, a requested position X=100.007 may not correspond to a whole microstep on the stepper.)
3. Due to input shaper. We'll actually be reporting the position of the steppers, and input shaper alters those positions relative to the requested positions - in particular during cornering.

It is possible to return requested XY positions, but it may also be worthwhile to return the actual XY positions. Not sure. Note that, even previously, probe XY positions could deviate slightly from requested positions (due to stepper coarseness - in particular on delta type kinematics).

For the scan method, registering lookahead callbacks would likely speed up the probing procedure a bit

Unless I'm missing something, there shouldn't be a difference. Calling dwell() should immediately flush all the lookahead callbacks. So, as far as I'm aware, calling register_lookahead_callback() followed by dwell() should produce the same result as print_time = toolhead.get_last_move_time() followed by dwell().

Cheers,
-Kevin

[Arksine]

It is possible to return requested XY positions, but it may also be worthwhile to return the actual XY positions. Not sure. Note that, even previously, probe XY positions could deviate slightly from requested positions (due to stepper coarseness - in particular on delta type kinematics).

I don't think we need to return the requested positions, I have a fix for this in bed_mesh. It checks against the received positions and logs large deviations, but it uses the positions generated by bed mesh to transpose the matrix.

Unless I'm missing something, there shouldn't be a difference. Calling dwell() should immediately flush all the lookahead callbacks. So, as far as I'm aware, calling register_lookahead_callback() followed by dwell() should produce the same result as print_time = toolhead.get_last_move_time() followed by dwell().

I think you're right. I noticed that it took longer and assumed it introduced a difference, but after a closer look its likely due to self._sample_time_delay being set to .5 rather than .05 which is what was previously used.

[KevinOConnor]

but after a closer look its likely due to self._sample_time_delay being set to .5 rather than .05 which is what was previously used.

Eeks! I must have goofed on one of my rebase jobs. It was intended to be 0.050.

I restored the value to 0.050. I also merged in the backend probe work (PR #6605), merged the 2-byte command id work (PR #6613), and rebased this branch on top of the latest master.

-Kevin

[c0psrul3]

@KevinOConnor thanks for putting some updates.
i'm still getting the error bed_mesh: Invalid y-axis table length
with the following additional output to console:

bed_mesh: Invalid y-axis table length
Probed table length: 30 Probed Table:
...

what i do notice is the output Probed table length: 30 while i have configured in bed_mesh: probe_count: 15,15

thanks!

EDIT:
tried a few different values for probe_count and it consistently gives 2x of y-value from probe_count for Probed table length in output

EDIT:
add complete mesh table example (probe_count: 10,5)

Probed table length: 10 Probed Table:
[[0.13948000000000027, 0.11794200000000021, 0.09388700000000028, 0.09322800000000031, 0.08988200000000024, 0.08212400000000031, 0.06828500000000015, 0.044971000000000316, 0.03268700000000013, -0.010650999999999744], [-0.019593999999999667], [0.06679900000000027, 0.05984800000000012, 0.0636420000000002, 0.06671100000000019, 0.06403500000000029, 0.05484000000000022, 0.041493000000000224, 0.029705000000000314], [0.07425400000000026], [0.030070000000000263], [0.0359440000000002, 0.036696000000000284, 0.04737600000000031, 0.05553700000000017, 0.05721900000000013, 0.04842100000000027, 0.03242500000000015, 0.017195000000000293, -0.018325999999999842], [-0.03324499999999975], [-0.03331799999999974, -0.02686799999999967, -0.02284699999999984, -0.012907999999999697, 0.0009810000000001207, 0.011664000000000119, 0.009973000000000232, 0.003603000000000245, -0.0005309999999998372], [-0.0792609999999998], [-0.08605499999999977, -0.07997099999999979, -0.07212299999999972, -0.06187099999999979, -0.051767999999999814, -0.05245799999999967, -0.05827199999999988, -0.06190499999999988, -0.09390499999999968]]```

[Sineos]

@c0psrul3, thanks for testing.
Without this case in particular, it is generally recommended to call a M112 immediately after an issue and just post the log. This will give the devs the best possible information to continue working with.

[KevinOConnor]

i'm still getting the error bed_mesh: Invalid y-axis table length

As indicated at #6610 (comment) , the current bed_mesh code would need changes in order to be used with "rapid_scan".

I'll drop the "rapid_scan" part of this PR prior to committing.

-Kevin

[Arksine]

I have created PR #6617, adding support for rapid scanning in bed_mesh. It should resolve the errors discussed above.

[c0psrul3]

@KevinOConnor @Arksine thank you again for your efforts. combined, these changes perform excellent with no issues.

@KevinOConnor I wanted to point something out which was interesting to me, the map for METHOD=scan looks smoothed when compared to the map from METHOD=rapid_scan. this is also revealed by comparing each profile's deviation (0.137 for "scan" :: 0.170 for "rapid_scan"). klipper source is not well known to me, as i am new to looking under the hood, but still wanted to comment since I would expect the "rapid" scan to be less precise (but maybe i'm wrong here).

i am attaching screenshots of the maps (probe_count=30)

lmk if i'm off-base here and again, thank you both for the excellent contributions!

[KevinOConnor]

Thanks for testing.

the map for METHOD=scan looks smoothed when compared to the map from METHOD=rapid_scan. this is also revealed by comparing each profile's deviation (0.137 for "scan" :: 0.170 for "rapid_scan").

Well, I guess that's what I would have expected - the "scan" mode would produce a more accurate map, and the "rapid_scan" would produce a map with more "noise". That is, rapid_scan runs a little faster but isn't as accurate.

Or are you saying you've done some kind of analysis showing that your bed is actually very "crinkly" like the rapid scan shows?

Cheers,
-Kevin

[c0psrul3]

lol, my n00b is showing. I've not done any analysis and noise didn't occur to me. it is textured, but that wouldn't show at 30,30. noise would make better sense than accuracy. in any case, it all looks good and ready imo

[KevinOConnor]

I merged the "scan" mode support in this PR into the main repo. I did not merge the initial "rapid_scan" work previously discussed here - it looks like that can be done in the upcoming PR #6617.

-Kevin