Fix rigctld extended response parsing

[worldwidedx]

Summary

• Drop the echoed extended-command header as well as the final RPRT status line.
• Update the rigctld test daemon to reproduce Hamlib's extended-response framing.

Verification

• pytest tests/radio/test_rigctld_client.py -q — 29 passed
• Verified against local Hamlib 4.7.2 rigctld controlling an Elecraft K4 (model 2047): frequency, mode, and PTT reads succeeded.

This fixes get-command parsing when Hamlib returns the command header before response fields.

[bucknova]

Welcome, and thanks — this is a well-aimed first contribution. Testing against
a real Hamlib 4.7.2 driving a K4 and reporting the version and model is exactly
the evidence that makes a protocol fix reviewable, and you may well have found
the root cause of a bug we mis-diagnosed months ago (more on that below).

One change to ask for before it goes in, and one question.

The ask: make the header optional rather than mandatory

lines[1:-1] drops the first line unconditionally, which doesn't widen what we
accept — it moves which daemons work. We stood up a socket-level fake and
cross-tested both client versions against both daemon behaviours:

main   lines[:-1]   vs daemon echo_header=False  ->  freq=14230000  OK
main   lines[:-1]   vs daemon echo_header=True   ->  RigCommandError: could not
                                                     parse frequency from 'get_freq:'
pr53   lines[1:-1]  vs daemon echo_header=False  ->  RigCommandError: empty
                                                     frequency response
pr53   lines[1:-1]  vs daemon echo_header=True   ->  freq=14230000  OK

They're mutually exclusive. Since ping() calls get_freq(), whichever
assumption is wrong means the rig won't connect at all for those users — the
failure is total, not cosmetic. We'd rather not bet the whole rigctld path on
every Hamlib version and backend in the wild behaving like the one we can test.

Handling both is a couple of lines, and can't regress anyone either way:

body = lines[:-1]  # drop the RPRT terminator
# Extended-response mode echoes the command name on its own line before
# the fields ("get_freq:").  Drop it only when it's actually present: a
# header has nothing after the colon, while a data line is "Field: value".
if body and body[0].endswith(":"):
    body = body[1:]
return body

A regression test for each shape — one response with the header, one without,
both yielding the same parsed frequency — would lock the tolerance in.

Why we're asking, rather than just trusting the tests

Not a criticism of the patch, but worth naming: this PR changes the client and
the test double together, so the 29 green tests only confirm the two halves
agree with each other. Run either client against the other's fake and it fails.
The suite would look equally green if the change were backwards, which means it
can't tell us which behaviour real Hamlib has — your hardware run is the only
evidence in play, and it covers one version and one backend.

The question

Could you paste the raw bytes your daemon returns? Something like:

echo "+f" | nc localhost 4532

Two things we'd like to pin down:

1. Whether the echo is the long or short command name. We believe Hamlib
   emits get_freq: / get_level:, but the fake in this PR emits f: and
   l STRENGTH:. That's invisible today because the line is dropped blindly,
   but the PR describes the fake as reproducing Hamlib's framing, and with the
   short name it doesn't. If we ever validate the header, the fake would pass
   while real hardware failed.
2. Whether the header appears on set commands and errors too (+F, and an
   RPRT -1 path), which would tell us if it's truly unconditional.

Happy to make the fake faithful ourselves once we know the real shape.

Credit where it's due

We think this may be the actual root cause of a report we fixed the wrong way
in #33. A FlexRadio user on rigctld reported "Test rigctld Connection does
nothing" and "Connect Rig immediately shows Connection lost" — the exact
symptom an echoed header produces, since the parse failure surfaces as a failed
connection. We attributed it to float-formatted frequencies and added numeric
tolerance. Our own commit message admits we "reproduced against a fake daemon",
i.e. we reproduced our hypothesis rather than their daemon's real output. If
you're right, that fix turned a silent crash into a tidy error message without
ever making rigctld work, and that user was never actually fixed.

So this is likely a more important patch than its one-line diff suggests.

Housekeeping

CI hasn't run yet — first-time contributions from a fork need a maintainer to
approve the workflow, which we'll do. Locally the branch is clean: ruff passes
and all 185 tests in tests/radio pass.

Make the header optional and we'll get this merged.

73, Kevin/W0AEZ and Claude

[worldwidedx]

I captured these directly from my local Hamlib 4.7.2 rigctld on127.0.0.1:4532 controlling an Elecraft K4 (model 2047): +f b'get_freq:\nFrequency: 14260000\nRPRT 0\n' get_freq: Frequency: 14260000 RPRT 0 +l STRENGTH b'get_level: STRENGTH\n-24\nRPRT 0\n' get_level: STRENGTH -24 RPRT 0 +F not-a-frequency b'set_freq: not-a-frequency\nRPRT -1\n' set_freq: not-a-frequency RPRT -1 Also, Updated in 1dfbc11 (and pushed to this PR). The parser now accepts either framing: it always removes the final RPRT line, then removes an echoed first line only when it matches the long Hamlib command name for the command sent. This handles argument-bearing headers such as `get_level: STRENGTH`, which a simple `endswith(":")` check would miss. I updated the fake to make echoed headers optional and to use Hamlib long names, and added coverage for: - frequency responses with and without an echoed header; - the `get_level: STRENGTH` header shape. Validation: `pytest tests/radio -q` → 182 passed, 6 skipped. Raw responses captured from local Hamlib 4.7.2 rigctld at 127.0.0.1:4532 controlling an Elecraft K4 (model 2047): ```text +f b'get_freq:\nFrequency: 14230000\nRPRT 0\n' +l STRENGTH b'get_level: STRENGTH\n-6\nRPRT 0\n' +F not-a-frequency b'set_freq: not-a-frequency\nRPRT -1\n' Tim AE6LX

…

On Mon, Aug 31, 2026 at 9:41 AM W0AEZ ***@***.***> wrote: *bucknova* left a comment (bucknova/Open-SSTV#53) <#53 (comment)> Welcome, and thanks — this is a well-aimed first contribution. Testing against a real Hamlib 4.7.2 driving a K4 and reporting the version and model is exactly the evidence that makes a protocol fix reviewable, and you may well have found the root cause of a bug we mis-diagnosed months ago (more on that below). One change to ask for before it goes in, and one question. The ask: make the header optional rather than mandatory lines[1:-1] drops the first line unconditionally, which doesn't widen what we accept — it moves which daemons work. We stood up a socket-level fake and cross-tested both client versions against both daemon behaviours: main lines[:-1] vs daemon echo_header=False -> freq=14230000 OK main lines[:-1] vs daemon echo_header=True -> RigCommandError: could not parse frequency from 'get_freq:' pr53 lines[1:-1] vs daemon echo_header=False -> RigCommandError: empty frequency response pr53 lines[1:-1] vs daemon echo_header=True -> freq=14230000 OK They're mutually exclusive. Since ping() calls get_freq(), whichever assumption is wrong means the rig won't connect at all for those users — the failure is total, not cosmetic. We'd rather not bet the whole rigctld path on every Hamlib version and backend in the wild behaving like the one we can test. Handling both is a couple of lines, and can't regress anyone either way: body = lines[:-1] # drop the RPRT terminator# Extended-response mode echoes the command name on its own line before# the fields ("get_freq:"). Drop it only when it's actually present: a# header has nothing after the colon, while a data line is "Field: value".if body and body[0].endswith(":"): body = body[1:]return body A regression test for each shape — one response with the header, one without, both yielding the same parsed frequency — would lock the tolerance in. Why we're asking, rather than just trusting the tests Not a criticism of the patch, but worth naming: this PR changes the client and the test double together, so the 29 green tests only confirm the two halves agree with each other. Run either client against the other's fake and it fails. The suite would look equally green if the change were backwards, which means it can't tell us which behaviour real Hamlib has — your hardware run is the only evidence in play, and it covers one version and one backend. The question Could you paste the raw bytes your daemon returns? Something like: echo "+f" | nc localhost 4532 Two things we'd like to pin down: 1. *Whether the echo is the long or short command name.* We believe Hamlib emits get_freq: / get_level:, but the fake in this PR emits f: and l STRENGTH:. That's invisible today because the line is dropped blindly, but the PR describes the fake as reproducing Hamlib's framing, and with the short name it doesn't. If we ever validate the header, the fake would pass while real hardware failed. 2. *Whether the header appears on set commands and errors too* (+F, and an RPRT -1 path), which would tell us if it's truly unconditional. Happy to make the fake faithful ourselves once we know the real shape. Credit where it's due We think this may be the actual root cause of a report we fixed the wrong way in #33 <#33>. A FlexRadio user on rigctld reported "Test rigctld Connection does nothing" and "Connect Rig immediately shows Connection lost" — the exact symptom an echoed header produces, since the parse failure surfaces as a failed connection. We attributed it to float-formatted frequencies and added numeric tolerance. Our own commit message admits we "reproduced against a fake daemon", i.e. we reproduced our hypothesis rather than their daemon's real output. If you're right, that fix turned a silent crash into a tidy error message without ever making rigctld work, and that user was never actually fixed. So this is likely a more important patch than its one-line diff suggests. Housekeeping CI hasn't run yet — first-time contributions from a fork need a maintainer to approve the workflow, which we'll do. Locally the branch is clean: ruff passes and all 185 tests in tests/radio pass. Make the header optional and we'll get this merged. 73, Kevin/W0AEZ and Claude — Reply to this email directly, view it on GitHub <#53?email_source=notifications&email_token=CJEXG2RTP3G4MXZ57UNXUKD5MWTB3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBYGE2DGMRXGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5481432716>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/CJEXG2Q2CVID2MWBX3HMBD35MWTB3AVCNFSNUABGKJSXA33TNF2G64TZHMYTEMBWGIZDIMRVGI5US43TOVSTWNJSHAZTAMZQGE3DFILWAI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/CJEXG2SXBOLLTLVN3ESONJT5MWTB3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBYGE2DGMRXGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/CJEXG2X2XDHX54WDL22UMGD5MWTB3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBYGE2DGMRXGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Owner, worldwidedx.com AE6LX, Amateur Radio

[bucknova]

Thank you for the captures — and you were right to not just take our
suggested one-liner.

Our proposed fix was wrong, and your data is what shows it. We suggested
dropping the first line when it endswith(":"). Against your own bytes:

'get_level: STRENGTH'.endswith(":")  ->  False

so the header would have survived, _parse_value would have handed back
'STRENGTH', and get_strength() would raise on every call — trading the
old bug for a new one on the level path. Checking the long command name
instead is the correct discriminator, and the argument-bearing header is
exactly the case that rules out the cheap test. Good catch.

The captures also settle both open questions: the echo is the long name
(get_freq:, get_level:, set_freq:), not the short command, and it does
appear on set commands and on RPRT -1 error paths. So the fake's original
f: / l STRENGTH: framing was wrong in a way that would have hidden this.

Verified on our side

We replayed your exact captured bytes through the new parser, plus the same
responses with the header stripped, to confirm it really does accept both
framings rather than having moved which daemons work:

WITH echo (your captures)   freq=14260000  mode=('USB', 2700)  ptt=False  strength=-24
WITHOUT echo                freq=14260000  mode=('USB', 2700)  ptt=False  strength=-24

Both shapes, all four getters, same values. That is the property we were
asking for.

Also clean: ruff, and 188 passed in tests/radio. The fake taking an
echo_header switch and using long names is the right shape — it can now
model a daemon we don't have, which is what a test double is for.

One thing to be aware of, not a blocker

_LONG_COMMAND_NAMES is a fixed table, so a future command added to the
client without a matching entry silently falls back to "no header" and would
mis-parse against an echoing daemon. Today it covers everything we send
(f/F/m/M/t/T/l). If you want a belt-and-braces guard, a KeyError-style
assertion in the client's own command sites — or a test asserting every
command string the client sends has an entry — would make that failure loud
instead of quiet. Entirely optional; we can add it after merge.

What this fixes beyond the PR

We think this is the real cause of a bug we mis-diagnosed in #33. A
FlexRadio-on-rigctld user reported "Test rigctld Connection does nothing"
and "Connect Rig immediately shows Connection lost" — precisely what an
unparsed get_freq: header produces, since ping() calls get_freq() and
the failure surfaces as a failed connection. We blamed float-formatted
frequencies and added numeric tolerance, having "reproduced" it against our
own fake — i.e. against our hypothesis rather than their daemon. That likely
turned a silent crash into a tidy error message without ever making rigctld
work.

So this is a considerably more important patch than a one-line diff suggests,
and it came from a first-time contributor with a real radio and a packet
capture. Much appreciated.

CI is approved and running on 1dfbc11. Assuming it comes back green we'll
merge as-is.

73, Kevin/W0AEZ and Claude