fix: Fix IntegrityId and response parsing for Browse.#764
Conversation
- Shorten the expected response from 14 to 10 bytes since the response is not expected to carry the SessionId - Add the IntegrityId as being optional for the Browse symbolic read. I believe this has to be a incremental number, otherwise the PLC drops the connection.
gijzelaerr
left a comment
There was a problem hiding this comment.
Thanks for this @bvanelli — both fixes are well-motivated and the response header change is independently confirmed by @EliTheCoder's hex dump analysis in #763.
Response header 14 → 10 bytes — correct. The CreateObject response doesn't carry a SessionId field (only the request does). The S7CommPlusDriver reference confirms this. The 4-byte misalignment was causing the parser to consume the entire attribute tree (including ServerSessionVersion) as fake "object IDs".
IntegrityId — a few things to address:
-
Sync/async inconsistency: the async
read_symbolicpasseswith_integrity=False, but the sync_s7commplus_client.pyread_symbolicwasn't updated — it still calls_build_symbolic_read_payload(access_area, lids, symbol_crc)with the defaultwith_integrity=True. These should behave the same way. -
_integrity_id_readinitialization: the async client referencesself._integrity_id_readbut I don't see it being initialized or incremented anywhere in this diff. Does the base class set this up? If not, this will raiseAttributeError. -
with_integrity=Falsefor browse: removing the integrity ID entirely for browse reads works as a workaround, but as you note it needs a permanent solution. Worth a# TODOand maybe a tracking issue so it doesn't get lost.
The header fix is solid and I'd be happy to merge that independently if you want to split it out — it unblocks V2 session setup for anyone hitting the framing bug.
|
Hi @gijzelaerr , The sync
This is initialized here: I believe the incrementing was never implemented, needs to be done in a follow up. It would be a lot easier to tests once we have a stable working browse. It should not matter as of now as this is ignored. For TestingBefore my changes (full)After my changes (partial, collapsed all browse calls) |
|
Confirming this works on a second CPU model. Tested the sync path ( Browse returned 5144 tags with correct names/types/offsets ( Two notes for whoever picks up the follow-ups:
|
This PR does two things for the Browse to work (tested on a S7 15XX):
SessionId: Can be found on CreateObjectResponse.DeserializeFromPdu, where theSessionIdis not expected to be available on the response (only request, therefore you need 4 bytes less).IntegrityIdas being optional for the Browse symbolic read. I believe this has to be a incremental number, otherwise the PLC drops the connection. You can this as optional on GetMultiVariablesRequest.Serialize.There needs to be a "permanent" solution for integrating an incremental
IntegrityId, but this changes were enough to acomplish a basic browse.Here is my example browse:
As a heads up, I have tested the async functions, but not the sync one.