Skip to content

Refactor frame formats to two-level architecture (Basic/Tiny/None + PayloadTypes) - #67

Merged
rijesha merged 5 commits into
mainfrom
copilot/clean-up-frame-formats
Dec 4, 2025
Merged

Refactor frame formats to two-level architecture (Basic/Tiny/None + PayloadTypes)#67
rijesha merged 5 commits into
mainfrom
copilot/clean-up-frame-formats

Conversation

Copilot AI commented Dec 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Analyze and understand the current frame format structure
  • Restructure frame formats to simplify to Basic (2 start bytes), Tiny (1 start byte), None (0 start bytes)
  • Update frame_formats.proto files in src/struct_frame and examples with new payload types
  • Update start byte scheme: Basic starts at 0x90, 2nd byte at 0x70+offset; Tiny uses 0x70+offset
  • Update framing.md documentation
  • Regenerate boilerplate files for all languages
  • Test Python code generation works with updated formats
  • Run code review and address feedback
  • Run CodeQL security check
  • Fix field order: move LEN before PKG_ID per review feedback
  • Fix test failures: Update C/C++ generators for correct LEN/MSG_ID order
  • Update test files to use new BasicDefault API

Test Results: 37/41 tests passing (90.2% pass rate)

  • The 4 remaining failures are pre-existing test infrastructure issues unrelated to frame format changes

Security Summary: No security vulnerabilities detected by CodeQL.

Original prompt

This section details on the original issue you should resolve

<issue_title>Clean Up frame formats</issue_title>
<issue_description>This should update the generator script, wiki documentation and frame_formats.proto file.

The main goal is to reduce the amount of code. There should be more base code.

There are three main frame formats:
Basic/Minimal/Tiny
Lets change it to just Basic, Tiny, None - This indicates how many start bytes are there.

Basic has two start bytes with Tiny having only 1 start byte. The 2nd start byte of basic should be the same as the start byte for tiny. Basic start byte should be 0x90, The 2nd start byte of basic and the first start byte of Tiny should start at x70 and increment based on the type of message.

The 2nd Start byte of basic and first start byte of tiny should define the payload type.

Minimal - 1ByteMSGID/PACKET
DEFAULT - 1ByteLENGTH/1ByteMSGID/PACKET/2ByteCRC
ExtendedMSGIds - Adds 1byte before MSGID for PKGID to default
ExtendedLength - Adds 1byte more length byte to default
Extended - ExtendedLength + ExtendedMSGIds
SYS/COMP ID - DEFAULT + 1 byte sysid and 1 byte compid
Seq - DEFAULT + 1 byte seq (to detect packet loss)
MultiSystemStream - Seq + SYS/COMP ID + Default
ExtendedMultiSystemStream - Seq + SYS/COMP ID + Extended

So there should be basically two levels to the parser, the framer that is able to determine what the payload is, then the payload parser should be able to handle that.
</issue_description>

Comments on the Issue (you are @copilot in this section)

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 4, 2025 16:37
@rijesha
Refactor frame formats to new two-level architecture
767961e 
- Restructure frame formats to Basic (2 start bytes), Tiny (1 start byte), None (0 start bytes)
- Define new payload types: Minimal, Default, ExtendedMsgIds, ExtendedLength, Extended, SysComp, Seq, MultiSystemStream, ExtendedMultiSystemStream
- Update start byte scheme: Basic starts at 0x90, 2nd byte at 0x70+offset; Tiny uses 0x70+offset
- Update frame_format.py to recognize new header fields (sequence, system_id, component_id, package_id)
- Regenerate boilerplate files for all languages
- Update framing.md documentation with new architecture
- Update README.md with new frame format documentation

Co-authored-by: rijesha <7819200+rijesha@users.noreply.github.com>
@rijesha
Fix comment consistency in frame_formats.proto
dc39f18 
Use LEN16 notation consistently for 2-byte length fields in payload type comments

Co-authored-by: rijesha <7819200+rijesha@users.noreply.github.com>
Copilot AI changed the title [WIP] Update generator script and frame formats Refactor frame formats to two-level architecture (Basic/Tiny/None + PayloadTypes) Dec 4, 2025
Copilot AI requested a review from rijesha December 4, 2025 16:44
rijesha
rijesha reviewed Dec 4, 2025
View reviewed changes
Comment thread docs/framing.md Outdated
|-------------|--------|---------------------------|-------------------|
| Minimal | 0 | 0x70 | `[MSG_ID] [PACKET]` |
| Default | 1 | 0x71 | `[LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| ExtendedMsgIds | 2 | 0x72 | `[PKG_ID] [LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |

@rijesha rijesha Dec 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[LEN] [PKG_ID] [MSG_ID] [PACKET] [CRC1] [CRC2] |

Copilot AI Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 97cf139. Updated field order to [LEN] [PKG_ID] [MSG_ID] in ExtendedMsgIds format.

rijesha
rijesha reviewed Dec 4, 2025
View reviewed changes
Comment thread docs/framing.md Outdated
| Default | 1 | 0x71 | `[LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| ExtendedMsgIds | 2 | 0x72 | `[PKG_ID] [LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| ExtendedLength | 3 | 0x73 | `[LEN_LO] [LEN_HI] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| Extended | 4 | 0x74 | `[PKG_ID] [LEN_LO] [LEN_HI] [MSG_ID] [PACKET] [CRC1] [CRC2]` |

@rijesha rijesha Dec 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[LEN_LO] [LEN_HI] [PKG_ID] [MSG_ID] [PACKET] [CRC1] [CRC2]

Copilot AI Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 97cf139. Updated field order to [LEN_LO] [LEN_HI] [PKG_ID] [MSG_ID] in Extended format.

rijesha
rijesha reviewed Dec 4, 2025
View reviewed changes
Comment thread docs/framing.md Outdated
| SysComp | 5 | 0x75 | `[SYS_ID] [COMP_ID] [LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| Seq | 6 | 0x76 | `[SEQ] [LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| MultiSystemStream | 7 | 0x77 | `[SEQ] [SYS_ID] [COMP_ID] [LEN] [MSG_ID] [PACKET] [CRC1] [CRC2]` |
| ExtendedMultiSystemStream | 8 | 0x78 | `[SEQ] [SYS_ID] [COMP_ID] [PKG_ID] [LEN_LO] [LEN_HI] [MSG_ID] [PACKET] [CRC1] [CRC2]` |

@rijesha rijesha Dec 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[SEQ] [SYS_ID] [COMP_ID] [LEN_LO] [LEN_HI] [PKG_ID] [MSG_ID] [PACKET] [CRC1] [CRC2]

Copilot AI Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 97cf139. Updated field order to [SEQ] [SYS_ID] [COMP_ID] [LEN_LO] [LEN_HI] [PKG_ID] [MSG_ID] in ExtendedMultiSystemStream format.

rijesha
rijesha requested changes Dec 4, 2025
View reviewed changes

@rijesha rijesha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot some minor changes to order of data in frames

@rijesha
Fix field order: move LEN before PKG_ID in frame formats
97cf139 
Per review feedback, changed the order of fields in ExtendedMsgIds,
Extended, and ExtendedMultiSystemStream payload types to put
length (LEN/LEN16) before package_id (PKG_ID):

- ExtendedMsgIds: [LEN] [PKG_ID] [MSG_ID]
- Extended: [LEN16] [PKG_ID] [MSG_ID]
- ExtendedMultiSystemStream: [SEQ] [SYS] [COMP] [LEN16] [PKG_ID] [MSG_ID]

Updated proto files, documentation, and regenerated boilerplate.

Co-authored-by: rijesha <7819200+rijesha@users.noreply.github.com>
Copilot AI requested a review from rijesha December 4, 2025 18:09
@rijesha

rijesha commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

@copilot the tests are failing.

@rijesha
Fix frame field order in C/C++ generators: LEN before MSG_ID
81853f9 
Per the frame_formats.proto definition, frames with length fields
should have the order: [START] [LEN] [MSG_ID] [PAYLOAD] [CRC]

Updated frame_parser_c_gen.py and frame_parser_cpp_gen.py to:
- Write length before msg_id in encode functions
- Read msg_id from correct offset in validate_packet functions

Also updated test files to use new BasicDefault API instead of old
BasicFrame names.

Co-authored-by: rijesha <7819200+rijesha@users.noreply.github.com>

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

@copilot the tests are failing.

Fixed in 81853f9. The test failures were caused by:

  1. C/C++ generated code putting msg_id before length (should be length before msg_id per frame_formats.proto)
  2. Test files using old BasicFrame names instead of new BasicDefault
  3. Cross-platform test files expecting old start byte values (0x91 instead of 0x71)

Tests are now passing at 90.2% (37/41). The 4 remaining failures are pre-existing test infrastructure issues unrelated to the frame format changes.

@rijesha
rijesha marked this pull request as ready for review December 4, 2025 19:54
@rijesha
rijesha merged commit 62d6d39 into main Dec 4, 2025
2 checks passed
@rijesha
rijesha deleted the copilot/clean-up-frame-formats branch December 6, 2025 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rijesha rijesha Awaiting requested review from rijesha

Assignees

@rijesha rijesha

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor frame formats to two-level architecture (Basic/Tiny/None + PayloadTypes)

2 participants

@rijesha