_parse_read_response does not raise an exception in case of a SystemEvent

[bvanelli]

See #775 (comment)

Incorrect framing can cause a V254 SystemEvent and it this gets completely swallowed by the library at _parse_read_response because it thinks this is a valid response and does not raise an SystemEvent exception.

Here is the reproduction:

# blob A - correct request framing
A = b"\x00\x01\x00\x12\x02\x05\x00\xe0\x00\x00\x07\x00\x00\x00\x00"

# blob B - incorrect request framing (the SystemEvent)
B = bytes.fromhex(
    "00000000000001d10000000000000000000000170000"
    "9d6c00009d6d000000080000000200009d6e00000004"
    "70000e5200009d6f00000004"
    "0000054c00009d7000000003000500009d7100000009"
    "a2013b000294ff8800009d720000000400000000"
    "00009d730000000400" "00054c00009d74000000040000"
    "04b600009d75000000040000000000009d7600000003"
    "000900009d770000000300000000" "9d78000000031000"
    "00009d79000000031012" "00009d7a0000000308010000"
    "9d7b0000000300" "0e00000000"
)

from s7commplus.client import _parse_read_response

_parse_read_response(A)  # -> [b'\x02\x05\x00\xe0']
_parse_read_response(B)  # -> []   <-- the bug: silent, 195 bytes discarded

On read_symbolic (during a browse), this causes it to raise the RuntimeError.

[gijzelaerr]

Confirmed: this is a real bug, but I do not think _parse_read_response() is the right layer to fix it.

The sync transport already knows the frame is V254 in S7CommPlusConnection.send_request(), but currently returns its raw payload as though it were a normal application response. The async transport does not special-case V254 at all and strips the first ten bytes as a normal response header. By the time _parse_read_response() sees the bytes, the protocol-version information has been lost, and this particular payload happens to resemble an empty successful response.

The upstream driver also shows an important nuance: not every SystemEvent is fatal. Some are confirmations or unsolicited notifications that should be consumed while the caller continues waiting for the actual response. Fatal events should terminate the operation and surface useful diagnostics.

Proposed direction:

• Handle V254 at the transport/request-response boundary in both sync and async clients.
• Parse enough of SystemEvent to distinguish fatal data events from non-fatal confirmations/messages.
• Raise S7ProtocolError for fatal events, including decoded fields where available and the raw payload for diagnostics.
• Consume non-fatal SystemEvents and continue waiting for the matching normal response, with a bound to avoid an infinite stream.
• Cover ordinary and fragmented/reassembled receive paths.
• Add regression tests around the captured payload above, plus non-fatal and malformed events, for both sync and async clients.

I would avoid teaching _parse_read_response() to recognize this blob heuristically: it cannot reliably know that the bytes came from a V254 frame once send_request() has discarded that context.

The captured payload is a good fatal-event fixture. The next useful input would be a captured non-fatal V254 confirmation/message, if one is available, so we can lock down the consume-and-continue behavior as well.

[Poseidonas]

Your breakdown of where the version information gets lost was useful for orienting, so I spent some time tracing the paths against 8838aa9. Two things came out of it that I have not seen mentioned yet, and one of them changes what the user actually experiences.

A second path that never reaches the V254 check

The check at connection.py:804 only runs on the non-reassembled branch. When reassemble=True, send_request() returns at connection.py:784:

if reassemble:                          # 777
    data = self._recv_reassembled_payload()
    ...
    return bytes(data[10:])             # 784   <- returns before line 804

_recv_reassembled_payload() does not look at the version byte at all — it validates buf[0] != 0x72 and takes the length from buf[2:4], so buf[1] is never read. A V254 frame satisfies that shape as well as a V2 one, and the first ten bytes of the SystemEvent get stripped as though they were a response header.

Four call sites take that branch: explore(), get_cpu_state(), list_datablocks() and _explore_type_info_container(). The async client mirrors it at async_client.py:772, and its normal path at async_client.py:785 matches what you described.

This is not the path in bvanelli's report — read_symbolic() goes through GET_MULTI_VARIABLES without reassemble, so your reading of that one was right. It is a separate gap that would need covering too, since browse() calls into both.

The RuntimeError does not surface

This part I found more concerning than the silence itself. _read_typeinfo_rid() at client.py:605 catches it:

except S7ConnectionError:
    raise
except Exception:
    return 0

and browse() treats zero as a skip:

if ti_rid == 0:
    continue  # load-memory-only DB, skip     # client.py:553

So a DB whose symbolic read returned a SystemEvent is not reported as an error — it quietly drops out of the tag list. I confirmed the routing by raising RuntimeError("Symbolic read failed") from a stubbed read_symbolic and watching _read_typeinfo_rid() return 0. The except Exception is clearly deliberate for load-memory-only DBs, so this is more about the two cases being indistinguishable than about the handler being wrong.

On the payload as a fatal-event fixture

Decoding bvanelli's capture by hand against the upstream layout: ConfirmedBytes 465, then a struct of type 40300 with fifteen members 40301..40315. Member 40305 is an LInt holding 0xa2013b000294ff88, which is negative, so IsFatalError() in the upstream driver would classify it as fatal. That matches your read of it.

What I could not verify

I have no S7 hardware, so everything above is from the captured payload plus a stubbed _recv_s7_data. Specifically:

• Whether a real V254 frame carries the 0x72 <ver> 0x0000 trailer. If it does, the reassembly path swallows it as described. If it does not, ensure(4) would block instead. Both are wrong, but I cannot say which one happens on a real PLC.
• I have no non-fatal V254 capture either, so I cannot help with the consume-and-continue part you asked about.
• I did not decode what 0xa2013b000294ff88 means beyond its sign.

Offer

If it is useful, I am happy to open a PR for the deterministic part only: recognising V254 across all four transport paths, decoding SystemEvent enough to apply the upstream fatal test, raising S7ProtocolError with the decoded fields and the raw payload, and regression tests built on the captured blob. I would leave the non-fatal consume-and-continue behaviour to you, since writing it without a fixture would mean shipping something neither of us can test.

Entirely understand if you would rather keep this one in your own hands given it touches the transport on both clients — in that case the line references above may still save you a little tracing.