Repository navigation
Bound a point's DCC address to the accessory space (#152) - #177
Merged
Merged
Conversation
points.dcc_address is an accessory address, valid on [1, 2044]. The old z.number().int().positive() accepted 9999 -- a valid loco address, and an accessory address PicoDCC will not transmit. An out-of-range command is dropped by the station, but setPoint resolves on the serial write regardless and the route holds the point lock believing the point moved. dccAccessoryAddressSchema carries the bound on pointCreateSchema, pointUpdateSchema and pointRowSchema. The read path is deliberate: there is no CHECK constraint and no migration (DD9, B9 -- a rebuild of a live table to guard a writer that does not exist), so the row schema is what catches a row predating the bound, throwing PointRowInvalidError on the way out of the database rather than reaching setPoint. The Configure -> Points form mirrors the bound as affordance; the backend stays the authority. The other half of #152 -- acting on the <X> a bad address now draws -- landed with #148 as the point-command-rejected route fault, and is untouched here. Decision recorded as docs/dcc-link.md D16. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #152.
points.dcc_addressis an accessory address. The old check wasz.number().int().positive(), which happily accepts 9999 — a perfectly good loco address,and an accessory address the command station will not transmit. Two numeric spaces that are
not the same one, and nothing said so.
The DCC accessory space ends at 2044, and PicoDCC enforces that at its parser's single
validation choke point: an address outside
[1, 2044]means the packet is never sent. Whatthat costs is specific.
setPointresolves on the serial write either way, the route holdsthe point lock believing the point moved, and with
positionFeedback: 'none'on every livepoint there is no confirmation channel to say otherwise — so a train is routed over a point
still lying the other way.
Why now
The issue was written when a bad address produced nothing at all. Two things have changed
since.
bazauto/PicoDCC#47made the station answer<X>, and #148 made the orchestrator acton one: an
<X>following an accessory command is now apoint-command-rejectedfaultagainst the route that issued it (
docs/dcc-link.mdD5). That half of #152 is alreadylanded — it arrived with #148 rather than here, and this PR does not touch it.
What is left is the half that stops the
<X>being generated at all. The route fault catchesthis late and at the worst possible moment: a fault raised while a train is running is an
expensive way to learn that somebody typed 20450 into a config form last Tuesday. Both halves
stand. This one refuses the value where it was entered.
What lands
services/validation.tsgainsdccAccessoryAddressSchema—z.number().int().min(1).max(2044)— on
pointCreateSchema,pointUpdateSchemaandpointRowSchema. A bad value onPOST/PUT .../pointsis an ordinary 400, which is the existing convention for a malformedoperator UI request and not a Safe-Stop.
The Configure → Points form mirrors the bound:
min/maxon the create input, and a namedDCC address must be 1–2044on both the create form and the inlineEditableCelledit,rather than a generic 400 coming back. Affordance, not authorisation — the backend stays the
authority, exactly as with role.
No CHECK constraint, and no migration
The issue asked for one. It is deliberately not here, following DD9's call on
sensors.in_serviceand B9's onblocks.length_mm: a CHECK on an existing SQLite tableforces drizzle-kit to emit a table rebuild — a
DROP TABLE points— against a databasedeployed to a live layout that cannot be reset.
I checked what the constraint would be guarding before deciding. The live layout holds six
points on addresses 11–16 (read with its
-wal, on the bench box), and the dev databaseholds the same. Nothing has ever written an out-of-range value, because the only writer is
the route that now rejects one.
The half worth having is covered instead by putting the bound on
pointRowSchema. That isthe read path, so a row written before this bound existed, or edited outside the app, throws
PointRowInvalidErroron the way out of the database rather than reachingsetPoint.It is
parsePointRow's stated posture already — a row in the database is either valid or itis corruption, and corruption must throw.
Also refused: validating in
formatSetPointorSerialDccAdapter.setPoint.domain/dccWireFormat.tsformats and never validates (#147), and by the time an addressreaches it the configuration error is already several layers deep.
The sub-address is untouched.
SerialDccAdapter.setPointhardcodes0, which is inside thevalid 0–7 range, so there is nothing to guard yet.
Tested
npm testfrom the repo root:npm run lintclean, and both workspace builds pass (tsc, andvite buildfor thefrontend).
New coverage:
tests/unit/services/validation.test.tstakesparsePointRow,pointCreateSchemaandpointUpdateSchemaover the boundaries — 1 and 2044 accepted, 0, -1,2045, 20450 and 1.5 refused, and 9999 named explicitly, since that is the loco address
that motivated the whole thing.
tests/integration/routes.test.tsasserts the 400 on bothPOSTandPUT, that the repository is never called, and that the boundary values stillcreate.
Docs
docs/dcc-link.md— D16, the decision: the bound, why the boundary rather than thereply, and the three refused alternatives above
docs/point-feedback.md— D10 gains the cross-reference, since that is where this table's"no CHECK constraint" position is recorded
docs/current-state.md— the DCC link sectionCLAUDE.md— the index line, and one Traps entry, because a bound with no migrationbehind it is exactly the kind of thing that reads as an oversight
README.md— Next Milestones now names Per-loco speed step mode (128 / 28) #151 as what is left of the command-station workdocs/mqtt-contract.mdis untouched and needed no amendment: nothing here changes a topic,payload, QoS or retention setting.