Fix teensytoany read failures after timeout by matthewc0llins · Pull Request #51 · ramonaoptics/python-teensytoany

matthewc0llins · 2026-10-01T15:59:05Z

Problem

_ask writes one command and reads one line back with pyserial's read_until, which gives the whole line a single deadline and checks it after every byte. If the reading thread is held up past that deadline, for example while another thread holds the GIL, the reply has already arrived, but the thread reads one byte ("0"), sees the deadline has passed and returns. The \n stays in the buffer.

From then on, every command reads the reply meant for the one before it. A write command accepts the lone "0" silently, so the first visible error is one command later: ValueError: invalid literal for int() with base 10: '\n', followed by TypeError: int() can't convert non-string with explicit base when a read command gets a bare acknowledgement. Some shifted replies parse cleanly with the wrong value, and nothing brings the stream back. The device itself replies on time throughout.

A 0.1 s pulse uses half of the 0.205 s timeout before the device replies, so a stall of about 105 ms during a pulse is enough.

Changes

_read reads one byte at a time with read(1), whose timeout restarts with every byte, instead of read_until. A reader that was held up still gets the rest of a reply that has already arrived.
_ask clears the input buffer before each command, dropping anything left by a late reply to an earlier command.
A missing, incomplete or unparseable reply flushes the input buffer, waits for the port to go quiet, checks that a version round trip succeeds, and raises the new TeensyToAnyReplyError. This covers a reply the device sends after the timeout, which the per-byte timeout can't.
TeensyToAnyReplyError subclasses RuntimeError, so existing except RuntimeError handlers still catch it. A reply carrying an error code from the device is raised as before, without resynchronizing, since the replies are still in step.
If the replies can't be brought back in step, the error says to power cycle the device.

Testing

pytest, pylint, isort and flake8 pass.
On Vireo systems running firmware 0.9.0, 0.20.0 and 0.21.0, under a stress test that holds the GIL for 140–190 ms at a time while sending a mix of pulses, PWM writes, GPIO reads and I2C reads: without the fix, 44–60% of replies were wrong; with the fix to the read, none were, including more than 700 replies per run that took longer than the timeout.
Against a simulated device on a pseudo-terminal, through pyserial, with stalls of up to 250 ms: no wrong replies.
With the simulated device sending every 40th reply 0.3 s, 0.5 s or 1.0 s late: every other command got its own reply, and each late one raised TeensyToAnyReplyError. Without the version check, a reply more than about 0.4 s late could still be read by the next command.

Cost

A normal reply costs one extra buffer clear per command; the per-byte reads are what read_until already did internally.
A resync takes about 0.5 s and only follows a bad reply.
A device that stays connected but stops answering now takes a few seconds per failed command to raise, rather than 0.2 s.

🤖 Generated with Claude Code

hmaarrfk · 2026-10-01T18:12:57Z

i really doubt the usefulness of any changes to this file.

all tests added in this file have been removed

hmaarrfk · 2026-10-01T18:13:26Z

The link to the upstream source that is relevant is
https://github.com/pyserial/pyserial/blob/v3.5/serial/serialutil.py#L654-L674

showing in fact, that reading byte by byte can be broken mid message.
https://github.com/pyserial/pyserial/blob/v3.5/serial/serialutil.py#L654-L674

hmaarrfk · 2026-10-01T18:37:19Z

As mentioned IRL, i would be pretty interested to see if this 200 line change could be a 20 line fix by redefining read_until (within teensytoany only) to match the guarantees we want.

For example: Every character received might reset the timeout.

matthewc0llins · 2026-10-01T19:53:04Z

I have implemented the per byte timeout and verified the fix still works on real hardware.

I believe TeensyToAnyReplyError is helpful to keep since it is a small addition that allows the code to differentiate between a communication error and other errors.

The resynchronization is also important since it is a failsafe that allows communication to recover in the unlikely event something does cause a teensy to stall past the timeout.

hmaarrfk · 2026-10-01T20:43:31Z

+            except ValueError:
+                pass
+        if error is None:
+            # The reply may still be on its way, or belong to another command.


i just don't see why it might belong to another command. in which case would this happen?

I am updating the comment to clarify what happens there. This will happen in the case that the teensy takes longer than the timeout window to reply.

My question is: how can that happen our firmware should NOT be that slow, and it should take it us to complete its tasks, and reply quickly, it should NOT be taking a long unpredictable time to respond.

hmaarrfk · 2026-10-01T20:44:42Z

+        if returned.endswith('\n'):
+            try:
+                error = int(returned_list[0])
+            except ValueError:
+                pass
+        if error is None:


Suggested change

if returned.endswith('\n'):

try:

error = int(returned_list[0])

except ValueError:

pass

if error is None:

if returned.endswith('\n'):

error = int(returned_list[0])

else:

I'm just not convinced any of these "else" paths are actually triggered.

It never happened when testing real teensys, but it can be recreated artificially. It would be rare, but it could happen where the teensy could get held up by an I2C or SPI transaction that times out (200ms). This only gives 5ms for the teensy to respond to the serial command. The branch where the resync fails would be even rarer, requiring the teensy to still be connected but stop responding.

a hung bus on an i2c transaction is a real concern thx for flagging that.

Can we be defensive about this in firmware? As in, add a i2c timeout in firmware of like 5-20 ms?

I would rather "fix" this concern in the firmware itself (which we control) rather than hack around this at this level.

hmaarrfk · 2026-10-01T20:47:24Z

+            # The reply may still be on its way, or belong to another command.
+            recovery = ("" if self._resynchronize() else
+                        "; replies are out of step, power cycle the device")
+            raise TeensyToAnyReplyError(
+                f"Failed to read a response for command: {data} "
+                f"(got {returned!r}){recovery}")


im ok with this, since we will be able to monitor that this actually gets triggered. I don't believe we will ever see the resynchronization get triggered. were you able to trigger it?

"safety" that doesn't get exercised, is more problematic since it stops future creativity for "fear" of breaking a usecase that doesn't exist.

The resync was never triggered in testing real hardware, but in the artificial case where the reply takes longer than 205ms the resync is needed. Its rare, but it could happen. My thought process is I would rather have failsafes in place to account for things that might happen that are outside of our control. Maybe a flaky cable/connection or unstable hardware could cause a brief delay. Only 1 late reply is needed to desync everything and require a restart. Over the course of a several hour timelapse, or in use of many different systems over the course of months, I wouldn't be surprised if it happens at least once.

excessive failsafes are really problematic since it triggers one of two behaviors:

Increased in bloat, and slower code. Nobody can "trigger" a failsafe to happen, and thus nobody understands how they can redesign it.

Completely ignoring them when they want to redesign the system in the future.

Both are quite dangerous, and I want to make sure people take your changes seriously.

I am ok with the addition of TeensyToAnyReplyError since it allows people to observe the issue.

However, I would like you to add a call to warning.warn for the other ValueError so that it can be observed, at least in the terminal.

I would also like to add a note that we would revisit its need in 6 months from today in April 2027 to assess its usefulness.

One way you can test your desync, in actual hardware is

# setup GPIO 13 (the LED on the teensy) as output, set it off teensy.gpio_pin_mode(13, 'OUTPUT') teensy.gpio_digital_write(13, 0) pytest.assert_raises(TeensyToAnyReplyError): teensy.gpio_digital_pulse(13, 1, duration=400E-3) teensy.gpio_digital_write(13, 1) sleep(3) teensy.gpio_digital_write(13, 0) teesny.gpio_pin_mode(13, 'INPUT')

which should create a 400 ms pulse, triggering a timeout, and causing the whole command chain to be desync'ed.

I think this is a useful test since it relies on real behavior from the protocol.

Today, I don't think that our code uses try/except on every teensy command, so the error, which used to be ValueError ... int.... will just become TeensyToAnyReplyError which has the same effect (in my mind) of generally crashing the sotware of generally crashing the sotware , but it is more understandable what it does.

Beyond this example that proves that today's API, when "correctly used" can trigger this error (i think it an), i think it would be good to dive deeper in your claim that I2C could trigger that timeout, even if by "inspection" of the underlying source code of the teensy firmware. Though this investation should not block these changes from getting merged.

Quick update: I am currently testing this and I am reliably able to trigger the desync with the 400ms pulse. I am also testing what happens after an I2C timeout. A full I2C timeout hasn't caused a desync just yet, but the full delay has hit 205ms several times. 1 more ms of delay would trigger the serial timeout and cause a desync.

hmaarrfk · 2026-10-01T20:52:14Z


    def _ask(self, data, *, size=1024, decode=True) -> str:
+        # Drop anything a late reply to an earlier command left behind.
+        self._serial.reset_input_buffer()


i just don't think that this code gets triggered.

It seems this is mostly redundant if the resync is left in. There is a niche case were this would be needed, if a reply shows up after a resync has cleared the buffer.

I'm asking people to think of "not harmfull" and approve changes that follow that essence.

Unfortunately, I feel like this call is harmful since it takes an action that cannot be observed. There is no way to know how many bytes have been cleared.

If there is an error in our protocol, it is important that it be loud (as it was today) to ensure that we can observe it.

The fact that a reply can show up after a resync is a problem in and of itself.

Basically, our protocol is as follows:

The user sends a message

The reply is expected to happen quickly. In at most 10s of ms.

The host side waits for the appropriate amount of time

If that is broken, we should adjust the amount of time we wait for it.

We do in general "adjust this time" when we do "long pulses" (and perhaps there is a better way to initiate a pulse)

but the protocol, and teensytoany in general, does not support multiple in-flight messages at the same time.

One transmit, one receive.

If there is a flaw, we don't really know if the message was received, or if it was received correctly. THe only "safe" assumption for any downstream user would be to stop, and to restart the system, which should presumably go through a safer procedure where we do flush buffers on startup (since we cannot assume that other applications flushed their commands)
https://github.com/ramonaoptics/python-teensytoany/blob/main/teensytoany/teensytoany.py#L420

So ultimately, because it takes an action that cannot be observed, i think we should remove it, unless we can prove that it is necessary to take on.

hmaarrfk · 2026-10-01T23:44:36Z

+        # Unlike pyserial's read_until, which gives the whole line one deadline,
+        # the timeout restarts with every byte: a reader held up past the
+        # deadline still gets the rest of a reply that has already arrived.
+        data = bytearray()
+        while len(data) < size:
+            byte = self._serial.read(1)
+            data += byte
+            if not byte or byte == LF:
+                break
+        data = bytes(data)


Lets define this a as a function at the top of the file with the signature

def read_until(serial, LF, size):

so that it reads just as the old code did.

matthewc0llins · 2026-10-02T17:23:00Z

Added the changes we discussed and tested on real hardware.

fix teensytoany read failures after timeout

7c42906

matthewc0llins self-assigned this Oct 1, 2026

matthewc0llins requested review from hmaarrfk and vetonsaliu8 October 1, 2026 16:54

hmaarrfk reviewed Oct 1, 2026

View reviewed changes

matthewc0llins added 2 commits October 1, 2026 14:44

remove reply desync tests

f3c0b2a

add per byte timeout

b959e79

hmaarrfk reviewed Oct 1, 2026

View reviewed changes

clarify comments

bb941fe

hmaarrfk reviewed Oct 1, 2026

View reviewed changes

Add read_until helper, warn on unparseable replies, release 0.15.0

977e14c

hmaarrfk merged commit e8afde2 into main Oct 2, 2026
3 checks passed

Conversation

matthewc0llins commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Testing

Cost

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Oct 1, 2026

Uh oh!

hmaarrfk commented Oct 1, 2026

Uh oh!

matthewc0llins commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewc0llins Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewc0llins commented Oct 2, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matthewc0llins commented Oct 1, 2026 •

edited

Loading

matthewc0llins Oct 1, 2026 •

edited

Loading