filament_sensor: advanced filament detection #1301

Arksine · 2019-02-23T01:04:08Z

This PR contains an advanced implementation for filament detection. Features include:

Runout and insert detection. By default the sensor module monitors the current print status and enables the correct detection mechanism based on the status (runout while printing, otherwise insert).
Executes a user configured gcode script when an event is detected. If the gcode is not configured the event is disabled.
Allows for multiple underlying sensor implementations. Includes a default switch based implementation.
Will not allow multiple events to execute concurrently.
Does not call a gcode handler from inside a reactor callback or button event.
Allows the user to specify a minimum event delay time. This is desirable to keep multiple events from executing too close together.
Exposes filament sensor API for other Klipper modules to monitor filament events and set their own event handlers.
Switch sensor ignores false positives. A change in button state must hold for 100ms before the event triggers.

There are a couple of things in here that may require explanation. First, when pausing Octoprint, I have to send the action command from inside the runout event handler before the current move is acknowledged. Otherwise Octoprint will queue up the next move after acknowledgement, which will be executed after the PAUSE gcode. To make PAUSE optional on runout I do a nested search on the runout_gcode for the PAUSE command. This is why I added the contains() function to gcode_macro.

Second, changes to gcode.py are necessary in order for the SD card to pause immediately, as mentioned in proposal outlined in #1236. Alternatively I could directly call cmd_M25 inside the runout event handler as PR #1098 does. I don't think this would have any negative consequences as it just sets must_pause_work to True, but it probably isn't good practice.

Signed-off-by: Eric Callahan arksine.code@gmail.com

KevinOConnor · 2019-02-24T04:18:47Z

Thanks. I have a couple of high-level comments.

I think it would be preferable to be able to define a [filament_switch_sensor my_sensor] and be able to define a [pat9125 my_other_sensor]. That is, I think both the config and the implementation would be easier to understand if the hardware specific code (eg, SwitchSensor() ) was the main object and that it owned a FilamentSensorHelper(), instead of the other way around.
I'm struggling to understand the PAUSE mechanism. It seems like the goal is to run special logic if a PAUSE request is detected in the runout_gcode. If so, I think it would be preferable to instead add a "pause_on_runout" config option (default to true) that enables the special pause logic. I think that logic can simply be printer.try_load_module("pause_resume").do_pause_from_event(). As for pausing the virtual_sdcard, I think we could also add a virtual_sdcard.do_pause_from_event() handler.
Very minor, but if some buttons need a particular debounce period (more than the current 2ms debounce), then I wonder if adding that logic to klippy/extras/button.py is a better place for it.

-Kevin

Arksine · 2019-02-24T12:43:03Z

That is doable. One reason I didn't take that approach initially is that I wanted to restrict the PAT9125 from being a prefix module as its an i2c device. However I suppose it would be possible to have more than one on MCUs that have multiple i2c buses. I'll look into reversing the relationship.
You have it correct. I think your alternative is reasonable.
I don't know if the 100ms debounce is absolutely necessary, but I thought it would be wise given the vast number of ways a switch style sensor can be implemented. The endstop sensors are the most common, and should be reliable. On the MMU2 prusa has a strange filament sensor (they love doing this) where they use an inductive probe and a steel ball. Filament pushes the ball toward the probe to trigger it. I could see this type of implementation being prone to false positives. Regardless, I wouldn't be against moving it to buttons.py.

I'll get on these items and have it updated as soon as I can.

Arksine · 2019-02-25T19:59:43Z

Okay, I have added some commits to address some of the current issues:

I added a send_pause_command(self, from_event=False) to pause_resume.py and do_pause_from_event() to virtual_sdcard.py. This solves the issues with pausing from inside an event. To be honest, do_pause_from_event() does the exact same thing cmd_M25() does, but the upside is that if anyone is looking at the code perhaps it will reinforce that gcode handlers generally shouldn't be called from reactor callbacks.
The second commit renames the module to filament_switch_sensor.py and refactors it so that the FilamentSensor class is now the BaseSensor class. All sensor implementations will need to subclass it, implement query_sensor() and do event detection. They can optionally override functions as necessary. I chose this option because when another module does a lookup on a filament sensor they have direct access the the correct API (they will need to call set_enable and set_callbacks if they want to monitor the sensor status themselves). I made sure the non-accessable functions are indicated private.
The third commit is just an update to the configuration.

I still need to look into moving the additional debounce period to buttons.py, I think I have an idea of how to do it without breaking the API for all other callers. I also think it might be useful to add a get_filament_sensors() function to the module, because it will be possible to have multiple prefixes when other sensor implementations are added.

KevinOConnor

Thanks. In general it looks good to me. I am a bit confused by the callbacks and the use of the reactor (see below), but nothing that would prevent merging.

KevinOConnor · 2019-02-26T03:46:27Z

config/example-extras.cfg

+#[filament_switch_sensor my_sensor]
+#pause_on_runout: True
+#  When set to True, a PAUSE will execute immediately on runout.  Note
+#  That when runout_on_pause is set to True the runout event will be


Typo - should be "pause_on_runout".

Oops, I'll fix the typos, formatting issues, and clarify the gcode descriptions as recommended.

KevinOConnor · 2019-02-26T03:47:17Z

config/example-extras.cfg

+#  The gcode to execute after a filament runout event.  If pause_on_runout is
+#  set to True this gcode will execute after the pause is complete.  Otherwise
+#  it will execute as soon as resources are available.  If the runout_gcode
+#  is omitted and runout_on_pause is set to False, the runout event is disabled.


It's not clear what a "runout event" is. Could this just be worded as "The default is to not run any g-code commands".

KevinOConnor · 2019-02-26T03:48:04Z

docs/G-Codes.md

+## Filament Sensor
+
+The following command is available when a "filament sensor" config section is enabled.
+ - `QUERY_FILAMENT_SENSOR SENSOR=<sensor_name>`: Querys the current status of the filament sensor.  The data displayed on the terminal will depend on the sensor type defined in the confguration.


Typo - "queries". Also, in keeping with the formatting of this file, lines should be wrapped at 80 columns.

KevinOConnor · 2019-02-26T03:49:05Z

klippy/extras/filament_switch_sensor.py

+        for status in ["idle", "ready", "printing"]:
+            self.printer.register_event_handler(
+                "idle_timeout:%s" % (status),
+                (lambda e, s=self, st=status: s._update_print_status(st)))


FWIW, I think the code would be more readable if explicit handle_idle(), handle_ready(), and handle_printing() methods were used.

Okay, I will change this.

KevinOConnor · 2019-02-26T03:50:02Z

klippy/extras/filament_switch_sensor.py

+        self.gcode.register_mux_command(
+            "QUERY_FILAMENT_SENSOR", "SENSOR", self.name,
+            self.cmd_QUERY_FILAMENT_SENSOR,
+            desc=self.cmd_QUERY_FILAMENT_SENSOR_help)


FWIW, I think the code would be more readable if the command code and its registration was done in SwitchSensor() (and any other descendent classes).

Okay, I will make that change. I do think it would still be wise to keep the following in the base class:

def cmd_QUERY_FILAMENT_SENSOR(self, params): raise NotImplementedError( "Sensor must implement cmd_QUERY_FILAMENT_SENSOR")

This makes sure subclasses implement the correct gcode.

KevinOConnor · 2019-02-26T04:02:21Z

klippy/extras/filament_switch_sensor.py

+        except:
+            raise
+    def set_callbacks(self, runout_cb=None, detected_cb=None, monitor=True):
+        # Allow other modules to receive sensor status notifications


I don't understand what this method does - it isn't invoked anywhere. If the goal is to allow other modules to be aware of this event, then I'd use self.printer.send_event("filament_switch_sensor:my_event", some, parameters)

Yes, this method is specifically for external modules to be able to set their own callbacks. I have some concerns with using Klippers internal event system for this, as it would execute a callback for every module registered. I'll provide an example:

A module (such as MMU) is changing filament, and it wants to be notified when filament triggers the sensor closest to the extruder. To do this it would have to change the behavior of the sensor, as it would need to enable insert detection while the printer is in a "printing" state. Also, it wouldn't be desirable for the built-in callback to execute the autoload_gcode while its being used for this.

There are ways around this. Let me think on this and I'll see if I can come up with a solution. I do think that I would need a more generic name for the event prefix, I wouldnt want to have to register event handlers for each underlying implementation (ie: filament_switch_sensor:runout_event, pat9125:runout_event, etc)

FWIW, I'd say add that callback support when the software for the first callee is added. It's hard to predict what future code will need.

KevinOConnor · 2019-02-26T04:03:35Z

klippy/extras/gcode_macro.py

@@ -37,6 +37,8 @@ def cmd(self, params):
            self.gcode.run_script_from_command(script)
        finally:
            self.in_script = False
+    def contains(self, gc):
+        return gc.upper() in self.script.upper()


I think this change should no longer be necessary.

Yes, I will remove it.

KevinOConnor · 2019-02-26T04:04:39Z

klippy/extras/pause_resume.py

+                if from_event:
+                    self.v_sd.do_pause_from_event()
+                else:
+                    self.v_sd.cmd_M25({})


Could this just be self.v_sd.do_pause() instead of making the distinction between M25 and do_pause_from_event()?

Yep, not a problem.

KevinOConnor · 2019-02-26T04:12:46Z

klippy/extras/filament_switch_sensor.py

+                reactor.monotonic() + 2.)
+    def _update_print_status(self, status):
+        logging.info(
+            "filament_sensor: print status changed to: %s" % (status))


This adds logging for every printer state change. I'm not sure that is desirable. If it is desirable, it should be added to idle_timeout.py and be done separate from this series.

You are correct, I used that to make sure the internal state was updating correctly, I will remove it.

KevinOConnor · 2019-02-26T04:22:44Z

klippy/extras/filament_switch_sensor.py

+            return
+        self.event_button_state = state
+        self.reactor.update_timer(
+            self.switch_event_timer, eventtime + SETTLE_TIME)


I'm not sure what this is attempting to do, but it doesn't seem right. Timing in the host code is always a bit iffy because there could be variable amounts of delays between the events and their reception in the host. Better to get the actual event time (a clock/print_time from the micro-controller) or to use the sent/receive_time from the actual timestamp of the received micro-controller messages.

At a high-level I don't think the filament sensor should need to use the reactor at all. The reactor is more intended to schedule system events - at first glance the filament sensor is just reacting to internal events.

This timer makes sure that a change in switch state is held for ~100ms before executing a callback. I see where the issue is here, I shouldn't use eventtime to update the timer.

I moved this functionality to buttons.py. There I use reactor.monotonic() to set the wake time. This probably isn't absolutely necessary functionality, I thought it would be wise given that the switch will likely move around and have filament running past it. Depending on the way the switch is mounted this could result in the switch triggering prematurely. If you do not like it I have no problem removing it.

FWIW, I'd say add support for extended debouncing when we get confirmation that it is required. There's a good chance the troublesome hardware will need something different than anticipated.

I'm battling with the cheap filament runout sensor that came with my Two Trees Sapphire Pro. It keeps giving false alerts and pausing my prints. I have a feeling a longer debounce could be helpful in my case

This allows for pausing from inside a reactor callback. Signed-off-by: Eric Callahan <arksine.code@gmail.com>

…nt runout/insert sensor This implementation includes a BaseSensor class that all underlying sensor implementations should subclass. Signed-off-by: Eric Callahan <arksine.code@gmail.com>

Signed-off-by: Eric Callahan <arksine.code@gmail.com>

Arksine · 2019-02-26T22:07:23Z

I have force pushed with the above recommendations, including removal of the debounce timer and the set_callbacks() function.

Since I moved the gcode registration to the subclass, I also moved registration for the klippy:ready event there, that should keep subclasses from registering it twice.

Finally, I renamed autoload_gcode to insert_gcode, as that seems more consistent with the rest of the module.

KevinOConnor · 2019-02-27T16:55:17Z

Thanks. It looks good to me. If there are no further comments I'll look to commit in the next couple of days.

FYI, my earlier comment on the calls to self.printer.register_event_handler() was not the loop, but the use of the lambda. It's not an issue though.

-Kevin

KevinOConnor · 2019-03-01T05:46:34Z

Thanks!

-Kevin

Arksine force-pushed the work-filament_sensor-2_8_19 branch from d167279 to 30dff32 Compare February 23, 2019 01:11

KevinOConnor reviewed Feb 26, 2019

View reviewed changes

Arksine added 3 commits February 26, 2019 17:01

pause_resume: Implement send_pause_command()

c97da00

This allows for pausing from inside a reactor callback. Signed-off-by: Eric Callahan <arksine.code@gmail.com>

filament_switch_sensor: initial implementation of switch based filame…

f0afb03

…nt runout/insert sensor This implementation includes a BaseSensor class that all underlying sensor implementations should subclass. Signed-off-by: Eric Callahan <arksine.code@gmail.com>

filament_switch_sensor: update documentation

f3f436d

Signed-off-by: Eric Callahan <arksine.code@gmail.com>

Arksine force-pushed the work-filament_sensor-2_8_19 branch from f2203ca to f3f436d Compare February 26, 2019 22:05

Hywelmartin mentioned this pull request Feb 28, 2019

Filament jam sensor #1327

Closed

KevinOConnor merged commit 69491e2 into Klipper3d:master Mar 1, 2019

KevinOConnor mentioned this pull request Mar 1, 2019

Filament_runout_sensor: Module to use one or mode filament sensors #1098

Closed

Arksine deleted the work-filament_sensor-2_8_19 branch March 27, 2019 00:32

KevinOConnor mentioned this pull request Jul 1, 2019

Filament runout sensor #545

Closed

4 tasks

github-actions bot locked and limited conversation to collaborators Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filament_sensor: advanced filament detection #1301

filament_sensor: advanced filament detection #1301

Arksine commented Feb 23, 2019

KevinOConnor commented Feb 24, 2019

Arksine commented Feb 24, 2019

Arksine commented Feb 25, 2019

KevinOConnor left a comment

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

KevinOConnor Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019 •

edited

KevinOConnor Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019

KevinOConnor Feb 26, 2019

Arksine Feb 26, 2019 •

edited

KevinOConnor Feb 26, 2019

thucar Oct 21, 2020

Arksine commented Feb 26, 2019

KevinOConnor commented Feb 27, 2019

KevinOConnor commented Mar 1, 2019

filament_sensor: advanced filament detection #1301

filament_sensor: advanced filament detection #1301

Conversation

Arksine commented Feb 23, 2019

KevinOConnor commented Feb 24, 2019

Arksine commented Feb 24, 2019

Arksine commented Feb 25, 2019

KevinOConnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arksine Feb 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arksine Feb 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arksine commented Feb 26, 2019

KevinOConnor commented Feb 27, 2019

KevinOConnor commented Mar 1, 2019

Arksine Feb 26, 2019 •

edited

Arksine Feb 26, 2019 •

edited