Skip to content

fix(s7commplus): remove misleading TLS hint - #851

Open
gijzelaerr wants to merge 1 commit into
masterfrom
fix/createobject-error-guidance
Open

fix(s7commplus): remove misleading TLS hint#851
gijzelaerr wants to merge 1 commit into
masterfrom
fix/createobject-error-guidance

Conversation

@gijzelaerr

Copy link
Copy Markdown
Owner

Summary

  • stop treating a non-zero CreateObject return value as evidence that TLS is required
  • share the same status logging between synchronous and asynchronous clients
  • clarify that parsing continues because affected PLCs can still return usable session data

Testing

  • uv run --no-sync pytest (1823 passed, 78 skipped)
  • uv run --no-sync pre-commit run --all-files

Progresses #710.

@gijzelaerr
gijzelaerr enabled auto-merge (squash) September 1, 2026 16:08
@gijzelaerr

Copy link
Copy Markdown
Owner Author

@bvanelli I requested your review because this change is directly based on the CreateObject behavior seen in the recent PLC traces: a non-zero return value is logged without falsely claiming that TLS is required, while usable session parsing continues.

Could you check that the revised interpretation and sync/async behavior match what you observed? No additional byte capture is needed for this one.

@bvanelli

bvanelli commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@bvanelli I requested your review because this change is directly based on the CreateObject behavior seen in the recent PLC traces: a non-zero return value is logged without falsely claiming that TLS is required, while usable session parsing continues.

Could you check that the revised interpretation and sync/async behavior match what you observed? No additional byte capture is needed for this one.

I assume you are talking about my change (Int16)ReturnValue < 0 for the Legitimation?

I did check the reference implementation, and those seems like fundamentally different things. Also, S7CommPlusConnection.Connect() does not seem to check it at all, and it seems to go straight to TLS every time:

https://github.com/thomas-v2/S7CommPlusDriver/blob/dbd61e447c7aaf4486cf1f1fe0201212a6bd93c8/src/S7CommPlusDriver/S7CommPlusConnection.cs#L436-L445

Is there a historical reason why this warning (or the flag tls=True) exists?

@gijzelaerr

Copy link
Copy Markdown
Owner Author

The historical reason is #760: when the CreateObject return value was first surfaced, I inferred from that TLS failure that a non-zero value generally meant the session was being rejected and that TLS was the likely remedy. That was an incident-driven assumption, not a rule established by a capture or the reference implementation. The newer traces show that inference is too strong because usable session data can follow a non-zero value. You are also right that the reference connection path goes directly to TLS; our client additionally retains V1 non-TLS support. This PR only removes the TLS claim from CreateObject status logging. It does not change the separate signed ReturnValue check used during legitimation.

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