LS-50 (Coolscan V) direct-USB capture: driver, bridge RPC, Done Previews button

[rjerham12]

Summary

Adds direct-USB capture support for the Nikon LS-50 ED (Coolscan V) so it can be driven over raw USB (no SANE), producing the calibration workflow's artifacts.

What's included

coolscanpy (protocol/ls50):

• capture.py: session + RGBI capture. The image READ carries DTC (data type code) in byte 2 and DTQ (color<<8|element-width) in bytes 4-5 per the Nikon spec, reads in whole-granule chunks, and treats a short read / sense 05-2C (ILI, OutOfSequence) as the normal end of the image stream (fixes the LS-50 desync).
• artifacts.py: raw linear 16-bit RGB TIFF + -ir.tif sidecar + receipt (4000:16:1:rgbi).
• workflow.py: Ls50Roll — preview (per-slot streaming, end-of-strip detection), set_spacing_offset (frame adjust), scan_many.
• pass_runner.py: CLI for the calibration passes.
• hardware-free tests under tests/protocol/ls50/.

bridge:

• CoolscanPyTransport.preview streams LS-50 thumbnails as they capture (avoids the engine's 600s stream-silence deadline).
• preview_stop() + roll.previewStop RPC so the operator can finish a preview early.

app:

• 'Done Previews' button in ContentView while a preview is in progress.

Verification

Live-tested against a physical LS-50 ED with a color-negative strip in an SA-30 adapter: preview completes with roll.previewComplete and end-of-strip detection; full-res RGBI captures are correct raw negatives.

The wire grammar is adapted from nkscan (activexray/nkscan, MIT/Apache-2.0), which documents the LS-5000/LS-9000 ED protocol and lists the LS-50 as supported.

[rohanpandula]

LS-50 review and implementation handoff

Updated September 12 after an independent adversarial review with Claude and verification of the key corrections against the same reviewed snapshot. This update does not claim a fresh review of any newer commits. Claude performed static analysis; the previously reported test runs and boundary reproductions were performed separately. Corrections below narrow the library/transport claims, explain the actual smear and manifest effects, and add the missed debug log.

This is a self-contained Markdown handoff you can paste into an implementation LLM. Please verify each finding against the branch before editing, fix the root causes, and report the tests and remaining hardware uncertainties.

Reviewed this PR at 768ddcd368a340223c46f0dc285f0831d0f16857 and standalone coolscanpy #21 at a17791e50de1bb47fa116940da770365aa6841c8. Both heads were rechecked before the original September 11 comment. Review covered every added/removed line, every new LS-50 file, and affected existing call paths—not every unchanged line of both repositories. No physical scanner testing was performed.

The existing suites passing is useful, but the boundary reproductions below expose gaps. I recommend addressing these before merging. Keep fixes small, reuse the existing transaction/artifact machinery where its contract fits, and preserve LS-5000 behavior. Do not make tests green by replacing unknown measurements with plausible constants or by disabling identity checks.

Findings and fix direction

1. P2 — USB-first discovery breaks the existing LS-5000 plain scan

In #123, _device.get_devices() now returns immediately when USB enumeration succeeds, yielding IDs such as usb:1:2. The LS-5000 branch of Device.scan() still passes that ID to SANE. SANE's mandatory fresh identity check matches the raw SANE name exactly, such as coolscan3:usb:libusb:001:002, and rejects the USB ID as a disappeared scanner. Reproduced with actual discovery, Device, service and SANE scan code, replacing only enumeration/initialization. This affects the documented coolscanpy Device.scan() public API when USB discovery succeeds and capture uses SANE. ScanStudio’s bridge does not call that plain-scan API, so this is not evidence that the app’s LS-5000 roll capture is broken. Preserve or resolve the SANE identity for this path.

Discovery change, plain scan dispatch.

2. P2 — The new READ loop does not satisfy its transport contract

Both PRs reuse perform_transaction, which insists on reading the complete declared transfer. A short packet followed by zero bytes raises DesynchronizedProtocolError before the status-trigger write. Consequently the new outer loop's short-read-as-EOF branch cannot handle this case. Separately, a full payload accompanied by nonzero sense is accepted because the sense check is inside the wrong branch. Reproductions confirm both behaviors, including acceptance of 031100. The short-transfer probe proves an unreachable phase-3 short-payload branch, not that a real LS-50 ends its data phase this way. A phase-1 no-data response is a different path and can reach the existing break. Actual LS-50 EOF framing requires hardware traces. The full-payload/nonzero-sense acceptance is independently confirmed. Handle the transaction result and error contract before returning image data; do not interpret arbitrary error status as successful EOF.

READ loop.

3. P1 — Streaming thumbnails does not prevent the preview timeout

#123 adds per-frame thumbnail events to avoid the 600-second timeout, but the existing acquire_thumbnails watchdog deliberately uses a total deadline and never extends it when a thumbnail arrives. That policy predates this PR; the new streaming callback does not fulfill its stated timeout-avoidance purpose. A preview lasting longer than that deadline is quarantined despite making progress. A Rust reproduction with a 150 ms deadline and thumbnails every 60 ms delivers two thumbnails and then emits BRIDGE_STREAM_STALLED. Reset the inactivity deadline on relevant progress, or explicitly model a separate total deadline.

New streaming callback, fixed deadline.

4. P1 — Requested multisampling is silently discarded

The bridge passes samples_per_scan from the capture recipe. Ls50Roll.scan_many accepts it through unused **kwargs, then creates options with the default of one sample. A request for four samples reaches session.capture with one; reproduced. The engine also writes passes: recipe.multisample_passes (real_backend.rs:5743), so the manifest claims four for a one-sample capture. The bridge advertises (1,4) using the existing LS-5000 capability path, making this reachable through the app. Advertise only LS-50 modes supported by evidence and reject other values before motion; absent verified four-sample support, expose [1]. Do not simply write four into SET_WINDOW based on the LS-5000 implementation. This review does not establish whether LS-50 hardware supports multisampling.

Scan options construction.

5. P1 — A stop request can be cleared after it was accepted

scan_many is a generator, so its initial _stop_event.clear() runs on first iteration, not when the iterator is created. If safe_stop() occurs between those two steps, starting iteration erases the cancellation and enters capture. Reproduced in that exact ordering. This is a narrow but real bridge contract mismatch: iterator creation occurs under _stop_lock, while first iteration occurs outside it. P1 here reflects starting scanner capture after an accepted stop. Clear stale preview cancellation when establishing the new operation, before exposing it to cancellation.

Cancellation reset.

6. P1 — Repeat captures overwrite earlier outputs

In #123, Arep02 creates two captures of hardcoded slot 20, both with the same output stem. The second TIFF replaces the first; --page-frame 7 is ignored. Reproduced with distinct pixel values: only the second capture survives. P1 reflects silent data loss once capture prerequisites are satisfied; finding 9 is P2 because it fails visibly before producing output. Give each repetition a distinct output identity and honor the selected physical frame.

Repeat and output naming.

7. P1 — Receipts report measurements that were never made

The receipt supplies zero clipping, an out-of-vocabulary focus verdict ok with score 1, a transport-smear verdict none, zero receipt fingerprints, and device ID usb:0:0. These values are forwarded as telemetry, but the actual smear effect is the opposite of a clean pass: server.rs:1870 and SessionModel.swift:5339 flag any verdict other than clean, so none flags every LS-50 frame as smeared. Use the existing vocabulary and represent unmeasured values explicitly (for example indeterminate where supported); update consumers so unknown evidence is not mislabeled as measured failure or success. Compute supported clipping metrics from the decoded data and preserve actual identity/geometry. Do not fix this by replacing none with an invented clean.

The zero receipt hashes are placeholders, but this review has not established that those fields currently enforce a film-identity check. Separately, the preview fingerprint ls50-<count> cannot distinguish different rolls with the same frame count; audit the actual approval binding rather than treating zero receipt hashes as proof of an active safety-check bypass.

Receipt defaults and telemetry.

8. P1 — LS-50 eject still selects an LS-5000

The new roll has no eject method, so the bridge falls back to Device.eject. The adapter-status/medium-unload connection helper forwards bus/address but retains the worker's default expected product, LS-5000 ED. An LS-50 therefore cannot satisfy that selector. The same issue affects presence probing. The helper's omitted product argument was reproduced; physical eject was not attempted. The selector default predates this PR; adding the LS-50 roll exposes the integration gap. Carry the selected scanner identity through these helpers. Also account for the interface already held by Ls50Session: the fallback opens a second connection while the roll session remains open. A second claim is a potential additional failure, not a hardware result reproduced here. Reuse the held connection or release ownership deliberately, then verify presence/eject and post-eject status on the LS-50.

Device integration, adapter connection helper.

9. P2 — The pass CLI cannot perform its default invocation

In #123, omitting optional --preview leaves _preview unset. scan_many skips every requested frame, so the CLI raises slot 1 produced no capture; reproduced. With preview enabled, --frames is not passed to preview to bound its work. In standalone #21, the CLI still calls nonexistent roll.capture_frame, so its capture path fails even earlier. These are distinct failures in the two heads.

Current CLI, standalone CLI.

10. P2 — CLI controls and output metadata disagree with capture

The new single-frame CLI parses exposure arguments but constructs Ls50ScanOptions without them. --exp-r 2000000 captures with 1000000, while the requested exposure is passed to the artifact writer; reproduced. The pass CLI also parses DPI/depth without configuring the roll and writes TIFFs directly without the advertised JSON receipts. Its slot map uses the mapped label as the physical slot as well. Reuse the existing artifact writer and explicitly connect supported CLI arguments to capture options.

Single-frame options, pass writer.

11. P2 — Temporary scan-event logging is unbounded

real_backend.rs:7028 appends to ~/.scanstudio/engine-debug.log; scan-event call sites include full JSON payloads (real_backend.rs:7348). The PR introduces this temporary diagnostic path without rotation or a size bound. Remove the temporary instrumentation or route narrowly needed diagnostics through the established bounded/opt-in logging mechanism. Verify normal scans do not continually append full event payloads to this file. This was missed in the original review and independently checked after Claude identified it.

Logger, event logging.

Suggested implementation order and acceptance checks

1. Preserve scanner identity end to end. Fix discovery-to-SANE identity resolution and propagate the selected USB product/bus/address through presence and eject. Test with both an LS-5000 and LS-50 enumerated; prove the selected scanner is used. An unsupported identity must still refuse before motion. Do not globally weaken the existing LS-5000 identity gate.
2. Settle the READ contract. Inspect the actual perform_transaction implementation and DesynchronizedProtocolError recovery requirements. Cover a full successful payload, nonzero sense with a full payload, partial transfer, zero transfer, and transport timeout. A short USB packet alone is not proof of protocol-level EOF. Use captured protocol evidence to distinguish legitimate EOF from failure; do not fabricate scanner behavior from these doubles. Preserve shared LS-5000 transaction tests.
3. Fix operation lifecycle. An active preview producing valid thumbnails must survive beyond its original inactivity deadline; a genuinely stalled stream must still fail. Stop-before-first-iteration must prevent capture; Done Previews followed by a newly started scan must work; stopping between frames must stop further capture. Avoid sleeping to make races disappear—control event ordering in tests.
4. Honor the recipe. Request four samples and inspect the options delivered to capture. Either deliver four or reject the request before motion if the hardware/driver cannot support it. Apply the same rule to exposure, DPI, and depth. Verify written metadata describes the options actually executed.
5. Repair CLI semantics and artifact ownership. The documented invocation must either establish required preview geometry or fail before hardware access with an explicit prerequisite. Bound any automatic preview to the requested slots. Preserve physical slot separately from frame label ({1:20} means scan physical slot 1 and label it frame 20). Define whether Arep02 is a repetition label or count, honor --page-frame/--page-frames, and ensure multiple intended captures never silently replace one another. Use the existing artifact writer for RGB, IR and receipts; keep the bridge's output ownership intact.
6. Make evidence truthful. Preserve real device identity and actual spacing/exposure. Compute supported measurements or explicitly represent unavailable measurements through the Python/Rust contract and consumers. Audit the actual preview approval binding separately from receipt hash placeholders. Test both serialization and how the UI handles indeterminate; do not mark unmeasured focus/smear/clipping as passed or automatically label unknown smear as a measured smear. Test serialization and downstream eligibility handling for unavailable evidence.
7. Remove temporary logging and synchronize the two implementations deliberately. Remove the unbounded full-event debug log. ScanStudio: ScanStudio could not complete that action (INTERNAL) - Doesn't recognize CoolScan 5000 #21 is an older implementation, not equivalent to this head. Port verified shared fixes with their tests; keep Swift/Rust/bridge integration changes in ScanStudio. Do not blindly copy this head into ScanStudio: ScanStudio could not complete that action (INTERNAL) - Doesn't recognize CoolScan 5000 #21 because this head still has the findings above. Reconcile merge conflicts against current base and rerun relevant checks on that result.

Checks already run on the reviewed ScanStudio snapshot

• cd app/ScanStudio && swift build: passed.
• From coolscanpy, with its src on PYTHONPATH: python -m pytest tests/test_facade.py tests/transport tests/session tests/protocol/ls50: 513 passed, 1 xfailed.
• From bridge, with both Python source roots on PYTHONPATH: python -m pytest: 414 passed.
• From app/ScanStudio/engine: cargo test --locked --test end_to_end_sim: 5 passed; real-backend mapping suite: 40 passed.
• These are software checks, not a hardware qualification. After fixes, report what was run on actual LS-50 hardware separately, including preview → Done Previews → scan → stop → eject, and a sufficiently long preview to exercise the inactivity behavior.

Runnable Python defect reproductions

Save the following as review_reproduce.py outside the source tree, install the repository's normal development dependencies, and run from the ScanStudio repository root:

PYTHONPATH="$PWD/coolscanpy/src:$PWD/bridge/src" python review_reproduce.py

These assertions confirm current defects. They are diagnostic probes, not desired-behavior regression tests. After fixing a defect, invert/replace its assertion with the intended behavior. The endpoint doubles describe input cases; they do not establish which byte sequence a real scanner emits at EOF. The repeat-output test bypasses the separately broken CLI preview prerequisite to isolate overwrite behavior. The single-frame probe records y_min but the reported defect there is the ignored exposure, not an assumption that --frame must seek rather than label.

Python reproductions (copy the complete block)

"""Hardware-free reproductions for ScanStudio PR 123 at 768ddcd.

Run with the PR's coolscanpy/src and bridge/src on PYTHONPATH.
Assertions confirm the reported defects, rather than claiming correct behavior.
"""
import contextlib
import io
import json
import sys
import tempfile
from pathlib import Path
from types import SimpleNamespace
from unittest.mock import patch

import numpy as np
import tifffile
from coolscanpy.protocol.ls50 import capture, pass_runner
from coolscanpy.protocol.ls50 import __main__ as one_frame
from coolscanpy.protocol.ls50.workflow import Ls50Roll
from coolscanpy.protocol.ls5000_single_pass.worker import DesynchronizedProtocolError


class FakeSession:
    def __enter__(self): return self
    def __exit__(self, *args): pass
    def open(self): pass
    def close(self): pass
    def capture(self, opt, **kwargs): return bytes([80]) * opt.stream_bytes


def run():
    findings = {}
    class EndpointOut:
        def __init__(self): self.writes = []
        def write(self, data, **kwargs):
            self.writes.append(data)
            return len(data)

    class EndpointIn:
        def __init__(self, responses): self.responses = iter(responses)
        def read(self, size, **kwargs): return next(self.responses)

    opt = capture.Ls50ScanOptions(dpi=4000, x_max=1, y_max=0)
    session = capture.Ls50Session()
    session._ep_out = EndpointOut()
    session._ep_in = EndpointIn([b'\x03', bytes(opt.stream_bytes - 10), b''])
    try:
        session._read_lines(opt)
    except DesynchronizedProtocolError as exc:
        assert b'\x06' not in session._ep_out.writes
        findings['short_read'] = str(exc)
    else:
        raise AssertionError('short-read reproduction changed')

    session._ep_in = EndpointIn([b'\x03', bytes(opt.stream_bytes), bytes.fromhex('0203110000000000')])
    assert len(session._read_lines(opt)) == opt.stream_bytes
    findings['read_error_ignored'] = 'sense 031100 accepted with a full payload'

    with tempfile.TemporaryDirectory() as directory:
        argv = ['pass_runner', '--stock', 'test', '--pass', 'A1', '--out', directory, '--frames', '1']
        with patch.object(pass_runner, 'Ls50Session', FakeSession), patch.object(sys, 'argv', argv):
            try:
                pass_runner.main()
            except RuntimeError as exc:
                assert str(exc) == 'slot 1 produced no capture'
                findings['cli_without_preview'] = str(exc)
            else:
                raise AssertionError('CLI reproduction changed')

    roll = Ls50Roll(session=FakeSession())
    roll.preview(slots=[1])
    roll.preview_stop()
    class InspectCapture(Exception): pass
    def inspect_capture(opt, **kwargs):
        findings['actual_samples_per_scan'] = opt.samples_per_scan
        raise InspectCapture
    roll.session.capture = inspect_capture
    try: next(roll.scan_many([1], samples_per_scan=4))
    except InspectCapture: pass
    assert findings['actual_samples_per_scan'] == 1
    iterator = roll.scan_many([1])
    roll.safe_stop()
    try: next(iterator)
    except InspectCapture: findings['cancel_before_iteration'] = 'capture still entered'
    assert findings['cancel_before_iteration'] == 'capture still entered'

    class StopBeforeUSB(FakeSession):
        def capture(self, opt, **kwargs):
            findings['single_cli_actual'] = {'y_min': opt.y_min, 'exposure_r': opt.exposure_r_raw_10ns}
            raise InspectCapture
    with tempfile.TemporaryDirectory() as directory:
        argv = ['ls50', '--out', directory, '--stem', 'frame5', '--frame', '5', '--exp-r', '2000000']
        with patch.object(one_frame, 'Ls50Session', StopBeforeUSB), patch.object(sys, 'argv', argv):
            try: one_frame.main()
            except InspectCapture: pass
    assert findings['single_cli_actual'] == {'y_min': 0, 'exposure_r': 1000000}

    class FakeRoll:
        calls = []
        def __init__(self, **kwargs): pass
        def close(self): pass
        def scan_many(self, slots, **kwargs):
            self.calls.append(slots)
            yield SimpleNamespace(slot=slots[0], rgb=np.full((2, 2, 3), len(self.calls), np.uint16), ir=np.ones((2, 2), np.uint16))

    with tempfile.TemporaryDirectory() as directory:
        argv = ['pass_runner', '--stock', 'test', '--pass', 'Arep02', '--page-frame', '7', '--out', directory]
        with patch.object(pass_runner, 'Ls50Session', FakeSession), patch.object(pass_runner, 'Ls50Roll', FakeRoll), patch.object(sys, 'argv', argv):
            pass_runner.main()
        files = sorted(p.name for p in (Path(directory) / 'test' / 'coolscan').iterdir())
        assert FakeRoll.calls == [[20], [20]]
        assert files == ['pass-summary.txt', 'test_20_Arep02-ir.tif', 'test_20_Arep02.tif']
        assert tifffile.imread(Path(directory) / 'test/coolscan/test_20_Arep02.tif')[0, 0, 0] == 2
        findings['repeat_outputs'] = {'slots': FakeRoll.calls, 'files': files, 'last_capture_overwrote_first': True}

    from coolscanpy.transport import adapter_status
    calls = []
    with patch('coolscanpy.protocol.ls5000_single_pass.worker._connect_device', side_effect=lambda **kwargs: calls.append(kwargs)):
        adapter_status._connect_device(device_id='usb:1:2')
    assert 'expected_scanner_product' not in calls[0]
    findings['presence_and_eject_selector'] = 'uses default LS-5000 ED rather than selected LS-50 ED'
    return findings


if __name__ == '__main__':
    with contextlib.redirect_stdout(io.StringIO()):
        result = run()
    print(json.dumps(result, indent=2))

Existing LS-5000 plain-scan regression reproduction

Run with the same environment. This uses actual discovery, facade, service and SANE scan code, replacing enumeration and initialization only. Expected current error: Scanner device 'usb:1:2' disappeared from fresh SANE enumeration.

from types import SimpleNamespace
from unittest.mock import patch
from coolscanpy import _device
from coolscanpy.types import DeviceInfo, Capabilities
from coolscanpy.session.service import ScannerService
from coolscanpy.transport.sane import SaneBackend

info = DeviceInfo('usb:1:2', 'Nikon', 'LS-5000 ED',
    Capabilities(True, (4000,), (16,), True, 40, True, True, True, True))
backend = SaneBackend.__new__(SaneBackend)
backend._sane = SimpleNamespace(get_devices=lambda: [
    ('coolscan3:usb:libusb:001:002', 'Nikon', 'LS-5000 ED', 'scanner')])
service = ScannerService()
service._backend = backend
with patch.object(_device, '_usb_fallback_device_infos', return_value=[info]), \
     patch.object(backend, '_ensure_initialized'):
    selected = _device.get_devices()[0]
    try:
        _device.Device(selected, service).scan()
    except RuntimeError as error:
        assert 'disappeared from fresh SANE' in str(error)
        print(error)
    else:
        raise AssertionError('Expected current identity mismatch')

Rust preview-deadline reproduction

In an isolated checkout, add a test-only delay inside the existing for slot in [1u32, 2, 3] loop in engine/src/bin/mock_bridge.rs, before each thumbnail:

if std::env::var_os("REVIEW_PREVIEW_TICK").is_some() {
    std::thread::sleep(std::time::Duration::from_millis(60));
}

Save the following as engine/tests/reviewer_streaming.rs, then run cargo test --locked --test reviewer_streaming from the engine directory. It passed while demonstrating the defect: two timely thumbnails, then BRIDGE_STREAM_STALLED. For a permanent test, use a generous timing margin and assert the corrected successful completion; keep a separate genuine-stall case.

use std::{sync::{Arc,mpsc},time::Duration};
use scanstudio_engine::{domain::{ScannerBackend,FilmProcess},protocol::ConnectOptions,real_backend::RealLs5000};
#[test]
fn streamed_thumbnails_do_not_extend_the_current_deadline() {
 let backend=Arc::new(RealLs5000::new_with_env(env!("CARGO_BIN_EXE_mock_bridge"),Duration::from_secs(3),&[("REVIEW_PREVIEW_TICK","1"),("MOCK_BRIDGE_PREVIEW_DELAY_MS","0")]).unwrap().with_preview_silence_deadline(Duration::from_millis(150)));
 backend.connect("bridge-ls5000-0",&ConnectOptions::default()).unwrap();
 let (tx,rx)=mpsc::channel();
 RealLs5000::acquire_thumbnails(&backend,None,FilmProcess::default(),Some("review".into()),tx).unwrap();
 let mut delivered=0; let mut stalled=false;
 loop {let value:serde_json::Value=serde_json::from_str(&rx.recv_timeout(Duration::from_secs(3)).unwrap()).unwrap();
  if value["event"]=="scanner.thumbnail" {delivered+=1;}
  if value["event"]=="scanner.thumbnailsFailed" {stalled=value["payload"]["code"]=="BRIDGE_STREAM_STALLED";}
  if value["event"]=="scanner.thumbnailsComplete" {break;}
 }
 assert_eq!(delivered,2); assert!(stalled,"expected reproduction of absolute deadline despite 60ms heartbeat");
 std::thread::sleep(Duration::from_millis(150));
}

Please finish with a finding-by-finding disposition: fixed with code/test reference, already resolved with evidence, or still blocked on a specific hardware observation. Do not treat passing the old suites alone as closure.