[TGoodhew]

Fixes screen capture on the HP 54845A Infiniium, and stops a failed binary read from blocking the server.

The capture

• Wrong command: the profile sent :WAVeform:DATA?, which returns the waveform as ASCII. It now sends :DISPlay:DATA? GIF,SCReen (Programmer's Quick Reference Guide 54810-97065, p. 2-20).
• Seeded copies: the instrument database is copied to the user's folder on first run, so the bundled file lists the old blocks under supersedes. Otherwise existing installs would keep the wrong command.
• Headers on: with :SYSTem:HEADer ON the scope sends :DISP:DATA before the block. The block reader now skips one such word, so a capture neither depends on that setting nor changes it. This also covers visa_query_binary and other models. Text that only looks like a name is still reported as not being a block.
• Error queue: ErrorQueue.IsNoError treated :SYST:ERR 0,"No error" as an error, for the same reason.

Failed binary reads

A rejected query held the bus for 135 s on a 60 s timeout, with every later call queued behind it. NI-488 timeouts snap up to a step and overrun: over a GPIB-ENET/1000, 3000 ms waits 4.2 s and 5000 ms waits 16.8 s.

• The first read waits a short time for the first byte (firstReplyTimeoutMs, 3000 for the 54845A). It's one read, limited to a timeout step whose measured wait fits in half the time for reading, so a driver's step can't carry it past the limit. It must not be split into short reads: that was tried after review, and on the 54845A every capture failed and the scope stopped answering, serial poll and device clear included, until it was power-cycled.
• first_reply_timeout_ms asked for by name isn't capped by timeout_ms.
• After that, reads are 1 s slices, and only silence counts as a stall.
• The whole call has a budget: 25 s by default and 55 s at most, matching gpib_batch.
• On failure the server drains the reply, sends a device clear if the model allows it, and reports the error queue. Device clear is opt-in per model (deviceClearSafe) because it presets the HP 8560 series.
• Recovery stops reading the error queue when the call's time is up, and says so. The drain treats a read that timed out with data as a slow transfer, so a reply that stops part way still gets the device clear, and keeps only the first bytes of what it discards. A reply that started and went silent without END isn't counted as finished, and an instrument that may still be sending isn't asked for its errors.
• Not in this PR: the silence allowance after the first byte is counted in whole 1 s slices, so timeout_ms = 1500 stalls after 1 s. Fixing it changes mid-transfer timing, so it waits for a hardware test.

ErrorQueue moved from Tools to Instruments so InstrumentManager can use it.

Tested on a real 54845A (A.04.50, GPIB-ENET/1000, HP 37204A extender)

• Headers off: 11 of 11 captures at about 2 s, valid GIFs.
• Headers on, set for the test and put back after: the raw reply starts :DISP:DATA , and instrument_capture_screen, visa_query_binary and instrument_identify all work.
• A rejected command fails in about 4.25 s and names the error (-224), and the next call takes 12 ms.
• After the review fixes (power-cycled first): three captures at about 2 s, a rejected dump in 4.3 s with -108 and -420 read back, then *IDN? in 12 ms, headers still off and the error queue empty. The same again after the second round of fixes.
• :SYSTem:SETup? is byte-identical before and after a device clear.
• With headers on, the error queue's reply was read directly but not through the failure path, which is covered by unit tests. Other tools that parse values, such as gpib_batch, weren't tried with headers on.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Blocking reads and recovery can exceed the call budget, and partial-read timeouts can skip device clear while unread reply data remains, contaminating subsequent queries.

5 open issues

• First-byte read can block past the call budget · New
• Recovery diagnostics can exceed the remaining call budget · New
• Timed-out partial reads can skip required device clear · New
• Reply timeout incorrectly caps the first-reply allowance · New
• Drain buffers the entire discarded reply · New

What changed in this PR

This PR corrects HP 54845A screen capture and adds timeout and recovery controls to the shared binary-read path to reduce server blocking after failed queries.

Changes:

• Corrects the 54845A GIF capture command and migrates previously seeded profiles.
• Handles header-prefixed replies and adds binary-read budgets, model-aware recovery, and error reporting.
• Adds regression tests, documents the new controls, and bumps the version to 1.3.1.

File	Description
tests/​GpibMcp.Tests/​InstrumentManagerTests.cs	Tests binary-read timeouts, slow transfers, and recovery.
tests/​GpibMcp.Tests/​FakeInstrumentManager.cs	Records binary-read options for assertions.
tests/​GpibMcp.Tests/​DatabaseToolsTests.cs	Tests header-prefixed error-queue replies.
tests/​GpibMcp.Tests/​BinaryToolsTests.cs	Tests option propagation, corrected profiles, and seeded-copy migration.
tests/​GpibMcp.Tests/​BinaryBlockTests.cs	Tests reply-name prefixes and malformed input.
src/​GpibMcp.Core/​Tools/​InstrumentTools.cs	Supplies model context to binary-tool registration.
src/​GpibMcp.Core/​Tools/​CaptureTools.cs	Passes capture limits and recovery options to binary reads.
src/​GpibMcp.Core/​Tools/​BinaryTools.cs	Exposes timing controls and resolves model-specific options.
src/​GpibMcp.Core/​Instruments/​InstrumentManager.cs	Adds sliced reads, budget checks, and failure recovery.
src/​GpibMcp.Core/​Instruments/​InstrumentDefinition.cs	Adds first-reply timing and device-clear safety settings.
src/​GpibMcp.Core/​Instruments/​IInstrumentManager.cs	Extends binary queries with optional read settings.
src/​GpibMcp.Core/​Instruments/​ErrorQueue.cs	Moves shared error handling and recognizes reply prefixes.
src/​GpibMcp.Core/​Instruments/​BinaryBlock.cs	Defines read options and parses header-prefixed blocks.
src/​Directory.Build.props	Bumps the shared version to 1.3.1.
README.md	Documents timing controls and failure recovery.
data/​instruments/​README.md	Updates the 54845A command count.
data/​instruments/​54845A.json	Corrects capture settings, adds migration metadata, and documents commands.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved critical timeout handling can exceed whole-call budgets and hold _gate during recovery, while additional findings identify incomplete-reply handling and reads issued beyond the remaining timeout allowance.

3 open issues

• Reserve recovery time using effective timeout steps · New
• Bound recovery reads by the remaining deadline · New
• Recovery diagnostics can exceed the remaining call budget

2 previously missed issues

In code that hasn't changed since last review

Honor the exact inactivity timeout for each read slice

src/​GpibMcp.Core/​Instruments/​InstrumentManager.cs:134

The stall timeout is rounded down to whole seconds here, while every later read still requests 1000 ms. Consequently, timeout_ms=1500 fails after only one silent second, and timeout_ms=500 requests twice the specified wait. Track the remaining inactivity allowance and limit each slice to it, rather than counting a floored number of fixed-size slices.

Require END before treating a started reply as complete

src/​GpibMcp.Core/​Instruments/​InstrumentManager.cs:271

An empty drain does not prove that the reply ended: the parser may already have consumed an invalid byte such as - without END. If the next read times out empty, this returns true and skips an allowed device clear, leaving the transfer unfinished. Carry the original read's EndOfMessage state into recovery and require END before declaring a started reply finished.

4 resolved since last review

• Timed-out partial reads can skip required device clear
• First-byte read can block past the call budget
• Drain buffers the entire discarded reply
• Reply timeout incorrectly caps the first-reply allowance

_{Review effort: Balanced}