Skip to content

proto: split protocol contract, shared layout and POSIX PAL header - #9

Merged
bugobliterator merged 5 commits into
masterfrom
proto/split-contract
Oct 10, 2026
Merged

bugobliterator merged 5 commits into
masterfrom
proto/split-contract

Conversation

@bugobliterator

Copy link
Copy Markdown
Member

RateCommand/ArmingState fields for the split, CubePilotFW/SplitLayout.h (shared SRAM4 layout, kLayoutHash 0xED77D04D), SplitProtocol.h constants and the POSIX PAL header.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Version mismatch detection relies on mailboxes whose offsets can differ between the incompatible layouts being detected.

3 open findings
What changed in this PR

Introduces the shared M7/M4 split-protocol contract and SRAM4 layout.

Changes:

  • Extends rate, arming, and core-status protobuf messages.
  • Adds protocol constants, layout hashing, and transport registration.
  • Adds POSIX declarations and layout tests.
File Description
CMakeLists.txt Builds split-layout sources and test library.
CubePilotFW/​SplitLayout.cpp Registers per-core shared-memory groups.
CubePilotFW/​SplitLayout.h Defines slots, sizing, and layout hash.
CubePilotFW/​SplitProtocol.h Defines protocol constants and version.
CubePilotFW/​pal/​Posix.h Declares POSIX shared-memory interface.
proto/​Common.proto Extends status and arming contracts.
proto/​Rc.proto Extends rate commands.
tests/​CMakeLists.txt Registers the layout test.
tests/​split_layout_test.cpp Tests layout invariants and transport direction.

🧠 Review effort: Balanced


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

Comment thread CubePilotFW/SplitLayout.h Resolved
Comment thread proto/Common.proto Resolved
Comment thread proto/Common.proto
ARMING_FLAG_FAILSAFE = 4; // Failsafe active
ARMING_FLAG_FAILSAFE_STAGE1 = 4; // M4: RateCommand stale > stage-1 threshold
ARMING_FLAG_FAILSAFE_STAGE2 = 8; // M4: latched descend
ARMING_FLAG_VERSION_MISMATCH = 16; // Peer protocol_version differs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in #10 (5e777bd): VERSION_MISMATCH now covers a layout_hash mismatch as well as protocol_version.

@bugobliterator
bugobliterator removed this pull request from stack #23 October 10, 2026 02:31
@bugobliterator
bugobliterator changed the base branch from docs/cuberacer-split to impl/cuberacer-m4 October 10, 2026 03:09
@bugobliterator
bugobliterator added this pull request to stack #24 October 10, 2026 03:09
@bugobliterator
bugobliterator requested a balanced review from Copilot October 10, 2026 03:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The public registration API has undefined behavior for a null base, and the mismatch flag documentation omits layout-hash mismatches.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread CubePilotFW/SplitLayout.cpp Resolved
Base automatically changed from impl/cuberacer-m4 to master October 10, 2026 03:21
bugobliterator and others added 5 commits October 10, 2026 14:24
CoreInfo, ArmingState and RateCommand sizes come from their field options.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KqxcxgduRYDCp2rs1XqK5M
The previous asserts compared each envelope against the message it contains,
which nanopb guarantees. Check each envelope plus frame header against its
slot size, each slot against the layout end, and the layout end against the
receiver region.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The mismatch flag documentation is incomplete, and reverse duplex registration lacks coverage.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Comment on lines +48 to +52
assert(m7.send(kRc.groupId, payload, sizeof(payload)) == 0);
uint8_t out[64];
size_t len = 0;
assert(m4.receive(kRc.groupId, out, sizeof(out), &len) == 0);
assert(len == 4 && out[3] == 4);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reverse duplex coverage (M4-TX/M7-RX mappings) is added at the top of this stack in #18, to avoid re-cascading the whole chain for a test-only change.

@bugobliterator
bugobliterator merged commit ed2739c into master Oct 10, 2026
1 check 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