test(canopen): add the normative negative tests from #52 - #159
Conversation
CiA 301 separates the expedited bit from the size bit. A download initiate of 0x20 and an upload response of 0x40 are segmented transfers whose length arrives with the segments. The server was committing the initiate's four reserved bytes, and the client ignored 0x40. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
A download initiate without a size indicator must not commit those bytes, and an upload response without one must open the segment phase instead of being dropped or read as a multi-gigabyte length. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
SdoFrames and SdoBlockFrames were only exercised through a node, apart from the download-initiate length split. Round-trip the frame fields, including a size field that must be ignored when the size bit is clear. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08c021d5d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
A size-less download allocated a new buffer of the exact length so far on every segment and copied every preceding byte. Capacity now doubles, and clamps to MaxSdoTransferBytes, so a transfer near the cap stays linear. A segment past the cap still aborts with OutOfMemory before any copy. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
Drive the size-less server through a doubling transfer, a cap abort, a cap below the growth seed, and the fixed-width checks that only the last segment can make. Also pin the sized initiate and short-transfer length aborts next to those branches. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
|
Followed up on the patch coverage gap in CanOpenNode. The size-less download now has tests for the geometric grow (double, clamp to the cap, and a segment that fits in the slack), the OutOfMemory abort before the cap, a successful commit, and the fixed-width length checks on that path. Codecov should refresh on this push. |
A 0x21 whose declared length is past the cap aborts with OutOfMemory before a session exists. A download ack that names an upload still in its initiate phase is ignored, and the upload still completes. Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
2 participants
What does this change?
Closes #52.
The interop rows in that issue are already pinned on main, except the two SDO initiate forms in #38. Those were closed by a documentation commit whose message contained the words "closed #38"; the server still treated
0x20as an expedited write of four bytes, and the client still ignored0x40. This accepts both as segmented transfers whose length arrives with the segments, and adds the tests that fail if that check is removed.SdoFramesandSdoBlockFramesnow have codec unit tests of their own.A size-less download used to allocate a new buffer of the exact length so far on every segment. Capacity now starts at 8 bytes and doubles, clamping to
MaxSdoTransferBytesonce another double would pass it. A segment past that cap still aborts withOutOfMemorybefore anything is copied. Updated onto main after #157 and #158.Coverage against the issue table
CanOpenSdoCorrectnessTests: upload response, download ack, duplicate ack in the segment phase, and both block initiate responsesJ1939TpTests.Rts_To_The_Global_Address_Does_Not_Open_A_SessionIsoTpChannelIntegrationTests.MultiFrame_Receive_Ignores_A_Shortened_ConsecutiveFrame_That_Is_Not_The_LastUdsClientTests.SecurityAccess_Treats_An_AllZero_Seed_As_Already_UnlockedUdsClientTests.P2_Ends_With_The_First_Frame_Of_A_MultiFrame_ResponseJ1939TpTests.Parallel_Bam_Sends_Are_Transmitted_One_After_AnotherJ1939TpTests.Abort_Reason_Goes_On_The_Wire_As_Table_7_Assigns_ItSdo_Server_Treats_Download_Initiate_0x20_As_Segmented_Not_ExpeditedandSdo_Client_Treats_Upload_Response_0x40_As_Segmented_Not_Ignored, with the decode fix they requireCanOpenPdoEngineTests.Tpdo_Mapping_Entries_Take_Effect_By_Sub_Index_Not_By_Write_OrderBusStateMonitorTests.An_Error_Frame_Storm_Coalesces_Into_One_Pending_Actor_Post"123456789"→0x31C3CanOpenSdoCorrectnessTests.Crc16_Matches_The_CiA301_Check_Value(already on main)SdoFrames/SdoBlockFramescodec unit testsCanOpenCodecTests, including a size field that must be ignored when the size bit is clearNothing in the table is blocked on a further product fix.
The size-less server path (double, clamp to the cap, a segment that fits in the slack, the over-cap abort, and fixed-width length checks) is covered in
CanOpenSdoCorrectnessTests.Local Release on this revision:
dotnet test --filter FullyQualifiedName~TestCases.CANopenpassed 265 / 265.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)The branch contains
fix(canopen)commits, so the patch notes are the sizeless segmented initiate and the geometric growth of that buffer. The other commits are tests.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceedsdotnet test CanKit.Pro.sln -c ReleasepassesFR-RAW-031,ADR-7), if anyFR-CO-002 / FR-CO-003 for the SDO initiate forms. The other rows cite the requirements already named in their tests.