Ask the sign whether it still holds a picture before re-pushing everything

[mjaksn]

Stacked on #78, which is where the base points. Merge that one first and GitHub
retargets this to main.

The periodic re-push wrote every picture, variable, message and the run sequence
again every interval, because a sign power cycled behind a still-connected
adapter is otherwise undetectable. The reasoning was right and the cost had
grown: writing a picture blanks the whole display, so a sign showing five icons
blanked five times an interval, and with an alert up the alert came off and went
back on around it, all for a repair that is almost never needed.

It now asks first. J on one picture file, and a reply carrying rows says the
sign still holds what it was given. A power cycle takes the sign's memory or
leaves it alone, so one file answers for the whole sign.

J rather than one of the five special-function reads because of what PR #77
measured: an unwritten picture answers with no height, no width and no rows,
while a written one answers rows. Written or not needs no comparison against
what the service believes is in the file, which matters, because whether the
sign stores a bitmap byte for byte as it was sent is still unmeasured and a
comparison that never matched would cost a read an interval and change nothing.

Measured on the fake transport, one refresh with five icons on the sign:

	before	now
alert up	6 things a person sees	1
no alert	5	0

Everything it is not sure about re-pushes exactly as before

No answer, an answer that does not parse, a link that is down, no icons to ask
about, or a picture file just given back to an icon that had lost one. That last
one is why _reclaim_pictures now reports: a file it reclaims holds whatever
the icon before it left in it, so the bitmap still has to be drawn, and asking
the sign about a different file would answer yes and leave that one undrawn.

So the worst this can do is cost a read. It cannot leave the sign in a state the
unconditional re-push would have repaired.

What it costs, and what it does not do

A read stalls a scrolling message about half a second, measured 2026-09-12, and
costs a held one nothing. That replaces five full-display blanks.

One unanswered read and it stops asking for the life of the process. Without
that, the sign simulator, which decodes reads and answers none, would pay the
read's three second deadline every interval with the sign's lock held and then
re-push anyway. A restart tries again. It also means a sign that goes quiet for
one read gives up the saving until the service restarts, which is the safe
direction but is worth knowing.

It does not ask about the priority file. F) reads the run time table
"including whether a priority message is running", but its reply layout is not
recorded in docs/protocol-notes.md and nothing here has parsed it, so an alert
is still re-asserted on the timer. That is the one flash left in the table above,
and inferring the alert survived from the picture surviving would skip the repair
in precisely the case the re-assert exists for: a sign that lost only its alert.

A service with no icons has nothing to ask about and behaves exactly as it did.
POST /sign/reboot is unchanged and still re-pushes unconditionally.

Checking it

tests/test_pictures.py::TestAskingBeforeRePushing. The two substantive ones
were run with the canary disabled and fail there: the intact-sign test finds a
picture write and a TEXT write where it expects only the read.

The one thing offline tests cannot settle is the reply shape, which comes from
the spike's run against the sign on 2026-09-13 and is recorded in
docs/protocol-notes.md. Worth watching the first run against real hardware for
the sign still holds the picture it was given, so nothing was re-pushed at
DEBUG: if that line never appears, the sign is answering something this does not
recognise and the behaviour has silently fallen back to what it was before.

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in reply validation and dirty-state handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a picture read-back check to avoid unnecessary periodic sign rewrites while preserving repair fallbacks.

Changes:

• Adds picture-read framing, parsing, and controller support.
• Updates refresh logic and tests.
• Updates configuration and protocol documentation.

File summaries

File	Summary	Review comments
tests/test_pictures.py	Adds read-before-repush coverage.	No findings.
readerboard/sign/controller.py	Adds picture-read support.	Nit (1): Update the read_dots_file documentation to reflect production use.
readerboard/services/registry.py	Adds conditional refresh logic.	Moderate (2): Respect _dirty state before skipping rewrites. Nit (1): Update the refresh docstring.
readerboard/protocol/replies.py	Parses picture-read replies.	Moderate (3): Validate dimensions and row payload, raising ReplyError for malformed replies. Nit (1): Generalize the protocol summary for read reply formats.
readerboard/config.py	Updates refresh configuration description.	No findings.
docs/protocol-notes.md	Documents read-back behavior.	No findings.
CHANGELOG.md	Records the refresh optimization.	No findings.
AGENTS.md	Updates repository guidance.	No findings.

Review details

Suppressed comments (3)

readerboard/protocol/replies.py:67

• The module-level protocol summary still says at lines 18-19 that a read reply echoes E, but this new picture path uses I for a J read. Generalize that summary to say the echoed write command depends on the read family; otherwise the same module now gives contradictory guidance about the reply format.

    ``command`` is the code the sign echoes, which is the **write** code for the
    thing that was read rather than the read code that was sent. That looks like
    a mistake in the document and is what it specifies; it holds for a picture
    read as well, which answers ``I`` to a ``J``.

readerboard/services/registry.py:642

• The docstring above still says at lines 635-636 that every refresh drops the controller cache and rewrites everything, but the new healthy-picture path returns before doing either. Please update that paragraph so the internal contract documents the new no-op fast path rather than describing the old unconditional behavior.

        So it asks first, which is what :meth:`_sign_still_holds_its_pictures`
        is for, and re-pushes only when the answer is no or there is no answer.

readerboard/sign/controller.py:404

• This new production call makes readerboard/protocol/frames.py:347's read_dots_file docstring false: it still says that nothing in the service sends this frame. Please update the protocol-builder documentation in the same change so the code docs match the new timer path.

        Goes through :meth:`read_special` because collecting a reply is the same
        work whatever was asked. The name is about the four special functions
        that were the only reads there were when it was written.
        """
        return await self.read_special(frames.read_dots_file(label))

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Validate malformed picture replies and bypass the probe while the controller is dirty.

Review details

Suppressed comments (3)

readerboard/protocol/replies.py:117

• This only unwraps the command and label, so any non-empty suffix is treated as a valid written picture. A malformed reply such as invalid dimensions or HHWW with no rows will therefore make refresh() skip the repair, despite the documented fallback for answers that do not parse. Validate the HHWW fields and row framing (or raise ReplyError) before returning a truthy result.

    return unwrap(reply, label, command=c.COMMAND_WRITE_DOTS)

readerboard/services/registry.py:642

• The surrounding docstring still says every refresh drops the controller cache and rewrites everything, but the new fast path below returns without doing either when the picture read succeeds. Please qualify that description to the probe-failure/repair path so the method documentation matches the new behavior.

        So it asks first, which is what :meth:`_sign_still_holds_its_pictures`
        is for, and re-pushes only when the answer is no or there is no answer.

readerboard/services/registry.py:657

• When _dirty is set by a partially applied multi-step write (for example, the rollback path at readerboard/services/registry.py:1049-1056), the picture probe can still succeed and this branch leaves the sign's text or run sequence divergent indefinitely. The previous unconditional refresh was the recovery path for _dirty; bypass the probe while dirty so _rewrite_all() runs.

            if await self._sign_still_holds_its_pictures():

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The picture-reply parser needs complete-payload validation, and the documented behavior and wording require updates.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

readerboard/config.py:151

• The public setting description says a healthy interval “costs nothing,” but the documented measurement is that the read itself stalls/blanks a scrolling message for about half a second. This wording will understate the observable cost for operators configuring the refresh interval; describe it as avoiding the full rewrite/display blanks rather than as cost-free.

            "can detect. The check is one short read, so an interval that costs nothing "
            "on a sign that is fine is the point of it"

readerboard/services/registry.py:654

• The refresh contract changed from unconditional re-pushing to a read-and-skip decision, but the timer docs in readerboard/api/app.py:101-125 still describe this as pushing everything and say blind re-pushing is the only repair. Please update those caller docstrings with the new fallback/read behavior; otherwise the service's lifecycle documentation contradicts this implementation.

        So it asks first, which is what :meth:`_sign_still_holds_its_pictures`
        is for, and re-pushes only when the answer is no or there is no answer.
        The re-push is expensive in the one currency that matters here: writing
        a picture blanks the whole display, so a sign with five icons on it blank
        five times an interval, for a repair that is almost never needed. Asking
        costs a message held still nothing at all and a scrolling one about half
        a second of stall, measured on 2026-09-12.

        Returns whether an alert was put back on the sign as part of it, which
        the caller needs so that it does not write the alert a second time. See
        :meth:`_rewrite_all`. Nothing is put back when nothing was re-pushed, so
        the caller re-asserts the alert on that path exactly as it would have
        before: what the sign holds in its priority file is a separate question
        from what it holds in a picture file, and this does not ask it.

readerboard/services/registry.py:660

• The parser intentionally does not compare the returned bitmap with the service's bitmap, and currently only establishes that the reply has a nonempty header. Saying it still holds “the picture it was given” overstates what this diagnostic proves and can mislead operators if the sign returns different picture data; log that the picture file was reported as written instead.

                logger.debug(
                    "the sign still holds the picture it was given, so nothing was "
                    "re-pushed"

readerboard/sign/controller.py:404

• This new call makes frames.read_dots_file() part of the service's periodic path, but its docstring still says “Nothing in the service sends this” (readerboard/protocol/frames.py:344-349). Please update that protocol-builder documentation in the same change so maintainers are not told the read is unused.

        return await self.read_special(frames.read_dots_file(label))

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite