fix(tr): surface firmware NAK text on launch/upload failures#44
Merged
Conversation
…ilures
When the TeensyROM NAKs a command, _expect_ack drained the trailing error
text purely to resync the stream and then discarded it — so a failed
PostFile/LaunchFile surfaced only a bare "FailToken (0x9B7F)". Capture that
text and put it in the raised error:
* "Busy!" (program running / menu handler inactive) -> new TRBusyError
(subclass of TRError) so callers can tell it apart from a hard failure;
* other reasons ("Not enough room", "File already exists.", ...) appended
to the FailToken/RetryToken/unexpected-reply message.
The drain (resync) behavior is unchanged; we just stop throwing the reason
away. Unit-tested via the loopback transport (ErrorSurfacingTest).
Context: this is the salvaged, safe part of a TR-launcher investigation. The
investigation uncovered a separate, pre-existing INTERMITTENT bug — the
keyboard poller's ReadC64Mem racing the launcher's reset+PostFile on the
shared TR link can desync the stream and make the upload drop a byte (the
.prg then loads one byte short -> ?SYNTAX ERROR). It's a race (reliable
single-threaded and on the Ultimate), needs a soak harness to verify a fix,
and is tracked separately as a known issue (docs/caveats.md). This error
surfacing is what makes that failure diagnosable. No launch-path behavior
change here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
=======================================
Coverage 79.68% 79.69%
=======================================
Files 68 68
Lines 12943 12947 +4
Branches 1910 1911 +1
=======================================
+ Hits 10314 10318 +4
Misses 2190 2190
Partials 439 439 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When the TeensyROM NAKs a command,
teensyrom_dma._expect_ackdrained the firmware's trailing error text only to resync the stream, then discarded it — so a failedPostFile/LaunchFilesurfaced just a bareFailToken (0x9B7F). This captures that text and puts it in the raised error:"Busy!"(a program is running / the menu handler isn't active) → newTRBusyError(subclass ofTRError), so callers can distinguish it from a hard failure."Not enough room","File already exists.", …) → appended to theFailToken/RetryToken/unexpected-reply message.The drain (resync) behavior is unchanged — we just stop throwing the reason away. Covered by
ErrorSurfacingTestagainst the loopback transport. No launch-path behavior change.Why (context)
This is the safe, salvaged part of a TR-launcher investigation. The original goal (reset-free relaunch for looping launchers) turned out not verifiable and was backed out. The investigation also uncovered a separate, pre-existing intermittent bug: the keyboard poller's
ReadC64Memracing the launcher'sreset()+PostFile on the shared TR link can desync the stream so the upload drops a byte — the.prgthen loads one byte short and the BASIC autostart stub?SYNTAX ERRORs. It's a race (reliable single-threaded, and on the Ultimate which has no shared-link poll), so verifying a fix needs a soak harness; it's tracked as a known issue indocs/caveats.md, not fixed here. This error surfacing is what makes that failure diagnosable.Test
make check(ruff + mypy --strict + pyright + 1588 tests) green.