Skip to content

fix: Align all 0x34 and 0x36 framing bytes - #848

Open
bvanelli wants to merge 1 commit into
gijzelaerr:masterfrom
bvanelli:fix/align-all-0x34-byte-payloads
Open

fix: Align all 0x34 and 0x36 framing bytes#848
bvanelli wants to merge 1 commit into
gijzelaerr:masterfrom
bvanelli:fix/align-all-0x34-byte-payloads

Conversation

@bvanelli

Copy link
Copy Markdown
Contributor

Changes

Aligns the transmission with the received bytes when it comes to the transport flags. The C# library has the following definitions:

reference request class TransportFlags
InitSslRequest.cs:23 0x30
CreateObjectRequest.cs:23,57 0x36
DeleteObjectRequest.cs:23 0x34
ExploreRequest.cs:24 0x34 (with a // or 0x36??? comment)
GetMultiVariablesRequest.cs:24 0x34
GetVarSubstreamedRequest.cs:23 0x34
SetMultiVariablesRequest.cs:24 0x34
SetVariableRequest.cs:23 0x34

I aligned based on the testing with a S71500 PLC, and it seems to accept the framing 0x34 or 0x36 in most cases, but sometimes the wrong frame leads to a connection reset. Here are the sent and received frames:

request tx before tx now rx
InitSsl 0x30 0x30 0x70
CreateObject 0x36 0x36 0x36
SetMultiVariables 0x34 0x34 0x34
GetVarSubStreamed 0x36 0x34 0x34
Explore 0x34 0x34 0x34
DeleteObject 0x36 0x34 0x34

Once the new codes from the C# library are added, they can be added to the same frozenset to keep compatibility.

PS: I checked if this was the cause why #820 was not working, but that was not the issue.

@gijzelaerr gijzelaerr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The transport-flag mapping is protocol behavior and currently has no regression coverage in this PR. Please add byte-level tests for both sync and async request builders that demonstrate:

  • every function in FLAGS_34_FUNCTION_CODES emits 0x34;
  • an ordinary CreateObject remains 0x36;
  • session-key requests retain their existing 0x34 override;
  • _delete_session() emits 0x34 in both clients.

The hardware result is valuable, but without these tests a later request-builder refactor can silently restore the framing mismatch. This is also a prerequisite for #849, whose capture fixtures currently fail without this branch, so the boundary needs to be locked down before merge.

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