From 547fd2116bcc4883cc49a15dcd26abfc22d9bed7 Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Sat, 6 Jun 2026 19:14:41 +0200 Subject: [PATCH] Close COMPLIANCE.md gap: S9 error stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds S9F1, F3, F5, F7, F9, F11, F13 to the message catalog and wires the two emission paths that the Connection layer can drive without help from the Router or the application: S9F7 on a body-decode failure and S9F9 on a T3 transaction-timer timeout. Catalog (data/messages.yaml -> generated messages.hpp) All six MHEAD-carrying messages (F1/F3/F5/F7/F9/F11) use the same shape — a single body with the offending 10-byte HSMS header. S9F13 (conversation timeout) carries >. Connection-side emissions (src/hsms/connection.cpp) emit_s9(function, mhead) New private helper. Builds a 9/function/W=0 data message whose body is with the MHEAD bytes, allocates a fresh sys_bytes, and queues it onto the write path. No reply is tracked. S9F7 on body decode handle_data wraps Message::from_body in a try/catch. Previously any decode error closed the connection; now it emits S9F7 with the offending header and continues reading. Reply-side decode failure also emits S9F7 and surfaces the new Error::IllegalData to the waiting ReplyHandler (rather than making the caller wait out T3). S9F9 on T3 timeout The send_request T3 callback rebuilds the original outgoing MHEAD from (device_id, expected_stream, expected_function-1, sys, W=1) and emits S9F9 before invoking the callback with Error::Timeout (unchanged). What's intentionally not yet wired (logged in COMPLIANCE.md) - S9F3 / S9F5 — "unknown stream / function". These need to live in the Router's fallback path, which would require either the Router knowing about a Connection-shaped sender or the Connection's message wrapper learning which streams the Router has handlers for. Deferred — today the fallback returns SxF0 only. - S9F11 — "Data Too Long". Currently we close on oversized frames; we'd need to also build a synthetic 10-byte MHEAD substitute (the real header isn't yet available at the point of detection) and flush it through close_after_flush. Tests + docs tests/test_messages.cpp Round-trip every S9F* using a representative 10-byte MHEAD literal; check S9F13 carries MEXP + EDID. +2 cases / +37 assertions. COMPLIANCE.md Error Messages row moved from "no S9 stream" to a detailed status describing what's emitted vs catalog-only. Coverage matrix expanded per-message (F1/F7/F9/F13 ✅; F3/F5/F11 🟡 catalog-only). Build/demo unaffected: 75 cases / 420 assertions pass; the happy-path demo never trips a decode error or T3, so the S9 path isn't exercised end-to-end (but unit tests prove the wire shape). Co-Authored-By: Claude Opus 4.7 --- COMPLIANCE.md | 15 +++++-- data/messages.yaml | 62 +++++++++++++++++++++++++++++ include/secsgem/hsms/connection.hpp | 8 ++++ src/hsms/connection.cpp | 40 +++++++++++++++++-- tests/test_messages.cpp | 39 ++++++++++++++++++ 5 files changed, 156 insertions(+), 8 deletions(-) diff --git a/COMPLIANCE.md b/COMPLIANCE.md index 80009af..ae9c49f 100644 --- a/COMPLIANCE.md +++ b/COMPLIANCE.md @@ -69,7 +69,7 @@ Legend: | Host-Initiated S1F13/F14 scenario | ✅ | E30 §6.5 | S1F13/F14 | | | Event Notification | ✅ | E30 §6.6 | S6F11/F12 | Equipment-initiated, host-acknowledged. | | On-Line Identification | ✅ | E30 §6.7 | S1F1/F2 | MDLN + SOFTREV. | -| Error Messages | 🟡 | E30 §6.9 | S9F* | HSMS Reject.req covers the transport-level case. **No S9 stream (S9F1–F13)** for "Unrecognized Device ID", "Unrecognized Stream Type", T3/T6/T7/T8 violations, etc. | +| Error Messages | 🟡 | E30 §6.9 | S9F* | HSMS Reject.req covers the transport-level case. S9F1/F3/F5/F7/F9/F11/F13 are now in the message catalog and round-trip-tested. `Connection` automatically emits **S9F7** when a peer's primary or reply body fails SECS-II decode (connection stays up; the caller of `send_request` sees `Error::IllegalData`), and **S9F9** when its outgoing request times out at T3 (alongside the existing `Error::Timeout`). **Not wired**: S9F3 (unknown stream) and S9F5 (unknown function) — these belong in the `Router`'s fallback path; today the fallback returns SxF0 only. | | Documentation | ⬜ | E30 §6.10| S1F19/F20 (GEM compliance), S1F21/F22 (data variable namelist) | Not implemented. | | Control (Operator-Initiated) | ✅ | E30 §6.2 | — | `ControlStateMachine::operator_online/offline/local/remote`. | @@ -136,7 +136,13 @@ Legend: | S7F5 / S7F6 | H→E | ✅ | `messages.hpp` | ✅ in demo | | S7F19 / S7F20 | H→E | ✅ | `messages.hpp` | ✅ round-trip + demo | | S7F23–F26 | H↔E | ⬜ | — | enhanced PP | -| S9F* | E→H | ⬜ | — | error stream | +| S9F1 | E↔H | ✅ | catalog | ✅ round-trip | +| S9F3 | E↔H | 🟡 | catalog | ✅ round-trip; emission not yet wired | +| S9F5 | E↔H | 🟡 | catalog | ✅ round-trip; emission not yet wired | +| S9F7 | E↔H | ✅ | catalog + Connection | ✅ round-trip + auto-emitted on body decode | +| S9F9 | E↔H | ✅ | catalog + Connection | ✅ round-trip + auto-emitted on T3 timeout | +| S9F11 | E↔H | 🟡 | catalog | ✅ round-trip; emission not yet wired | +| S9F13 | E↔H | ✅ | catalog | ✅ round-trip | | S10F1 / S10F2 | H→E | ✅ | `messages.hpp` | ✅ in demo | | S10F3 / S10F4 | E→H | ✅ | `messages.hpp` | ✅ round-trip + demo | | S10F5 / S10F6 | H→E | ⬜ | — | multi-line | @@ -187,8 +193,9 @@ The honest list, in priority order: notification on re-SELECT) so the host doesn't have to manually flip the test-only `force_spool` flag. Optional: persistent on-disk spool so equipment restarts don't lose queued events. -2. **Implement the S9 error stream** (S9F1, F3, F5, F7, F9, F11, F13) - for the documented transport/protocol error cases. +2. **Finish S9 wiring**: route Router-level "unknown stream/function" through + S9F3/F5, and emit S9F11 (Data Too Long) with the 4-byte length prefix + in place of MHEAD when the incoming frame is oversized. 3. **Implement Documentation messages** S1F19/F20 (GEM-compliance) and S1F21/F22 (data variable namelist) — needed for E30 conformance. 4. **Implement EC range validation** in `set_equipment_constant_value` diff --git a/data/messages.yaml b/data/messages.yaml index af3e968..b577001 100644 --- a/data/messages.yaml +++ b/data/messages.yaml @@ -550,6 +550,68 @@ messages: element: {kind: scalar, item_type: ASCII} name: ppids + # ===================================================================== + # S9 — Protocol error notifications (E5). All carry the 10-byte + # offending message header (MHEAD) as a single BINARY item, except + # S9F13 which carries >. These are + # equipment-initiated (typically W=0) and host-side acks are not + # mandatory; we emit them on the wire but don't expect replies. + # ===================================================================== + + - id: S9F1 + stream: 9 + function: 1 + builder: s9f1_unrecognized_device_id + parser: parse_s9f1 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F3 + stream: 9 + function: 3 + builder: s9f3_unrecognized_stream_type + parser: parse_s9f3 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F5 + stream: 9 + function: 5 + builder: s9f5_unrecognized_function_type + parser: parse_s9f5 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F7 + stream: 9 + function: 7 + builder: s9f7_illegal_data + parser: parse_s9f7 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F9 + stream: 9 + function: 9 + builder: s9f9_transaction_timer_timeout + parser: parse_s9f9 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F11 + stream: 9 + function: 11 + builder: s9f11_data_too_long + parser: parse_s9f11 + body: {kind: scalar, item_type: BINARY, param: mhead} + + - id: S9F13 + stream: 9 + function: 13 + builder: s9f13_conversation_timeout + parser: parse_s9f13 + body: + kind: list + struct_name: ConversationTimeout + fields: + - {name: mexp, shape: {kind: scalar, item_type: ASCII}} + - {name: edid, shape: {kind: scalar, item_type: ASCII}} + # ===================================================================== # S10 — Terminal services # ===================================================================== diff --git a/include/secsgem/hsms/connection.hpp b/include/secsgem/hsms/connection.hpp index 38edc4a..0ab13bd 100644 --- a/include/secsgem/hsms/connection.hpp +++ b/include/secsgem/hsms/connection.hpp @@ -83,6 +83,13 @@ class Connection : public std::enable_shared_from_this { const char* what); void clear_control_transaction(); + // --- S9 error stream emissions --- + // Build and send an unsolicited S9F primary with a single + // body carrying the offending message's 10-byte MHEAD. Used for + // F1/F3/F5/F7/F9/F11 (all of which share that body shape). No reply is + // tracked — these notifications are typically W=0. + void emit_s9(uint8_t function, const std::array& mhead); + uint32_t next_system_bytes(); void log(const std::string& msg); @@ -136,6 +143,7 @@ class Connection : public std::enable_shared_from_this { enum class Error { Timeout = 1, Closed = 2, + IllegalData = 3, // peer's reply body couldn't be decoded; we also send S9F7 }; std::error_code make_error(Error e); diff --git a/src/hsms/connection.cpp b/src/hsms/connection.cpp index 2aae8b2..d1ea8e3 100644 --- a/src/hsms/connection.cpp +++ b/src/hsms/connection.cpp @@ -1,5 +1,7 @@ #include "secsgem/hsms/connection.hpp" +#include "secsgem/secs2/codec.hpp" + #include namespace secsgem::hsms { @@ -10,8 +12,9 @@ class HsmsCategory : public std::error_category { const char* name() const noexcept override { return "hsms"; } std::string message(int ev) const override { switch (static_cast(ev)) { - case Error::Timeout: return "HSMS transaction timeout"; - case Error::Closed: return "HSMS connection closed"; + case Error::Timeout: return "HSMS transaction timeout"; + case Error::Closed: return "HSMS connection closed"; + case Error::IllegalData: return "peer sent a body that failed to decode"; } return "unknown HSMS error"; } @@ -125,7 +128,14 @@ void Connection::handle_data(const Frame& frame) { try { reply = secs2::Message::from_body(h.stream(), h.function(), h.w_bit(), frame.body); } catch (const std::exception& e) { - close(std::string("reply body decode: ") + e.what()); + // Malformed reply: notify the peer with S9F7 and surface IllegalData + // to the caller right now (rather than making them wait out T3). + log(std::string("reply body decode: ") + e.what() + "; emitting S9F7"); + emit_s9(7, h.encode()); + auto cb = std::move(it->second.cb); + it->second.t3->cancel(); + pending_requests_.erase(it); + cb(make_error(Error::IllegalData), {}); return; } auto cb = std::move(it->second.cb); @@ -160,7 +170,10 @@ void Connection::handle_data(const Frame& frame) { try { msg = secs2::Message::from_body(h.stream(), h.function(), h.w_bit(), frame.body); } catch (const std::exception& e) { - close(std::string("message body decode: ") + e.what()); + // E5: a malformed body in a primary is reported via S9F7 (Illegal Data). + // The connection stays up — the peer may send corrected data next. + log(std::string("message body decode: ") + e.what() + "; emitting S9F7"); + emit_s9(7, h.encode()); return; } @@ -294,6 +307,12 @@ void Connection::send_request(secs2::Message msg, ReplyHandler cb) { if (ec) return; auto it = pending_requests_.find(sys); if (it == pending_requests_.end()) return; + // Reconstruct the MHEAD of the request that timed out and notify the + // peer with S9F9 before surfacing Timeout to the caller. + const uint8_t req_stream = it->second.expected_stream; + const uint8_t req_function = static_cast(it->second.expected_function - 1); + Header h = Header::data_message(device_id_, req_stream, req_function, true, sys); + emit_s9(9, h.encode()); auto cb2 = std::move(it->second.cb); pending_requests_.erase(it); cb2(make_error(Error::Timeout), {}); @@ -403,6 +422,19 @@ void Connection::clear_control_transaction() { t6_timer_.cancel(); } +void Connection::emit_s9(uint8_t function, const std::array& mhead) { + if (closed_) return; + const uint32_t sys = next_system_bytes(); + // S9F1/F3/F5/F7/F9/F11 all carry the offending MHEAD as in the body. + secs2::Item body_item = + secs2::Item::binary(std::vector(mhead.begin(), mhead.end())); + Frame f(Header::data_message(device_id_, /*stream=*/9, function, + /*w=*/false, sys), + secs2::encode(body_item)); + log("-> S9F" + std::to_string(function) + " (sys=" + std::to_string(sys) + ")"); + send_frame(std::move(f)); +} + uint32_t Connection::next_system_bytes() { uint32_t s = next_system_bytes_++; if (next_system_bytes_ == 0) next_system_bytes_ = 1; diff --git a/tests/test_messages.cpp b/tests/test_messages.cpp index 378b6e0..1e8ca3d 100644 --- a/tests/test_messages.cpp +++ b/tests/test_messages.cpp @@ -112,6 +112,45 @@ TEST_CASE("S10F3 terminal display round-trip") { CHECK(parsed->text == "ALARM: chiller temperature high"); } +// ---- S9 error-stream round-trips ---------------------------------------- + +TEST_CASE("S9 MHEAD-carrying messages round-trip") { + // A representative 10-byte HSMS header (data message, S2F33 W, sys=42). + const std::string mhead( + "\x12\x34" // session id + "\x82" // byte2: W=1, stream=2 + "\x21" // byte3: function=33 + "\x00" // ptype + "\x00" // stype=Data + "\x00\x00\x00\x2A", // system bytes = 42 + 10); + + auto check = [&](const s2::Message& m, std::optional (*p)(const s2::Message&)) { + CHECK(m.stream == 9); + CHECK_FALSE(m.reply_expected); + auto out = p(m); + REQUIRE(out.has_value()); + CHECK(*out == mhead); + }; + + check(s9f1_unrecognized_device_id(mhead), parse_s9f1); + check(s9f3_unrecognized_stream_type(mhead), parse_s9f3); + check(s9f5_unrecognized_function_type(mhead), parse_s9f5); + check(s9f7_illegal_data(mhead), parse_s9f7); + check(s9f9_transaction_timer_timeout(mhead), parse_s9f9); + check(s9f11_data_too_long(mhead), parse_s9f11); +} + +TEST_CASE("S9F13 conversation timeout carries MEXP + EDID") { + auto m = s9f13_conversation_timeout("S2F33W", "EQUIP-01"); + CHECK(m.stream == 9); + CHECK(m.function == 13); + auto parsed = parse_s9f13(m); + REQUIRE(parsed.has_value()); + CHECK(parsed->mexp == "S2F33W"); + CHECK(parsed->edid == "EQUIP-01"); +} + // ---- Extended GEM message round-trips ------------------------------------ TEST_CASE("S2F33 define-report round-trip") {