Skip to content

fix: accept NULL line, font and foreground attribute references; add try_from_iop - #48

Merged
Notgnoshi merged 1 commit into
Open-Agriculture:daan/terminal-designer-changesfrom
Arjan-Woltjer:fix/nullable-line-font-foreground-refs
Sep 11, 2026
Merged

Notgnoshi merged 1 commit into
Open-Agriculture:daan/terminal-designer-changesfrom
Arjan-Woltjer:fix/nullable-line-font-foreground-refs

Conversation

@Arjan-Woltjer

@Arjan-Woltjer Arjan-Woltjer commented Sep 11, 2026 •

Copy link
Copy Markdown

Fixes #47.

What

Object::read returned ParseError::UnexpectedNullObjectId for any Output Line / Rectangle / Ellipse / Polygon whose Line Attributes reference is NULL (0xFFFF), any Input/Output String/Number whose Font Attributes reference is NULL, and any Input Boolean whose Foreground Colour reference is NULL. ISO 11783-6 allows NULL for all of these (the Output Polygon table gives the range as 0-65534, 65535 outright), AgIsoStack++ accepts it in every one of the corresponding get_is_valid checks, and real terminals ship such pools — a John Deere StarFire pool captured off the bus nulls line attributes on 213 shapes and foreground colour on all 38 of its Input Booleans, and the John Deere VT accepted it with error bitmask 0x00.

Because ObjectPool::from_iop loops with while let Ok(o) = Object::read(..), the first such object silently ended the parse: that StarFire pool came back as 1 638 of 9 619 objects, and with no Working Set, because John Deere sends the Working Set last.

Changes

  • The nine fields become NullableObjectId, are read with .into(), and are collected with push_nullable_id in Object::referenced_objects. write_u16 already takes NullableObjectId, so the writer is untouched and NULL round-trips as 0xFFFF (asserted in the new test).
  • ObjectPool::try_from_iop / try_extend_with_iop: strict counterparts that return the first ParseError. from_iop / extend_with_iop keep their lenient behaviour and now delegate to the strict path. End-of-data between objects is the normal end of a pool; end-of-data inside an object is reported as DataEmpty.
  • Tests: NULL reference on Output Line, Output String and Input Boolean; a 31-byte pool with a NULL reference before its Working Set that must keep all three objects, keep the Working Set, and round-trip; and try_from_iop on a truncated object.
  • One-token fix in the existing read_working_set_test (Colour::new_by_id(0xF0).into()): on this branch WorkingSet::background_colour is u8, so the test target did not compile before this PR.

This is a type change on public struct fields, so AgIsoTerminalDesigner needs a small companion change (it already treats fill_attributes the same way): Open-Agriculture/AgIsoTerminalDesigner#36.

Verification

  • cargo fmt --check clean; cargo clippy produces the identical 34 pre-existing lib lints as the base branch and no new ones; cargo test --lib 21/21 (including the previously non-compiling one). The two doctests (Object::referenced_objects, ObjectPool::from_iop) fail on the base commit 5274565 as well — one references an undefined variable, the other opens a hard-coded C:/project/... path — and are untouched here.
  • Against real pools, reading with this branch:
pool before after
John Deere StarFire (327 501 B) stopped at object 1 640, 0 Working Sets 9 619 objects read to the last byte, 1 Working Set
John Deere tractor ECU (21 643 B) stopped at object 31, 0 Working Sets 47 objects, 1 Working Set
John Deere tractor ECU AUX pool (1 260 B) ok ok (105 objects)
our own MW03 pool (538 B) ok ok (26 objects, 1 Working Set)
31-byte reproducer from the issue 0 objects 3 objects, 1 Working Set

Based on daan/terminal-designer-changes because that is what the Designer builds from and where the NullableObjectId plumbing from #40 already lives; happy to retarget to main once #40 lands.

🤖 Generated with Claude Code

ISO 11783-6 allows the Line Attributes reference of the output shape
objects, the Font Attributes reference of the string and number objects
and the Foreground Colour reference of Input Boolean to be the NULL
object ID, and real pools use it: a John Deere StarFire pool nulls line
attributes on 213 shapes and foreground colour on all 38 of its Input
Booleans. Reading them with try_into() failed with
UnexpectedNullObjectId, and because from_iop stops at the first error
the rest of the pool was silently dropped -- including the Working Set,
which John Deere sends last.

Make the nine fields NullableObjectId. The writer already accepts it, so
NULL round-trips as 0xFFFF.

Add ObjectPool::try_from_iop / try_extend_with_iop, strict counterparts
that return the first ParseError; from_iop keeps its lenient behaviour.

The existing read_working_set_test did not compile on this branch
(Colour where u8 is expected); one-token fix so the test target builds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Notgnoshi

Copy link
Copy Markdown
Member

Thanks for your contribution! I'll merge despite the CI failures because they look like rust toolchain / dependency drift bitrot that you shouldn't be responsible for fixing.

@Notgnoshi
Notgnoshi merged commit 1f49651 into Open-Agriculture:daan/terminal-designer-changes Sep 11, 2026
1 of 3 checks passed
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