Extend and Improve Interruption

[garethky]

This is a set of changes that causes the printer to stop what its doing when the existing HEATER_INTERRUPT gcode is run. This gcode triggers the internal interrupt mechanism inside GCodeDispatch. This is now extended to give a contract that much more similar to how VirtualSD works: between each gcode command there is a check to see if the interrupt has been triggered and this stops further execution of the gcode script.

This results in the following improvements to the cancel experience:

• Interruption is now instant because its using a ReactorCompletion, not polling
• Instant interruption now happens in the most frequently blocking locations, primarily waiting inside the toolhead and heater waiting.
• Homing and Probing moves (drip moves) can be interrupted mid-move
• Extras that interact with the toolhead are now interruptible, these tend to be long running programs like bed_mesh
• GCode Macros can now be interrupted

The existing counter based mechanism was replaced with an InterruptToken that wraps a ReactorCompletion. InterruptToken is a view/observer of the underlying completion. When trigger_interrupt() happens the ReactorCompletion is completed and all of its views subsequently wake if they are being waited upon.

Explanation

The behavior that we want to achieve is interrupting and cancelling whatever the printer is currently doing while allowing a script of cleanup commands to continue to run successfully. E.g. the user might write a macros like:

[gcode_macro CANCEL_INTERRUPT]
gcode:
    HEATER_INTERRUPT
    TURN_OFF_HEATERS
    PARK_TOOLHEAD

In this case we want any ongoing GCodeCommands to be interrupted when HEATER_INTERRUPT runs. But crucially, the subsequent command to shut off the heaters and park the toolhead should be run normally.

This means the interruption state is tied to gcode commands, represented by gcmd/GCodeCommand objects. Gcode commands are processed through the GCodeDispatch._process_commands() method. This is recursive, both macros and extras can invoke additional gcode commands creating a call stack into _process_commands().

When HEATER_INTERRUPT runs, its important that commands up the stack from that point uniformly observe that they are interrupted. This includes when the final gcode in the stack eventually returns and the stack begins to unwind. So the current global state is not the correct state to observe, the right state is the one tied to the gcmd that is currently at the top of the stack.

To make this more difficult, the gcmd object is not universally available in all the places where we want to get that context. Printer.wait_while() doesn't have a gcode command context. gcode_move.py and the system of movement transforms don't pass the active gcmd through that stack to toolhead.move(). It would be an invasive change to thread this state through all of these paths . So we need some way to get the interruption state for the current gcmd on top of the stack without having the gcmd object.

To make this more complicated, to get CANCEL_INTERRUPT to run immediately, we bypass the mutex that protects GCodeDispatch._process_commands(). This allows 2 greenlets to concurrently enter that block of code. They have independent stacks, so now there are 2 commands at the top of 2 different stacks. So its not good enough to simply look at entry and exit from _process_commands() as the boundaries of a stack frame, we have to track which greenlet (green thread) is actually in the method.

The new GCodeCommandStacks tracks a stack of GCodeCommands for each greenlet. Via this class its possible to get the correct InterruptToken from any context in the program. This class is doing the state tracking that would have been required by individual call sites. Its entirely encapsulated inside GCodeDispatch so extra/plugin developers don't have to think about how it works.

In the future if everywhere was passed a gcmd object and the GCodeDispatch.get_interrupt_token() method was deleted, then GCodeCommandStacks could also be deleted. The gcmd object has the right state internally and will always behave correctly. But right now that change looks invasive and impractical.

Checklist

• pr title makes sense
• added a test case if possible
• if new feature, added to the readme
• ci is happy and green

[dalegaard]

This seems wrong since self._interrupt_token gets assigned to this with a type cast to : InterruptToken. Either here or at the assignment site, an "inert" InterruptToken should be picked. Basically one that will never trigger but otherwise work completely normally. Since this could just be a global that everyone shares, I'd lean towards putting a default here instead of at the assignment site further down.

[garethky]

yeah, I had it be None to see if that would get it past the unit tests, but ill just have to fix the test.

[dalegaard]

For compatibility with Klipper modules we need to keep self.error around anyway, so keep the existing self.error usage instead of using the new name.

[garethky]

self.error still exist. I just didn't use to try and convey to readers that its not the original.

[dalegaard]

Would be good with a was_interrupted() as well, a non-throwing version of this.

[garethky]

sure, I can put a test() wrapper there and call it was_interrupted().

[dalegaard]

Can this hit a race? ReactorCompletion is known to be racy. Might be better to fix the race in ReactorCompletion.wait but I don't know if anything somehow relies on that race.

[garethky]

I don't think so. test() just checks if the completion is assigned any value.

The only "racy" thing in ReactorCompletion that I know of is that it allows the value being overwritten by multiple callers of complete(). I think that's a bug/defect (at least vs other systems I've worked with). This completion is only completed from 1 place and the complete() method is wrapped so holders of InterruptToken cant call complete().

Even if you spam INTERRUPT commands, they still end up executing sequentially.

[dalegaard]

ReactorCompletion I believe is racy today because the waiter checks for completion, and if not done it adds itself to the completion list. But the resolve side sets the value and then goes through the list. So I believe the following race can happen:

Waiter: check value, find not set
Completer: set value
Completer: get and invoke all waiters
Waiter: register in wait list

There's no documentation or other mechanism that prevents using ReactorCompletion on different threads as far as I'm aware.

[garethky]

The example you are giving here is possible only if ReactorCompletion.complete() is called directly from another OS thread.

Inside the reactor/greenlet model, there is no yield between:

if self.result is self.sentinel:
    wait = greenlet.getcurrent()
    self.waiting.append(wait)
    self.reactor.pause(waketime)

The greenlet does not give up control until reactor.pause(), and by that point the waiter has already been appended. Similarly, complete() does not yield. It sets the result and updates the waiting timers synchronously.

For cases where a separate OS thread needs to complete a completion, there is the async_complete() method. That queues the completion back onto the reactor thread via the async pipe. Existing cross-thread users are using that path, for example serial/mcu response handling.

So I agree that ReactorCompletion.complete() should not be called directly from arbitrary threads. But I don’t think this PR introduces that race: HEATER_INTERRUPT runs through gcode dispatch on the reactor/greenlet side, and the interrupt completion is completed from that same cooperative context.

If we want to make this harder to misuse, GCodeDispatch.trigger_interrupt() could be made private so callers don’t treat it as a thread-safe entry point.

[dalegaard]

Could this race somehow? What if a new gcode command samples self._interrupt_completion after the complete call but before the new assignment? Will this new gcode command be un-interruptible or one that immediately interupts?

[garethky]

So in normal threads that instinct is correct. This would have to be a synchronized method or you would have to grab a mutex to make the switch.

But in greenlets only 1 of them is running at a time. So there is no other way in here at the same time as another greenlet.

complete() doesn't yield. It just updates the timers of the associated list of waiting greeenlets. You would need something in here to yield to get a race.

[dalegaard]

Can we somehow mark or comment that calling this outside of a greenlet running on the reactor is prohibited? There's really nothing preventing calling this from e.g. an API handler, which doesn't have to be in a greenlet necessarily.

[garethky]

Agreed. I was just writing this in the other reply.

I feel that this function on line 280 is OK, because its inside a private variable inside GCodeDispatch. If you reach inside a private variable to call a method from another OS thread then... really you can do any number of other bad things.

But the public method on GCodeDispatch itself looks like it should be made private to protect against this.

[dalegaard]

I could see us wanting to at some point want to do this via the API as well and maybe we end up doing something with threads there, so really I just want to be sure that we can't end up shooting ourselves in the foot down the line when this specific detail is lost to time. Assuming all of these function(this one and the other racy one I was concerned about) are only ever called within the context of the reactor, I agree they should be safe.

[garethky]

Good point. We can have a public API like GCodeDispatch.async_trigger_interrupt() that calls this:

    def async_trigger_interrupt(self):
        self._printer.get_reactor().register_callback(lambda: self.trigger_interrupt())

That way the interrupt signal from the OS thread is passed into the reactor and run on a greenlet. That's the same pattern that Printer uses for invoke_async_shutdown.

[dalegaard]

I don't agree that the interruption context is tied to the gcode command. Interruption context is printer wide, whenever an interruption is triggered all commands preceeding the interruption command must abort. This is why the counter approach worked, everything was sampling the same counter and if it changed, everything took that to be an interruption. I think the entire stacks system can be done away with and replaced with a single "current completion" that gets replaced every time an interrupt is called. Your example interrupt macro example should still work in that case, because the interrupt command is called before the following commands sample the current completion and so they will correctly be allowed to run.

We do need to look at how we control the gcode command mutex though, because right now we could easily end up with running e.g. multiple commands in parallel just because the text "interrupt" exists somewhere in the macro. That's a separate issue from what you are working on here, but thought I'd note it.

/Lasse

[garethky]

We do need to look at how we control the gcode command mutex though, because right now we could easily end up with running e.g. multiple commands in parallel just because the text "interrupt" exists somewhere in the macro. That's a separate issue from what you are working on here, but thought I'd note it.

Agreed, I don't love it.

I would make 3 small change:

1. I don't like the "secret knock" aspect of this. Id rather pass an argument to register_command that makes the gcode an interruption. Then build a list of these and check it. That would stop users writing random scripts that bypass the mutex. Then I would register CANCEL_PRINT as an interrupting gcode. Users can then override these scripts if they want specific behavior.

2. I would trigger the interruption before any of these special gcodes run. Inside run_script(). That means that the cancellation happens clearly before the script begins. This stops any commands in the script executing before the interrupt happens. E.g. right now this looks like it runs a move while the printer might be doing something else:

[gcode_macro CANCEL_INTERRUPT]
gcode:
    # lift Z
    G91
    G1 Z15
    HEATER_INTERRUPT

3. If we did No. 2, I think we could just execute the interrupt script with the mutex as normal. When we try to acquire the mutex the greenlet should yield. Then the stack should raise the exception and unroll. And we want all of the unrolling behavior (any finally blocks) to run before running the next command.

[dalegaard]

1. I don't like the "secret knock" aspect of this. 

Completely agree!

Id rather pass an argument to register_command that makes the gcode an interruption. Then build a list of these and check it. That would stop users writing random scripts that bypass the mutex. Then I would register CANCEL_PRINT as an interrupting gcode. Users can then override these scripts if they want specific behavior.

I think a big part of the reason we have these issues is that run_script takes the mutex and then runs everything under that, but in practice we want to be able to execute multiple commands in parallel as long as they don't all interact with the "printer state"(which is really what the gcode mutex protects).

I think the correct option is actually to do away with taking the mutex in run_script and instead take it per-command within _process_commands. Reentrency becomes a bit more messy, _process_commands now needs a take_locks= argument so callers can communicate they already have the lock.

With this, instead of making interruptions special, we can give register_command an run_under_mutex=True argument(name TBD obviously). When the _process_commands loop executes a command, it checks if the mutex should be taken and compares it to the current mutex state, and if different takes/releases the mutex accordingly. Updating the mutex state as we go, instead of lock/unlock per command should maintain the current semantics as seen externally(a full sequence of gcode in a single run_script call gets to run to completion).

An example macro like the one we've been working with here, that starts with a HEATER_INTERRUPT call(which would be registered with run_under_mutex=False) can then go ahead and run that interrupt right away, and the following commands will then take the mutex as normal.

Does that make sense?

/Lasse

[garethky]

I think the correct option is actually to do away with taking the mutex in run_script and instead take it per-command within _process_commands.

Yes, that makes total sense. There are a lot of commands you want to run now, even if they do alter printer state. Babystepping come to mind. That could be directly handled by the toolhead to alter the next thing it sends to the MCU. We have a "bug" in klipper were you try to adjust your Z height on layer 1 and the commands all get piled up at the end of a long print line.

Edit: taking the mutex in _process_commands would allow two scripts to run in parallel and it would interleave their commands, it would just trade the mutex back and forth between the two threads. That feels pretty dangerous.

Something else that dawned on me this morning: all the threads that are waiting on the mutex when trigger_interrupt() happens should raise an exception (WaitInterruption? they were sort of waiting). Those are all commands that belong to the generation before the interrupt and so they should never run. I don't know if there is a cleaner way to do this, like directly raising that exception on those threads, but we could swap the mutex instance and do this:

local_mutex = self.mutex
with local_mutex:
    if local_mutex != self.mutex:
        raise WaitInterruption("Command interrupted")

[dalegaard]

Yes, that makes total sense. There are a lot of commands you want to run now, even if they do alter printer state. Babystepping come to mind. That could be directly handled by the toolhead to alter the next thing it sends to the MCU. We have a "bug" in klipper were you try to adjust your Z height on layer 1 and the commands all get piled up at the end of a long print line.

The baby stepping issue is a very good example of where this could be useful, I agree!

taking the mutex in _process_commands would allow two scripts to run in parallel and it would interleave their commands, it would just trade the mutex back and forth between the two threads. That feels pretty dangerous._

It doesn't need to, maybe I explained badly. _process_commands could do this:

have_lock = False
try:
  for cmd in commands:
    want_lock = cmd.must_lock
    if want_lock and not have_lock:
      acquire_lock()
      have_lock = True
    elif not want_lock and have_lock:
      release_lock()
      have_lock = False
    cmd.run()
  finally:
    if have_lock:
      release_lock()

This way, once the lock is taken, it will be held either until we get to a command that doesn't require the lock, or we are done with all of our commands. This prevents the ping-pong issue. Alternatively, we could say that once the lock has been requested by a command, it will be held for the remainder of the command sequence. Either can work I think.

Something else that dawned on me this morning: all the threads that are waiting on the mutex when trigger_interrupt() happens should raise an exception (WaitInterruption? they were sort of waiting). Those are all commands that belong to the generation before the interrupt and so they should never run. I don't know if there is a cleaner way to do this, like directly raising that exception on those threads, but we could swap the mutex instance and do this:

This is a pretty nice observation, and I think we can actually avoid swapping the mutex and simply augment the ReactorMutex implementation with a new acquire type method that can abort an acquire operation if the provided InterruptToken is triggered. That would then replace the acquire_lock() call in my pseudocode above.

/Lasse

[garethky]

Ok, if I combined our ideas together I can do the interruption without having to get into the baby stepping use case. If I move the mutex logic inside _process_commands, then I don't need the "no mutex" behavior for interrupting commands. The interrupt happens before the command, cancelling everything, then the command takes the mutex immediately because nothing will be holding it. No mutex bypass needed.

The interrupting behavior needs to follow any command override. Consider this:

[gcode_macro CANCEL_PRINT]
rename_existing: BASE_CANCEL_PRINT
gcode:
    TURN_OFF_HEATERS
    CLEAR_PAUSE
    BASE_CANCEL_PRINT
    SET_GCODE_OFFSET Z=0.0
    SAFE_LIFT_Z Z=50 F={15 * 60}

if BASE_CANCEL_PRINT triggers an interrupt this falls apart. And I don't want to force users to re-write their macros for this. So the logical thing is to have the interrupt behavior be a property of the base macro and have it transferred to the override when its created. We could even allow users to write their own interrupting macros with an attribute in the config (but i'll skip this for now):

[gcode_macro MY_STOP_MACRO]
interrupt_printer: True
gcode:
    SAFE_LIFT_Z Z=50 F={15 * 60}

We can leave the bypass-mutex-run-in-parallel idea until we do a use-case like baby stepping where we want to think about how that would work. I've been thinking about a move transformer that would split long slow moves into chunks that are shorter than 1/10th of a second. That way baby stepping could take effect "mid-move" and this would still not impact step compression too much in real prints.

[garethky]

It doesn't need to, maybe I explained badly. _process_commands could do this:

What happens when an inner recursive command releases the lock? Lets say a module calls some user injected gcode and its last command releases the lock. When that execution returns, the mutex has been release and it wont be locked again until the next command. This opens up a window for the module that ran the script to alter printer state while it no longer owns the mutex. That is something that is not safe e.g.:

def cmd_CUSTOM(self, gcmd):
    # this command acquired the mutex to run
    # execute some user configured gcode that releases the mutex:
    self.gcode.run_script_from_command('SET_GCODE_OFFSET Z_ADJUST=0.1 MOVE=1')
    # mutex unlocked for command CUSTOM, unsafe state modification:
    self.toolhead.set_position(...)

To be safe, if the stack frame that called _process_commands had the lock on the way in, it needs to be locked on the way back out. This is whats happening today with run_script.

I'm just going to avoid this in this PR. I think we need to do it, but its not required, yet.

[dalegaard]

I'm not sure what you mean. As I said earlier, _process_commands would get a parameter to indicate if it should take locks at all, and run_script_from_command would set this to false, so locks won't be touched at all.

[garethky]

ok, got it

[garethky]

I decided the variable tracking was just too tricky. So I put a method on ReactorMutex so you can just test if the current greenlet owns the lock: mutex.owns_lock(). Now we can assert that the lock is owned at certain points, its reads a lot better and I think its easier to reason about. It also lets us do entry checking on run_script and run_script_from_command to see if they are being misused.