Skip to content

docs(designs): add First-Print pipeline design#57

Merged
strausmann merged 8 commits into
mainfrom
feat/first-print-design
May 15, 2026
Merged

docs(designs): add First-Print pipeline design#57
strausmann merged 8 commits into
mainfrom
feat/first-print-design

Conversation

@strausmann
Copy link
Copy Markdown
Owner

Summary

Adds the design document for the First-Print pipeline — the first end-to-end print path targeting the Brother PT-P750W.

This is a design-only PR, no code changes. Opening as a Draft to gather AI-assistant review (Copilot, Gemini Code Assist) on the design itself before any implementation work begins.

What this introduces

  • PrinterBackend Protocol as the extension point for hardware adapters (siblings to ADR 0004's PrinterModel plugin pattern)
  • PTouchBackend as the first concrete adapter, wrapping the ptouch library
  • PTP750WDriver (PrinterModel) + PTP750WPrinter (bridge to PrintQueue's _PrinterLike)
  • PrintService orchestrator (lookup → render → enqueue)
  • POST /print (async, returns job_id) + GET /jobs/{job_id} REST surface
  • Pre-print validation chain with a tight PrinterError hierarchy and HTTP status mapping
  • Testing strategy: pure unit + mock-backend integration + manual hardware smoke against a real PT-P750W
  • Explicit Acceptance Criteria (11 items)

Open questions called out in the design

  • ptouch status-block field surface — verify loaded_tape_mm availability in implementation Phase 1, otherwise fall back to raw parsing per Brother Raster Command Reference
  • Mock backend location: keep under tests/ (pure test double) or move to app/printer_backends/mock_backend.py to enable local dev without hardware
  • 5-minute in-memory job TTL is fine for First-Print; revisit alongside persistence work in Phase 5

Review focus areas

I'd appreciate feedback especially on:

  1. Protocol shape — is the print_image + send_bytes + query_status split right, or should send_bytes wait until a concrete need (template editor) materialises? (YAGNI vs future-proofing)
  2. Exception → HTTP mapping — does the 409 / 503 / 500 split make sense for the various hardware/state errors?
  3. Backend instantiation in lifespan — local import of the mock for printer_backend=mock keeps test deps out of production, but feels slightly awkward. Better idiom?
  4. Test-pyramid balance — sufficient coverage from pure unit + mock-integration, or should we add anything?

Next steps after this PR

  1. Address review comments (Copilot, Gemini, maintainer)
  2. Mark ready for review → merge as docs-only
  3. Open feat/first-print implementation branch following the design here, with its own writing-plans implementation plan

Refs #22

Design for the first end-to-end print pipeline targeting the
Brother PT-P750W. Introduces the PrinterBackend Protocol, the
PTouchBackend adapter wrapping the ptouch library, the PTP750W
driver/bridge combo, a PrintService orchestrator, and the
POST /print + GET /jobs/{id} REST surface.

Includes error hierarchy, HTTP status mapping, retry policy,
testing strategy (unit + integration via mock backend +
manual hardware smoke), and explicit acceptance criteria.

Marked as Draft pending review.

Refs #22
@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a comprehensive design document for the First-Print pipeline, aimed at enabling end-to-end printing functionality for the Brother PT-P750W. It defines the necessary architectural components, data flow, and error handling strategies required to integrate hardware adapters into the existing system, while also outlining clear acceptance criteria and testing methodologies to guide future implementation.

Highlights

  • Architecture Design: Introduces the First-Print pipeline design, establishing a PrinterBackend protocol for hardware abstraction and a PTouchBackend adapter for the Brother PT-P750W.
  • API and Orchestration: Defines the REST surface (POST /print, GET /jobs/{job_id}) and the PrintService orchestrator for handling template resolution, rendering, and queue management.
  • Error Handling and Testing: Establishes a robust PrinterError hierarchy with HTTP status mapping and outlines a comprehensive testing strategy including unit, integration, and manual hardware smoke tests.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a design document for the First-Print Pipeline, which establishes the architecture for printing to Brother PT-P750W devices via a REST API. The review feedback points out several necessary adjustments to align the design with the existing codebase and style guides. Key issues include the incorrect placement of model-specific code, protocol field naming mismatches, and the potential for synchronous I/O violations in asynchronous functions. Furthermore, the feedback identifies discrepancies in the usage of the TapeRegistry and PrintQueue components and advises against importing test-specific code into the application's lifespan logic.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
| `MockPrinterBackend` | `tests/_fakes/mock_backend.py` | Test double, no network I/O |
| Exceptions | `app/printer_backends/exceptions.py` | `PrinterError` hierarchy |
| `PTP750WDriver` | `app/printer_models/ptp750w.py` | PrinterModel driver, holds model-specific constants |
| `PTP750WPrinter` | `app/services/printers.py` | Bridge: PrinterModel + Backend → PrintQueue's `_PrinterLike` |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The design places PTP750WPrinter in app/services/printers.py, which violates Rule 17 of the Repository Style Guide (model-specific code must reside within app/printer_models/<series>.py). Additionally, the proposed printer_id field (line 137) should be named id to conform to the _PrinterLike protocol defined in backend/app/services/print_queue.py.

Suggested change
| `PTP750WPrinter` | `app/services/printers.py` | Bridge: PrinterModel + Backend → PrintQueue's `_PrinterLike` |
| `PTP750WPrinter` | `app/printer_models/pt.py` | Bridge: PrinterModel + Backend → PrintQueue's `_PrinterLike` |
References
  1. Model-specific code outside app/printer_models/.py is a bug. (link)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6c55a84. Moved PTP750WPrinter into app/printer_models/pt.py per ADR 0004, and renamed the attribute to id: str to match the _PrinterLike Protocol in backend/app/services/print_queue.py. Thanks for catching both.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
- All `ptouch` calls are synchronous and dispatched via `asyncio.to_thread`.
- `query_status` parses the ptouch status block into our `StatusBlock` dataclass.
- `print_image` validates against the cached status (see Error handling) and calls `printer.print(label, auto_cut=..., high_resolution=...)`.
- `send_bytes` opens a raw TCP connection to `host:9100`, writes the bytes, and closes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for send_bytes suggests opening a raw TCP connection. If this uses standard synchronous socket operations within an async def function (as defined on line 105), it would violate Rule 33 of the Repository Style Guide. Ensure asyncio.open_connection or asyncio.to_thread is used for the implementation.

References
  1. Synchronous I/O in async functions is flagged. (link)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6c55a84. The design now explicitly says send_bytes uses asyncio.open_connection to keep the call non-blocking. The ptouch library is synchronous, so its calls go through asyncio.to_thread; only the direct-TCP escape hatch needs open_connection.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
printer_id: str # "PT-P750W@<host>"

async def print_image(self, image, *, tape_mm, **options):
tape_spec = self._tape_registry.for_pt_series(tape_mm)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The call to self._tape_registry.for_pt_series(tape_mm) does not match the TapeRegistry implementation in backend/app/services/tape_registry.py, which uses lookup_pt(width_mm, media_type). Additionally, lookup_pt requires a media_type which is not provided here.

Suggested change
tape_spec = self._tape_registry.for_pt_series(tape_mm)
tape_spec = self._tape_registry.lookup_pt(tape_mm, options.get("media_type", MediaType.LAMINATED))

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6c55a84. The bridge now calls TapeRegistry.lookup_pt(tape_mm, media_type) with the real signature. media_type comes from a per-print options["media_type"] override, falling back to a constructor-injected default_media_type (typically MediaType.LAMINATED for the dominant TZe-Tape case). Pre-print status validation catches a mismatched physical tape via TapeMismatchError.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
backend = _build_backend(settings)
driver = PTP750WDriver(backend=backend)
printer = PTP750WPrinter(driver=driver, backend=backend, tape_registry=tape_registry)
queue = PrintQueue(printer=printer, max_concurrency=settings.printer_max_concurrency)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The instantiation of PrintQueue does not match the existing implementation in backend/app/services/print_queue.py. The current __init__ expects a list of printers (printers: list[_PrinterLike]) and does not accept a max_concurrency argument.

Suggested change
queue = PrintQueue(printer=printer, max_concurrency=settings.printer_max_concurrency)
queue = PrintQueue(printers=[printer])

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6c55a84. PrintQueue is now instantiated as PrintQueue(printers=[printer]) and the bogus printer_max_concurrency setting is removed. The queue gives each printer its own dedicated worker by design, so there is no concurrency knob — added an explanatory note in the Settings section.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
def _build_backend(settings: Settings) -> PrinterBackend:
if settings.printer_backend == "mock":
# Import locally so production deployments without test deps still start
from tests._fakes.mock_backend import MockPrinterBackend
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Importing from tests/ in production code (app/main.py) is a maintainability anti-pattern. As noted in the 'Open question' on line 258, the mock backend should be moved to app/printer_backends/ if it is intended to be used for local development or within the application lifespan.

Suggested change
from tests._fakes.mock_backend import MockPrinterBackend
from app.printer_backends.mock_backend import MockPrinterBackend

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6c55a84. The mock backend moves from tests/_fakes/mock_backend.py to app/printer_backends/mock_backend.py. That removes the cross-boundary import and keeps the mock available for local dev (PRINTER_HUB_PRINTER_BACKEND=mock). The earlier "open question" about its location is now resolved in the Lifespan section.

- Move PTP750WPrinter bridge to app/printer_models/pt.py per ADR 0004
- Rename `printer_id` field to `id` to match `_PrinterLike` Protocol in
  print_queue.py
- Use the real TapeRegistry.lookup_pt(width_mm, media_type) signature;
  introduce per-print `media_type` option with a configurable default
- PrintQueue is instantiated with `printers=[...]` (no max_concurrency
  argument); remove `printer_max_concurrency` from settings — the
  queue gives each printer its own dedicated worker by design
- Move MockPrinterBackend from tests/_fakes/ to app/printer_backends/
  so production lifespan never imports from tests; mock stays usable
  for local dev without hardware
- Clarify `send_bytes` uses asyncio.open_connection (non-blocking)

Refs #22
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a draft design document describing the planned “First-Print” end-to-end pipeline (REST → template/lookup → render → PrintQueue → PT-P750W) for the backend, including proposed abstractions (PrinterBackend, PrintService), REST surface, error mapping, and a test strategy/acceptance criteria.

Changes:

  • Introduces a detailed architecture proposal for a first working print path targeting the Brother PT-P750W.
  • Specifies proposed backend protocol(s), service orchestration flow, REST schemas, and exception→HTTP mapping.
  • Outlines testing layers (unit/integration/manual hardware smoke) and acceptance criteria for the phase.
Comments suppressed due to low confidence (1)

docs/designs/2026-05-15-first-print.md:225

  • PrintJobStatusResponse doesn’t line up with the existing in-memory Job model: the queue uses state values like printing/completed (not running/done), and timestamps are submitted_at/started_at/finished_at (not created_at). If the REST surface is meant to be a thin view over PrintQueue.Job, consider aligning these names/values (or explicitly documenting the translation).
class PrintJobResponse(BaseModel):
    job_id: str
    status: Literal["queued"]

class PrintJobStatusResponse(BaseModel):
    job_id: str
    status: Literal["queued", "running", "done", "failed"]
    error_code: str | None = None
    error_message: str | None = None
    error_detail: dict[str, Any] | None = None
    created_at: datetime
    started_at: datetime | None = None
    finished_at: datetime | None = None
</details>

Comment thread docs/designs/2026-05-15-first-print.md Outdated
Comment on lines +172 to +176
6. PrintService calls `PrintQueue.enqueue(image, tape_mm=template.tape_mm, options=...)` → `job_id`.
7. The API responds `202 {job_id, status: "queued"}`.
8. The queue worker dequeues (existing FSM) and calls `PTP750WPrinter.print_image(...)`.
9. The backend runs pre-print validation and prints.
10. Job status transitions: `queued → running → done` (or `→ failed` with `error_code`).
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. The data flow and lifespan example now use PrintQueue.submit(printer_id, image, tape_mm, **options) → job_id with the real signature. The single printer.id is stored on app.state.printer_id and passed by PrintService; multi-printer routing comes with the persistence work.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
7. The API responds `202 {job_id, status: "queued"}`.
8. The queue worker dequeues (existing FSM) and calls `PTP750WPrinter.print_image(...)`.
9. The backend runs pre-print validation and prints.
10. Job status transitions: `queued → running → done` (or `→ failed` with `error_code`).
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. The spec now uses the actual JobState enum values (queued / paused / printing / completed / failed / cancelled) everywhere — data flow, REST schema, integration tests, and acceptance criteria. The PrintJobStatusResponse.status is typed as JobState directly.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
Comment on lines +181 to +186
- 404 when the job ID does not exist (or the TTL has expired).
- Response contains `status`, `error_code`, `error_message`, `error_detail`, timestamps.

### Persistence

In-memory store with a 5-minute TTL (existing). No database in this phase.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. The Persistence subsection now correctly states that _jobs has no eviction today and matches the existing TODO. Bounded job store + TTL/LRU is moved to Phase 5 alongside SQLite. The practical impact for First-Print is documented (small payloads, low daily volume, process restart acts as coarse eviction).

Comment thread docs/designs/2026-05-15-first-print.md Outdated
template_id: str
lookup: PrintLookupRequest | None = None
data: dict[str, str] | None = None
options: PrintOptions = PrintOptions()
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. options: PrintOptions = Field(default_factory=PrintOptions) so each request gets its own instance. Added model_config = ConfigDict(frozen=True) on PrintOptions as belt-and-braces against mutation across requests.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
Comment on lines +166 to +170
2. The API calls `PrintService.submit_print_job(request)`.
3. PrintService loads the template via `TemplateLoader.get(template_id)`. On miss → `TemplateNotFoundError` (404, synchronous).
4. PrintService resolves `LabelData`:
- When `lookup` is set: `AppLookupService.lookup(app, identifier)` → `LabelData`. On failure → `LookupFailedError` (502, synchronous).
- When `data` is set: `LabelData.from_dict(data)`.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. Introduced a dedicated RawLabelData Pydantic model that mirrors LabelData's shape (title, primary_id, qr_payload, secondary as list — coerced to tuple in LabelData). PrintService constructs LabelData(**raw.model_dump(), source_app="manual") — the client cannot set source_app (it is always "manual" for the raw path).

Comment thread docs/designs/2026-05-15-first-print.md Outdated
Comment on lines +124 to +133
### `PTP750WDriver` (PrinterModel)

- `model_id = "PT-P750W"`, `dpi=(180, 180)`, `print_head_pins=128`.
- `width_to_pixels(tape_spec)` returns `tape_spec.print_area_pins`.
- `build_print_job` raises `NotImplementedError` — encoding is done inside `ptouch`, not in the driver.
- `query_status(host="", port=9100, timeout_s=5.0)` delegates to `self._backend.query_status()`. The `host` argument is ignored because the backend is already bound to a connection.

### `PTP750WPrinter` (bridge to PrintQueue)

Adapter that combines PrinterModel + backend into the shape `PrintQueue._PrinterLike` expects (`async def print_image(image, *, tape_mm, **options)`):
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. The spec now states explicitly that build_print_job delegates to the ptouch library's internal raster builder (exact entry point confirmed in plan Phase 1; fallback to a raw encoder per Brother Raster Command Reference if not exposed). query_status raises ValueError when a non-matching, non-empty host argument is passed, rather than silently ignoring it. The driver now genuinely fulfills the Protocol contract.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
- All `ptouch` calls are synchronous and dispatched via `asyncio.to_thread`.
- `query_status` parses the ptouch status block into our `StatusBlock` dataclass.
- `print_image` validates against the cached status (see Error handling) and calls `printer.print(label, auto_cut=..., high_resolution=...)`.
- `send_bytes` opens a raw TCP connection to `host:9100` via `asyncio.open_connection` (so the call is non-blocking inside an `async def`), writes the bytes, and closes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. send_bytes now specifies the full lifecycle: writer.write(raster)await writer.drain()writer.close()await writer.wait_closed(). The spec calls both drain and wait_closed mandatory to avoid truncated raster under backpressure.

Comment on lines +281 to +291
```python
class Settings(BaseSettings):
# ...existing fields...
printer_backend: Literal["ptouch", "mock"] = "ptouch"
printer_pt_host: str | None = None
printer_pt_model: str = "PT-P750W"
printer_queue_timeout_s: float = 30.0
```

The env-var prefix `PRINTER_HUB_` is already established. Example: `PRINTER_HUB_PRINTER_PT_HOST=<printer-ip>`.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ae96393. Existing settings (pt750w_host, pt750w_port, ql820_host, ql820_port) stay as-is — no rename, no migration. First-Print adds only three new fields: printer_backend, printer_model, printer_queue_timeout_s. Each backend's from_settings(settings) knows which existing per-model host field to read (PTouchBackend reads pt750w_*, future BrotherQLBackend reads ql820_*). A migration to a more general structure is deferred until multi-printer support actually arrives.

Make printer extension a day-one property of First-Print rather than
a phase-2 retrofit. Concretely:

- Driver discovery is plugin-based: ModelRegistry walks the
  `label_hub.printer_models` entry-points group on app start, the
  built-in PT-Series driver self-registers, third-party drivers
  ship as separate pip packages.
- Backend discovery mirrors the same pattern with a new
  BackendRegistry on the `label_hub.printer_backends` group; the
  built-in `ptouch` and `mock` backends self-register.
- Backends expose `from_settings(settings) → PrinterBackend` so the
  lifespan stays trivial and series-agnostic (no per-backend
  if-tree).
- The bridge to PrintQueue's `_PrinterLike` becomes
  `Driver.make_queue_printer(...)` — a factory method on the driver,
  not a separate exported class. Subclassed drivers inherit it for
  free, the PT-specific `_PTPQueuePrinter` adapter stays private to
  `pt.py`.
- Settings: `printer_model` (series-agnostic string, resolved
  against ModelRegistry) replaces the PT-only `printer_pt_model`
  literal. Both `printer_backend` and `printer_model` are plain
  strings now so third-party plugins can be selected without code
  change.

New "Extensibility" section walks through five concrete extension
paths from smallest to largest intervention: subclass-in-series,
decorator backend (for vendor-lib quirks), subclass driver
(model-specific status/encoding), new series with new backend, and
third-party pip package via entry_points.

Acceptance criteria + test plan extended for plugin-discovery and
unknown-plugin failure modes.

Lifecycle hooks (before_print/after_print) are deliberately out of
scope for First-Print (YAGNI) — paths 2 and 3 cover every realistic
case today.

Refs #22
@strausmann
Copy link
Copy Markdown
Owner Author

Update — extensibility baked in (7fb6bf3)

Following the discussion about adding new printers / customising existing ones, I extended the design so that plugin discovery is a day-one property rather than a phase-2 retrofit. Five concrete extension paths are now documented (decorator backend, subclass driver, new series, third-party pip package, and same-series add-on).

Concrete architecture changes in this commit:

  • ModelRegistry is driven by setuptools entry_points (group label_hub.printer_models); built-in PT-Series self-registers.
  • New BackendRegistry mirrors that for backends (group label_hub.printer_backends); ptouch and mock self-register.
  • Backends expose from_settings(settings) → PrinterBackend so the lifespan has no per-backend if-tree.
  • Bridge to PrintQueue's _PrinterLike becomes Driver.make_queue_printer(...) — a method on the driver, not a separate exported class. Subclassed drivers inherit it for free.
  • Settings: printer_model (series-agnostic) replaces the PT-only printer_pt_model; both printer_backend and printer_model are plain strings so third-party plugins can be selected without code change.

Lifecycle hooks (before_print / after_print) deliberately stay out of scope for First-Print — the documented paths 2 and 3 cover every realistic case today and the PrinterModel Protocol stays unchanged.

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the design for the 'First-Print' pipeline, establishing the end-to-end flow from a REST API to physical printing on Brother PT-P750W hardware. The design covers a new PrinterBackend protocol, a PTouchBackend implementation, and the orchestration logic within a PrintService. The reviewer provided several critical architectural insights: specifically, the send_bytes method should be removed to adhere to the YAGNI principle, and raw TCP connections should be avoided to prevent resource conflicts with the ptouch library. Additionally, the reviewer noted that the PTP750WDriver implementation currently violates the PrinterModel protocol by ignoring the host argument in query_status and raising NotImplementedError for build_print_job, necessitating a refactor of the protocol or the driver to ensure type safety and runtime stability.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
high_resolution: bool = False,
) -> None: ...

async def send_bytes(self, raster: bytes) -> None: ...
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Regarding the send_bytes open question: following the YAGNI principle (cited in line 410 for lifecycle hooks), it is recommended to omit this from the initial PrinterBackend protocol. It introduces redundant connection logic and increases the maintenance surface before a concrete use case exists.

References
  1. Adhere to the YAGNI principle for features without immediate use cases, as applied to lifecycle hooks in the repository style guide. (link)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted. send_bytes removed from the PrinterBackend Protocol in 78cdab2 — YAGNI cut. No concrete caller in First-Print, and the second TCP path would conflict with ptouch's single session (your finding B). The escape hatch can be added back additively if a real use case appears (template editor writing raw raster, or a non-library backend); doing so is non-breaking.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
- All `ptouch` calls are synchronous and dispatched via `asyncio.to_thread`.
- `query_status` parses the ptouch status block into our `StatusBlock` dataclass.
- `print_image` validates against the cached status (see Error handling) and calls `printer.print(label, auto_cut=..., high_resolution=...)`.
- `send_bytes` opens a raw TCP connection to `host:9100` via `asyncio.open_connection` (so the call is non-blocking inside an `async def`), writes the bytes, and closes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Opening a raw TCP connection to port 9100 while the ptouch library is active may cause 'Resource Busy' errors. Brother PT hardware typically supports only one active TCP session at a time. It is safer to route all traffic through the library's transport mechanism to ensure serial access.

References
  1. Ensure compliance with Brother hardware limitations; the PT-series typically supports only one concurrent TCP connection per the Raster Command Reference. (link)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by removing send_bytes (your finding A). A note about Brother PT-Series allowing only one concurrent TCP session is now part of the YAGNI rationale so future implementers don't reintroduce the second connection by accident.

Comment on lines +142 to +144
async def query_status(self, host: str = "", port: int = 9100, timeout_s: float = 5.0):
# host is bound to the backend already; argument kept for Protocol compat
return await self._backend.query_status()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a conceptual mismatch between the PrinterModel protocol and the proposed PTP750WDriver. The protocol's query_status (line 32 of app/printer_models/base.py) is stateless, taking a host argument. However, the driver is stateful, being initialized with a backend that already contains the host. This results in an ignored host argument. Consider updating the PrinterModel protocol to reflect a stateful design if drivers are now expected to be bound to backends at instantiation.

References
  1. Ensure alignment between protocol definitions and concrete implementations to avoid 'dead' arguments and maintain type safety.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid alternative — and arguably the cleaner one. Kept the host-mismatch raise for First-Print so we don't touch the Protocol (which other PrinterModel call sites depend on), and documented the stateless-to-stateful Protocol refactor as a candidate follow-up PR in Open Questions. Bundling the Protocol change with First-Print would significantly expand scope; happy to file the follow-up issue once First-Print lands.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
Comment on lines +149 to +153
def build_print_job(self, image, tape_spec, auto_cut=True, high_resolution=False) -> bytes:
raise NotImplementedError(
"PT-P750W uses high-level backend.print_image; "
"raw raster encoding stays inside the ptouch library."
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Raising NotImplementedError for build_print_job violates the PrinterModel protocol (line 43 of app/printer_models/base.py). This will cause runtime failures in components expecting a full driver implementation. Refactor the protocol to make rasterization optional or move it to a sub-protocol (e.g., RasterizablePrinterModel) if the PT-series delegates this entirely to the backend.

References
  1. A class claiming to implement a Protocol must fulfill all required methods to prevent runtime crashes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid alternative. Two viable resolutions: (a) delegate build_print_job to ptouch's internal encoder (kept in 78cdab2), or (b) refactor the Protocol so rasterization moves into a RasterizablePrinterModel sub-protocol. The decision depends on what ptouch actually exposes — confirmed in implementation plan Phase 1. Both options are now documented in Open Questions; whichever turns out to be needed is a non-disruptive change.

Verified against the existing code; fixes for all eight findings:

- PrintQueue API: replace `enqueue(...)` with the real `submit(printer_id,
  image, tape_mm, **options) → job_id` signature throughout the data flow
  and acceptance criteria.
- Job state names align with the real `JobState` enum
  (queued / paused / printing / completed / failed / cancelled). Replaces
  the made-up `running` / `done` states in the data-flow, REST schema,
  integration tests, and acceptance criteria.
- TTL claim corrected: `_jobs` has no eviction today (queue source has a
  TODO about unbounded growth). Documented as planned for Phase 5 with
  the practical reasoning why First-Print can ship without it.
- `PrintOptions` shared-default-instance anti-pattern fixed:
  `Field(default_factory=PrintOptions)` plus `frozen=True` on the model.
- Raw-data payload schema is now explicit: a dedicated `RawLabelData`
  Pydantic model mirroring `LabelData`'s shape (title, primary_id,
  qr_payload, secondary as list-coerced-to-tuple), validated by the
  framework. `source_app` is set to "manual" by PrintService rather
  than accepted from the client.
- PTP750WDriver Protocol conformance: `build_print_job` is now described
  as delegating to the ptouch library's internal raster builder
  (exact entry point to confirm in plan Phase 1, fallback to a raw
  encoder per Brother Raster Command Reference). `query_status` raises
  on a non-matching host argument rather than silently ignoring it.
- `send_bytes` flow specifies the full lifecycle:
  write → `drain()` → `close()` → `wait_closed()` to avoid truncated
  raster streams under backpressure.
- Settings: keep the existing `pt750w_host`/`pt750w_port` naming.
  Add only `printer_backend`, `printer_model`, `printer_queue_timeout_s`.
  Backend `from_settings` knows which existing host field to read.

Refs #22
Four findings on the extensibility commit:

A — Removed `send_bytes` from `PrinterBackend` Protocol. No concrete
    caller exists in First-Print, the second TCP path would compete
    with the ptouch library for the printer's single TCP session
    (Brother PT hardware limit), and Customization Path 2 (decorator
    backend) does not need it. The hook can be added back additively
    if a real use case appears. Decorator-backend example in the
    Extensibility section trimmed to two methods.

B — Resource Busy on parallel TCP/9100 connections: resolved by
    removing `send_bytes`. A note about the single-session limit is
    now part of the YAGNI rationale.

C — `query_status(host=...)` Protocol mismatch: the current bound-
    backend driver still raises on a non-matching host (no behavioural
    change). The cleaner alternative — refactoring the Protocol so
    `query_status` takes no `host` — is documented in Open Questions
    as a follow-up PR that touches every PrinterModel call site, out
    of scope for First-Print.

D — `build_print_job` Protocol relaxation: kept the delegation-to-
    ptouch-encoder approach. The alternative — splitting rasterization
    into a `RasterizablePrinterModel` sub-protocol — is documented
    alongside C as a candidate follow-up. Either resolution will work,
    the decision depends on whether ptouch exposes a public encoder.

Refs #22
@strausmann strausmann marked this pull request as ready for review May 15, 2026 16:25
The print path uses TCP/9100 single-session (ptouch holds it during
a print). SNMP on UDP/161 gives us a parallel channel for the things
TCP cannot serve:

* Model auto-discovery (ADR 0004) — read Brother private OID
  1.3.6.1.4.1.2435.2.3.9.1.1.7.0 → PJL string → ModelRegistry.find_by_pjl.
* Live status during a running print — hrPrinterStatus and
  hrPrinterDetectedErrorState are reachable while ptouch holds the
  print socket.

ESC i S is retained for pre-print validation (it returns tape_mm and
media_type directly as integers/enums; SNMP would need string parsing
for the equivalent info).

Spec additions:

* New SNMP helper component in the component map and architecture
  diagram (sits beside the backend, not behind it).
* Two new async helpers: query_model_pjl() and query_live_status().
* New settings: printer_discover_via_snmp (default True),
  printer_snmp_community (default 'public'). printer_model becomes
  the fallback when SNMP fails or is disabled.
* Lifespan flow documented with the three valid configurations
  (auto+fallback, auto-only, manual).
* GET /jobs/{job_id} returns a `live` sub-object while the job is
  printing; null otherwise. SNMP failure at request time is
  non-fatal — the live block is just omitted.
* SnmpDiscoveryError and SnmpQueryError added to the exception
  hierarchy; SnmpDiscoveryError is the only one that can prevent
  app start.
* Four new acceptance criteria (12-14 + renumbered tail).

Refs #22
strausmann added a commit that referenced this pull request May 15, 2026
Six findings, all valid or sensible clarifications:

* query_status(host: str, ...) drops the empty-string default to match
  the PrinterModel Protocol signature exactly. Empty string is still
  accepted (means "use the bound backend's host"); a non-matching
  non-empty host still raises ValueError. The unit test that called
  query_status() without arguments now passes host="" explicitly.
* build_print_job raises NotImplementedError instead of returning
  empty bytes — unintended callers now fail loudly. Added a unit
  test asserting the raise. Coverage config already excludes
  `raise NotImplementedError`.
* Live-status SNMP call from GET /jobs/{id} now passes timeout_s=1.0
  (was the default 3.0s) — keeps the request path snappy even when
  SNMP is slow or unreachable; failure stays non-fatal and the live
  block is omitted.
* Retry-policy wording aligned with Acceptance Criterion 5: "exactly
  3 attempts" (was "3 retries", which Gemini read as 4 attempts).
  The implementation does 3 attempts with back-off sleeps 0s, 1s, 2s
  between them — semantic unchanged.

Memory growth of PrintQueue._jobs is acknowledged in the Persistence
section already; bounded eviction stays a Phase-5 deliverable for
First-Print. The other Gemini concern (lifespan bypassing
_resolve_printer_model in the Spec) lives in #57 and is fixed there.

Refs #22
Earlier draft of the lifespan snippet still did
ModelRegistry.find_by_model_id(settings.printer_model) directly,
bypassing the SNMP-discovery flow introduced in the SNMP-hybrid
section. Fixed: the snippet now picks model_id from
_resolve_printer_model (SNMP first, fall back to setting) before
the registry lookup. Also wires app.state.printer_host and
app.state.printer_snmp_community so the route handler can do live
SNMP queries.

Refs #22
…ter_id

Spec-vs-Plan drift fix: PrintService takes lookup_service +
printer_id (Plan Phase 10), not integration_registry. The lifespan
snippet in the spec is the authoritative example for the constructor
call.

Refs #22
@strausmann strausmann merged commit b100574 into main May 15, 2026
9 checks passed
@strausmann strausmann deleted the feat/first-print-design branch May 15, 2026 22:03
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