[tylerkron]

CompositeMessageParser carried a fourth classification heuristic (IsLikelyProtobufData) that could never change what the parser does. It only ever turned an Uncertain hint into LikelyProtobuf, and ParseMessages treats those two hints identically: anything that is not LikelyText tries the protobuf parser first and falls back to text. So the heuristic was dead weight that a reader had to understand before realising it was inert.

This PR deletes heuristic 4 and the IsLikelyProtobufData helper. Heuristics 1-3 (printable ratio, null-byte ratio, leading SCPI marker) are unchanged.

Why this is safe

• DetectMessageType and IsLikelyProtobufData are private and pure; the hint is consumed in exactly one place (ParseMessages), which only branches on == LikelyText.
• Heuristic 4 ran after every path that can return LikelyText, so it could only re-label Uncertain as LikelyProtobuf — both take the protobuf-first branch. No input can route differently.
• CompositeMessageParserBenchCaptureTests (Nq1 fw 3.7.2 status reply, 1- and 4-channel stream frames, SCPI replies, echoed command ahead of a status frame) and the rest of the suite pass unchanged: Core.Tests 4356 passed / 3 skipped on net9.0 and net10.0 (Release).

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Remove inert protobuf heuristic from CompositeMessageParser

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Remove a protobuf classification heuristic that could not change parser order.
• Preserve printable-ratio, null-byte, and SCPI-marker checks and their existing fallback behavior.

Diagram

graph TD
  Input["Input bytes"] --> Classify["Existing checks"] --> Hint{"Likely text?"}
  Hint -->|yes| Text["Text first"] -->|parsed| Result["Parsed messages"]
  Hint -->|no| Protobuf["Protobuf first"] -->|parsed| Result
  Text -->|empty| Fallback["Other parser"] --> Result
  Protobuf -->|empty| Fallback

Loading

High-Level Assessment

Removing the inert check is the narrowest approach. Changing the hint enum or parser-order logic would add risk without addressing additional behavior.

Files changed (1) +0 / -32

Refactor (1) +0 / -32

CompositeMessageParser.csRemove unused protobuf classification check +0/-32

Remove unused protobuf classification check

• Deletes the fourth detection heuristic and its private IsLikelyProtobufData helper. Inputs previously relabeled from Uncertain to LikelyProtobuf still take the same protobuf-first path.

src/Daqifi.Core/Communication/Consumers/CompositeMessageParser.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Bench test on a real Nq1 (USB, fw 3.8.0, hw 2.0.0): no behavior change.

I ran the same scenarios against origin/main (4adf62c) and against this PR merged locally onto that main. All 4365 Core + 342 MCP unit tests pass on net9/net10. Nothing was pushed, because the PR merges cleanly and the queue will retest.

Note: CompositeMessageParser has no production call site. Core's device stack, the MCP server, the example app, desktop and avalonia don't use it. So I also ran it directly on the raw serial stream (StreamMessageConsumer<object>), recording every read chunk.

Raw serial → CompositeMessageParser (ch0, *IDN?/SYSTem:ERRor?/SYSInfoPB? before, during and after streaming)

	main 100 Hz	PR 100 Hz	main 1 kHz	PR 1 kHz
stream frames / 30.6 s	3059 (99.9%)	3059 (99.9%)	30604 (100%)	30607 (100%)
status frames (incl. mid-stream)	2	2	2	2
text replies parsed	5	5	4	4
dup timestamps / consumer errors	0 / 0	0 / 0	0 / 0	0 / 0
hint tally	3058 Uncertain, 5 Text, 2 Pb	3057 / 5 / 2	26529 / 4 / 3	26378 / 4 / 2
heuristic 4 fired (main)	0	–	0	–

Deterministic replay: I replayed each of the 4 captures through both builds. Both builds produce the identical output hash (message sequence and consumed bytes), and replay reproduces the live run exactly. Heuristic 4 never fired on live traffic, even on main.

Full DaqifiStreamingDevice stack (connect, error drain, stream 30 s with a mid-stream SYSInfoPB?, stop, disconnect): 100 Hz 3000 vs 2999 frames, 1 kHz 30008 vs 30008 frames. Every run had 0 discarded frames, 0 decode failures, 0 gaps, 0 ErrorOccurred, a status frame received mid-stream while frames kept decoding, an empty error queue and a clean disconnect. The changes merged on main (#813, #815) behaved normally.

Pre-existing, not caused by this PR, and identical on main: when a text reply arrives mid-stream in the same read as a 10-byte stream frame, the buffer is Uncertain. The protobuf parser wins and the text gets skipped. At 1 kHz both mid-stream text replies were lost on both builds. Only direct users of CompositeMessageParser are affected.

LGTM from the bench.