Skip to content

Fix to allow canBUS MCUs to connect and disconnect mid print#905

Open
akitbalian wants to merge 1 commit into
KalicoCrew:mainfrom
akitbalian:local-fixes
Open

Fix to allow canBUS MCUs to connect and disconnect mid print#905
akitbalian wants to merge 1 commit into
KalicoCrew:mainfrom
akitbalian:local-fixes

Conversation

@akitbalian

@akitbalian akitbalian commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Modified a number of files that were causing issues when an MCU is connected/disconnected mid print or between prints.

This allows for example disconnecting an MCU on the carriage for toolchanged. In my specific case this was with Cartographer but should apply to any MCU.

Main change description: (also in the commit)

trdispatch: unlink trdispatch_mcu before free

trdispatch_mcu_alloc() links the object into td->tdm_list, but mcu.py
released it with ffi_lib.free() and nothing ever unlinked it. When
_build_config() re-runs after a non-critical MCU reconnect, the old
object is freed while still on the list; trdispatch_start() and
handle_trsync_state() then walk into freed memory on the next homing
move.

Add trdispatch_mcu_free(), which unlinks under td->lock and deregisters
the fastreader if trdispatch_start() had registered it, and call it
explicitly from _build_config(). Add trdispatch_free() as the parent's
destructor so any remaining children are released at teardown: a child
destructor cannot rely on its parent surviving, because CPython's
cyclic collector finalizes cycle members in arbitrary order.

Confirmed with ASAN (heap-use-after-free, 8-byte read at
trdispatch.c:141). Two core dumps corroborate: SIGSEGV in
trdispatch_start at the same line, and SIGABRT "corrupted double-linked
list" detected downstream in serialqueue_alloc.

Also modified the behavior on reconnect timer in serialhdl.py
and canbus_stats.py. This was causing time out errors because it was configured to wait for 1 second for a reconnect (changed to 0.1s)

Checklist

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

@rogerlz

rogerlz commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR. I reviewed the diff and reproduced the shutdown path on a real printer.

trdispatch/chelper changes: solid and this part is mergeable as-is.

The _handle_shutdown change has two problems, both reproduced:

  1. After the shutdown is rerouted to handle_non_critical_disconnect(), the reconnect timer fires immediately. _mcu_identify() succeeds and _send_get_config() then raises inside the reactor timer callback, which kills klippy:
Non-critical MCU 'sht36' shutdown: Command request - handling as disconnect
mcu: 'sht36' disconnected!
Unhandled exception during run
...
klippy.mcu.error: Can not update MCU 'sht36' config as it is shutdown

Same defect class as #870, fixed by #918: recon_mcu() now contains reconnect failures, and an MCU that reports a shutdown state gets a reset so the next attempt starts clean.

  1. self._is_shutdown = False disarms the dedup guard at the top of _handle_shutdown and a shutdown mcu answers every non-allowed command with is_shutdown, so command traffic re-enters the handler (we saw duplicate "handling as disconnect" entries within seconds, no traffic generation needed).

suggestion: rebase on main once #918 lands and drop the self._is_shutdown = False line. The recon_mcu() resets that flag after a successful identify, which keeps the guard armed while the MCU is down.
I tested exactly that combination (this PR + #918, minus that line) on hardware:

Non-critical MCU 'sht36' shutdown: Command request - handling as disconnect
mcu: 'sht36' disconnected!
MCU 'sht36' is in a shutdown state - attempting reset
mcu: 'sht36' reconnected!

One related heads-up: while testing CAN bus loss, I also reproduced the canbus_stats crash from #900 (the periodic query can be in flight when the connection drops, so the existing non_critical_disconnected check can't catch it) and #918 guards that path too. Without it any CAN disconnect can still take klippy down before the reconnect machinery gets a chance.

@rogerlz

rogerlz commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

To make this easier to land, I cherry-picked the trdispatch commit (c994cf6) to main via #919. Once it merges, rebase this PR on main, and it should shrink to the _handle_shutdown commit; see the suggestions above for that part.

@akitbalian

Copy link
Copy Markdown
Contributor Author

Hi!

So pardon if I make any mistakes (first ever PR for me) - I did run the rebase as #919 looks like it has been committed.

I do see that it has shrunk to the _handle_shutdown. I won't be able to test the code with out the self._is_shutdown = False until a few days out.

I removed that part as per your suggestion, and I think it all looks good now. But if I made any mistakes please let me know!

Note the rebase also dropped my serialhdl.py / canbus_stats.py changes — I'll take another look at whether anything there is still needed once #918 is in, and open a separate PR if so.

Thanks for taking care of the other stuff!

-AK

@akitbalian

Copy link
Copy Markdown
Contributor Author

Also — I just noticed my PR description mentions serialhdl.py / canbus_stats.py changes that aren't actually in the diff; those are still uncommitted on my machine and never made it in. So your test was of the mcu.py change alone. I'll check whether the reconnect timer change is still needed once I'm on current main with #918, and open a separate PR if it is. Updating the description now.

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.

2 participants