Skip to content

qemu_arm_mps2: replace the two-call init protocol with a closure - #22

Closed
ppannuto-claude wants to merge 1 commit into
ppannuto:qemu-arm-mps2from
ppannuto-claude:qemu-arm-mps2-closure-init
Closed

qemu_arm_mps2: replace the two-call init protocol with a closure#22
ppannuto-claude wants to merge 1 commit into
ppannuto:qemu-arm-mps2from
ppannuto-claude:qemu-arm-mps2-closure-init

Conversation

@ppannuto-claude

Copy link
Copy Markdown

Replaces mps2_base's two-call init protocol with a single start() that takes the chip allocation as a closure. This is the design question raised in the pre-merge review; opening it as its own PR so it can be judged on its merits, and dropped without disturbing anything else if the answer is no.

Stacked on #21 — only the last commit, qemu_arm_mps2: take the chip allocation as a closure, is new here. The four below it are #21's.

What changes

Today a board calls early_init(), then its own static_init!(ChipHw<C>, ...), then finish_start(). The ordering is enforced only by prose: "must be called exactly once, immediately after the early_init() call that produced early and the static_init!() that produced chip, both from the same boot, same C". That is three preconditions a caller can get silently wrong, in a crate whose whole purpose is to be shared by more than one board — and a third, if an500/an511 ever land.

The split is forced by a real constraint: static_init!(ChipHw<C>, ...) cannot be written inside a generic function, because the macro's type argument would name the enclosing function's own type parameter. Passing that one allocation in as a closure satisfies the constraint equally well — the closure body sits in the board's non-generic main(), at a concrete CortexMVariant — while keeping the sequence, and its ordering, in one place. EarlyInit goes away with the split, so the board no longer holds a half-initialized value across two unsafe calls.

Board side, before and after:

let early = unsafe { mps2_base::early_init::<CortexM3>(&PANIC_RESOURCES) };
let chip = static_init!(ChipHw<CortexM3>, ChipHw::<CortexM3>::new(early.peripherals));
let (board_kernel, platform, chip) = unsafe { mps2_base::finish_start(early, chip) };
let (board_kernel, platform, chip) = unsafe {
    mps2_base::start::<CortexM3, _>(&PANIC_RESOURCES, |peripherals| {
        static_init!(ChipHw<CortexM3>, ChipHw::<CortexM3>::new(peripherals))
    })
};

The objection worth weighing

AGENTS.md asks that static_init!() be called "either directly within main() or from the xx_component_helper!() class of macros". A closure defined inside main() and invoked once from main's callee is neither form literally — though it is called exactly once, from main, at a concrete type. If the rule is meant strictly, this PR is the wrong answer and the two-call version should stay.

Cost

None measurable. Both images are byte-for-byte the same size as on #21, and the merged frame is 1808 bytes against finish_start()'s 1824 (make stack-analysis), so peak init stack goes slightly down — there is one frame now where there were two, and it still returns before kernel_loop.

Also moves the inline(never) rationale out of the # Safety section, where rustdoc was rendering it as part of the safety contract. (Same fix applied to #21, so it is correct if that lands alone.)

Verified

Both boards build and boot; spi_loopback passes on an385 (SPI PASS) and c_hello on an386; the vector table is still at 0x0; make format-check, ci-job-syntax, ci-job-clippy, licensecheck and check-boards-readme.py all pass. No real hardware.

@ppannuto-claude
ppannuto-claude force-pushed the qemu-arm-mps2-closure-init branch 2 times, most recently from b452ff0 to bbbdc24 Compare September 2, 2026 19:15
mps2_base exported a two-call protocol: early_init(), then the board's
own static_init!(ChipHw<C>, ...), then finish_start(). The ordering was
enforced only by prose -- "must be called exactly once, immediately
after the early_init() call that produced `early` and the static_init!()
that produced `chip`, both from the same boot, same C" is three
preconditions a caller can get wrong silently, on a crate whose purpose
is to be shared by more than one board.

The split existed because `static_init!(ChipHw<C>, ...)` cannot be
written inside a generic function: the macro's type argument would have
to name the enclosing function's own type parameter. Passing that one
allocation in as a closure satisfies the constraint just as well -- the
closure body sits in the board's non-generic main(), at a concrete
CortexMVariant -- while keeping the whole sequence, and its ordering, in
one place.

start() replaces both halves and EarlyInit goes away with them, so the
board no longer holds a half-initialized value between two unsafe calls.

Costs nothing: both images are byte-for-byte the same size, and the
merged frame is 1808 bytes against finish_start()'s 1824 (per
`make stack-analysis`), so peak init stack is slightly lower rather than
higher -- there is one frame now where there were two.

Worth flagging for review: AGENTS.md asks that static_init!() be called
"either directly within main() or from the xx_component_helper!() class
of macros". A closure defined inside main() and called exactly once from
main's callee is neither form literally, though it is called once, from
main, at a concrete type.

Also move the inline(never) rationale out of the `# Safety` section,
where rustdoc was rendering it as part of the safety contract.

Verified: both boards build and boot, spi_loopback passes on an385 and
c_hello on an386, the vector table is still at 0x0, and format-check,
ci-job-syntax, ci-job-clippy, licensecheck and check-boards-readme all
pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L56it8rfV2vkZF9YRN5fa8
@ppannuto-claude

Copy link
Copy Markdown
Author

Integrated verbatim as d6b4825. Verified by content: the branch tree is byte-identical to this PR's head, start<C, F> is in place, EarlyInit/early_init/finish_start are gone from both mps2_base and the two main.rs files, both boards pass the closure, and the branch tip builds and passes spi_loopback on an385.

Closing.

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.

1 participant