diff --git a/radio_io/radio_io.c b/radio_io/radio_io.c index f88150c2..21c2774c 100644 --- a/radio_io/radio_io.c +++ b/radio_io/radio_io.c @@ -34,6 +34,7 @@ #endif #include "radio_io.h" +#include "radio_port.h" #include "../common/hermes_log.h" #define RADIO_LOG_TAG "radio-io" @@ -45,8 +46,35 @@ #endif #ifdef HAVE_HAMLIB -/* Configure the serial device path and speed through Hamlib's stable public - * conf API (rig_token_lookup + rig_set_conf) rather than writing the internal +/* Which sort of port does this rig speak? Hamlib knows; the user should not + * have to guess. See radio_port.h for why this matters (issue #179). + * + * Read through rig_get_caps_int() rather than rig->caps->port_type for the same + * reason the conf tokens are preferred below: it is an accessor keyed on the + * model number, so it does not depend on the layout of a struct that Hamlib 5 + * is progressively closing off. */ +static radio_port_kind_t radio_io_port_kind(rig_model_t model) +{ + switch ((rig_port_t)rig_get_caps_int(model, RIG_CAPS_PORT_TYPE)) + { + case RIG_PORT_SERIAL: + return RADIO_PORT_KIND_SERIAL; + case RIG_PORT_NETWORK: + case RIG_PORT_UDP_NETWORK: + return RADIO_PORT_KIND_NETWORK; + default: + return RADIO_PORT_KIND_OTHER; + } +} + +static const char *radio_io_model_name(rig_model_t model) +{ + const char *name = rig_get_caps_cptr(model, RIG_CAPS_MODEL_NAME_CPTR); + return name ? name : "this rig"; +} + +/* Configure the device path and speed through Hamlib's stable public conf API + * (rig_token_lookup + rig_set_conf) rather than writing the internal * rig->state.rigport struct directly. * * Since Hamlib 4.6 the port struct is reached via the HAMLIB_RIGPORT macro, @@ -55,7 +83,8 @@ * rig_data_pointer undefined at link time), so depending on it is fragile. The * conf tokens "rig_pathname" and "serial_speed" are stable across 4.6/4.7/5.x * and must be set before rig_open(). */ -static void radio_io_apply_serial_conf(RIG *radio, const char *device_path, +static void radio_io_apply_serial_conf(RIG *radio, rig_model_t model, + const char *device_path, int serial_speed) { int rc; @@ -70,6 +99,18 @@ static void radio_io_apply_serial_conf(RIG *radio, const char *device_path, if (serial_speed > 0) { + /* Only serial rigs have a baud rate. Pushing serial_speed at a network + * rig makes Hamlib reject a token it does not own, and the resulting + * "rig_set_conf(serial_speed) failed: -1" reads like a fault when it is + * really just a setting that does not apply here. */ + if (radio_io_port_kind(model) != RADIO_PORT_KIND_SERIAL) + { + HLOGI(RADIO_LOG_TAG, + "radio_serial_speed=%d ignored: %s is not a serial rig", + serial_speed, radio_io_model_name(model)); + return; + } + char rate[16]; snprintf(rate, sizeof(rate), "%d", serial_speed); rc = rig_set_conf(radio, rig_token_lookup(radio, "serial_speed"), rate); @@ -80,6 +121,19 @@ static void radio_io_apply_serial_conf(RIG *radio, const char *device_path, serial_speed); } } + +/* Say plainly when radio_device does not match the kind of port the rig uses. + * Advisory only -- Hamlib still gets to try, because a shape heuristic must + * never veto a configuration that would have worked. */ +static void radio_io_warn_port_mismatch(rig_model_t model, const char *device_path) +{ + radio_port_verdict_t v = radio_port_check(radio_io_port_kind(model), device_path); + const char *advice = radio_port_advice(v); + + if (advice) + HLOGW(RADIO_LOG_TAG, "radio_device='%s' looks wrong for %s -- %s", + device_path, radio_io_model_name(model), advice); +} #endif /* Global mutex — protects radio state (g_radio_type, radio pointer, @@ -186,15 +240,30 @@ int radio_io_init(int radio_type, const char *device_path, int hamlib_log_level, return -1; } - radio_io_apply_serial_conf(radio, device_path, serial_speed); + radio_io_apply_serial_conf(radio, radio_type, device_path, serial_speed); + radio_io_warn_port_mismatch(radio_type, device_path); HLOGD(RADIO_LOG_TAG, "Calling rig_open(device=%s)", device_path && device_path[0] ? device_path : "(default)"); int ret = rig_open(radio); if (ret != RIG_OK) { - HLOGE(RADIO_LOG_TAG, "rig_open: error = %s %s", - device_path ? device_path : "(default)", rigerror(ret)); + /* rigerror() returns Hamlib's debug ring buffer appended to the message, + * so at any log level above 0 the actual reason is buried in backend + * chatter. rigerror2() is the plain string; keep the numeric code too, + * since that is what is greppable in a bug report. */ + HLOGE(RADIO_LOG_TAG, "rig_open(%s) failed: %s (%d)", + device_path && device_path[0] ? device_path : "(default)", + rigerror2(ret), ret); + + /* Repeat the advice next to the failure: the pre-flight warning above + * is easily lost in Hamlib's own debug output, and this line is the one + * the user will paste into a bug report. */ + const char *advice = + radio_port_advice(radio_port_check(radio_io_port_kind(radio_type), + device_path)); + if (advice) + HLOGE(RADIO_LOG_TAG, "%s", advice); rig_cleanup(radio); radio = NULL; g_radio_type = RADIO_TYPE_NONE; @@ -229,17 +298,8 @@ int radio_io_init(int radio_type, const char *device_path, int hamlib_log_level, * member directly. Prefer the accessor so this survives the struct being * hidden. The hamlib version string goes on the same line because the field * only means anything once you know which hamlib produced it. */ - { -#if defined(HAMLIB_STATE) - const struct rig_state *rs = HAMLIB_STATE(radio); -#elif defined(STATE) - const struct rig_state *rs = STATE(radio); -#else - const struct rig_state *rs = &radio->state; -#endif - HLOGI(RADIO_LOG_TAG, "hamlib runtime: %s", - hamlib_version2 ? hamlib_version2 : "version unknown"); - } + HLOGI(RADIO_LOG_TAG, "hamlib runtime: %s", + hamlib_version2 ? hamlib_version2 : "version unknown"); pthread_mutex_unlock(&g_radio_mutex); return 0; #else diff --git a/radio_io/radio_port.h b/radio_io/radio_port.h new file mode 100644 index 00000000..9bfbded5 --- /dev/null +++ b/radio_io/radio_port.h @@ -0,0 +1,119 @@ +/* Does `radio_device` match the kind of port the chosen rig actually speaks? + * + * Copyright (C) 2026 Rhizomatica + * + * This is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3, or (at your option) + * any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * --------------------------------------------------------------------------- + * + * Not every rig Hamlib supports is reached over a serial cable. A FlexRadio + * (SmartSDR, model 23005), rigctld (2), FLRig (4) and friends are network rigs: + * their `rig_pathname` is an address, and Hamlib appends its own default port + * if none is given. Point one of those at a COM port and Hamlib dutifully + * builds "COM4:4992", tries to resolve it as a host, and fails with the rather + * unhelpful "Invalid parameter" -- reported as issue #179. + * + * The rig itself knows which it is (`caps->port_type`), so we can say so before + * the user has to guess. These checks only ever produce a diagnostic: the + * device string is matched by shape, and a heuristic must never be allowed to + * refuse a configuration that would in fact have worked. Hamlib still gets to + * make the final call. + * + * Kept free of Hamlib types so the shapes can be unit-tested on any host, + * including builds without Hamlib at all. + */ + +#ifndef RADIO_PORT_H +#define RADIO_PORT_H + +#include + +typedef enum +{ + RADIO_PORT_KIND_OTHER = 0, /* dummy, USB, parallel, ... -- no opinion */ + RADIO_PORT_KIND_SERIAL, + RADIO_PORT_KIND_NETWORK, +} radio_port_kind_t; + +typedef enum +{ + RADIO_PORT_OK = 0, + RADIO_PORT_WANTS_NETWORK, /* network rig, but radio_device names a tty */ + RADIO_PORT_WANTS_SERIAL, /* serial rig, but radio_device looks like an + address */ +} radio_port_verdict_t; + +/* "COM4", "com12", "\\.\COM12", "/dev/ttyUSB0", "/dev/cu.usbserial-1234" */ +static inline int radio_port_path_is_serial(const char *path) +{ + if (!path || !path[0]) + return 0; + + if (path[0] == '/') /* /dev/... (POSIX) */ + return 1; + + if (path[0] == '\\') /* \\.\COMnn (Windows, >COM9) */ + return 1; + + if ((path[0] == 'C' || path[0] == 'c') && + (path[1] == 'O' || path[1] == 'o') && + (path[2] == 'M' || path[2] == 'm') && + path[3] >= '0' && path[3] <= '9') + { + for (const char *p = path + 4; *p; p++) + if (*p < '0' || *p > '9') + return 0; /* "COM4x" -- not a COM port */ + return 1; + } + + return 0; +} + +/* Judge a configured device string against the rig's port kind. An empty + * device is always OK: Hamlib has its own defaults and the user may well be + * relying on them. */ +static inline radio_port_verdict_t radio_port_check(radio_port_kind_t kind, + const char *path) +{ + if (!path || !path[0]) + return RADIO_PORT_OK; + + int looks_serial = radio_port_path_is_serial(path); + + if (kind == RADIO_PORT_KIND_NETWORK && looks_serial) + return RADIO_PORT_WANTS_NETWORK; + + if (kind == RADIO_PORT_KIND_SERIAL && !looks_serial) + return RADIO_PORT_WANTS_SERIAL; + + return RADIO_PORT_OK; +} + +/* One line the user can act on, or NULL when there is nothing to say. */ +static inline const char *radio_port_advice(radio_port_verdict_t v) +{ + switch (v) + { + case RADIO_PORT_WANTS_NETWORK: + return "this rig is reached over the NETWORK, not a serial port: set " + "radio_device to the radio's address (e.g. 192.168.1.50, or " + "192.168.1.50:4992 to override the port). radio_serial_speed " + "does not apply."; + case RADIO_PORT_WANTS_SERIAL: + return "this rig is reached over a SERIAL port: set radio_device to a " + "port name (e.g. COM4 on Windows, /dev/ttyUSB0 on Linux)."; + case RADIO_PORT_OK: + default: + return NULL; + } +} + +#endif /* RADIO_PORT_H */ diff --git a/radio_io/rigctl_parse.c b/radio_io/rigctl_parse.c index 3ba2ff03..2400004c 100644 --- a/radio_io/rigctl_parse.c +++ b/radio_io/rigctl_parse.c @@ -48,6 +48,7 @@ struct mod_entry char version[32]; char status[32]; char macro_name[32]; + char port[8]; }; static struct mod_entry *g_models = NULL; @@ -77,6 +78,20 @@ static int collect_model(const struct rig_caps *caps, void *data) snprintf(e->macro_name, sizeof(e->macro_name), "%s", caps->macro_name ? caps->macro_name : ""); snprintf(e->status, sizeof(e->status), "%s", rig_strstatus(caps->status)); + /* Which port this rig speaks is the one fact you need before setting + * radio_device, and it was previously invisible here (issue #179: a + * FlexRadio was configured with a COM port because the listing gave no + * hint it is a network rig). */ + switch (caps->port_type) + { + case RIG_PORT_SERIAL: snprintf(e->port, sizeof(e->port), "serial"); break; + case RIG_PORT_NETWORK: + case RIG_PORT_UDP_NETWORK: snprintf(e->port, sizeof(e->port), "net"); break; + case RIG_PORT_USB: snprintf(e->port, sizeof(e->port), "usb"); break; + case RIG_PORT_NONE: snprintf(e->port, sizeof(e->port), "-"); break; + default: snprintf(e->port, sizeof(e->port), "other"); break; + } + return 1; } @@ -184,7 +199,7 @@ void list_models(void) close(saved_stderr); #endif - printf(" Rig # Mfg Model Version Status Macro\n"); + printf(" Rig # Mfg Model Version Status Port Macro\n"); status = rig_list_foreach(collect_model, NULL); if (status != RIG_OK) @@ -198,12 +213,13 @@ void list_models(void) for (int i = 0; i < g_count; i++) { struct mod_entry *e = &g_models[i]; - printf("%6d %-23s%-24s%-16s%-12s%s\n", + printf("%6d %-23s%-24s%-16s%-12s%-8s%s\n", e->id, e->mfg_name, e->model_name, e->version, e->status, + e->port, e->macro_name); } diff --git a/tests/Makefile b/tests/Makefile index 7c63a6e2..b9aa2086 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -46,7 +46,7 @@ TEST_BINS = test_ring_buffer test_arq_protocol test_arq_timing test_arq_fsm \ test_tcp_interfaces test_resampler test_pcm24 test_arq_olla test_arq_sim \ test_arq_conn test_cfg_utils test_arq_tnc test_channel_busy \ test_freedv_harq test_sock_wire test_virtual_clock test_watterson \ - test_ofdm_acq test_ui_status test_tx_pacing + test_ofdm_acq test_ui_status test_tx_pacing test_radio_port .PHONY: all test clean @@ -174,6 +174,11 @@ test_ofdm_acq: modem/test_ofdm_acq.c $(UNITY_SRC) $(FREEDV_LIB) test_tx_pacing: modem/test_tx_pacing.c $(UNITY_SRC) ../modem/tx_pacing.h $(CC) $(CFLAGS) -I../modem -o $@ modem/test_tx_pacing.c $(UNITY_SRC) $(LDFLAGS) +# radio_device vs the rig's port type (pure header; no hamlib needed). +# radio_port.h is a PREREQUISITE -- the logic under test lives in the header. +test_radio_port: radio_io/test_radio_port.c $(UNITY_SRC) ../radio_io/radio_port.h + $(CC) $(CFLAGS) -I../radio_io -o $@ radio_io/test_radio_port.c $(UNITY_SRC) $(LDFLAGS) + # UI status wire format (embedded struct vs remote JSON must not drift) test_ui_status: gui_interface/test_ui_status.c $(UNITY_SRC) ../gui_interface/ui_status.c $(CC) $(CFLAGS) -I../gui_interface -I../datalink_arq -I../modem -I../common -o $@ $^ $(LDFLAGS) diff --git a/tests/radio_io/test_radio_port.c b/tests/radio_io/test_radio_port.c new file mode 100644 index 00000000..036d811d --- /dev/null +++ b/tests/radio_io/test_radio_port.c @@ -0,0 +1,139 @@ +/* radio_device vs the rig's actual port type (issue #179). + * + * Copyright (C) 2026 Rhizomatica + * + * This is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3, or (at your option) + * any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * --------------------------------------------------------------------------- + * + * A FlexRadio (SmartSDR, model 23005) is a network rig. Configured with + * radio_device = COM4, Hamlib built "COM4:4992", failed to resolve it, and + * reported "Invalid parameter" -- which sent the reporter looking at baud rates + * and CAT ports rather than at the address he actually needed. + * + * These are shape checks on user-supplied strings, and their real risk is the + * false positive: a heuristic that cries wrong about a working configuration is + * worse than no heuristic. Hence the emphasis below on what must stay silent. + */ + +#include "unity.h" +#include "radio_port.h" + +/* --- the reported case ------------------------------------------------- */ + +void test_network_rig_with_a_com_port_is_flagged(void) +{ + /* Exactly issue #179. */ + TEST_ASSERT_EQUAL(RADIO_PORT_WANTS_NETWORK, + radio_port_check(RADIO_PORT_KIND_NETWORK, "COM4")); + TEST_ASSERT_NOT_NULL(radio_port_advice( + radio_port_check(RADIO_PORT_KIND_NETWORK, "COM4"))); +} + +void test_network_rig_with_a_tty_is_flagged(void) +{ + TEST_ASSERT_EQUAL(RADIO_PORT_WANTS_NETWORK, + radio_port_check(RADIO_PORT_KIND_NETWORK, "/dev/ttyUSB0")); +} + +/* --- what must NOT be flagged ------------------------------------------ */ + +void test_network_rig_with_an_address_is_accepted(void) +{ + const char *good[] = { + "192.168.1.50", /* what fixed the reporter's setup */ + "192.168.1.50:4992", /* explicit port */ + "localhost", + "localhost:4532", /* rigctld */ + "flex.local", + "::1", /* bare IPv6 loopback */ + "[fe80::1]:4992", /* bracketed IPv6 with port */ + }; + for (unsigned i = 0; i < sizeof(good) / sizeof(good[0]); i++) + TEST_ASSERT_EQUAL_MESSAGE(RADIO_PORT_OK, + radio_port_check(RADIO_PORT_KIND_NETWORK, good[i]), good[i]); +} + +void test_serial_rig_with_a_port_name_is_accepted(void) +{ + const char *good[] = { + "COM4", "COM12", "com4", /* Windows, incl. lower case */ + "\\\\.\\COM12", /* \\.\COM12 for ports > 9 */ + "/dev/ttyUSB0", + "/dev/cu.usbserial-A600eHIS", /* macOS */ + "/dev/serial/by-id/usb-FTDI", /* stable udev path */ + }; + for (unsigned i = 0; i < sizeof(good) / sizeof(good[0]); i++) + TEST_ASSERT_EQUAL_MESSAGE(RADIO_PORT_OK, + radio_port_check(RADIO_PORT_KIND_SERIAL, good[i]), good[i]); +} + +/* An empty device means "use Hamlib's default", which is a legitimate choice + * for either kind of rig and must never draw a warning. */ +void test_unset_device_is_never_flagged(void) +{ + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_NETWORK, "")); + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_SERIAL, "")); + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_NETWORK, NULL)); + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_SERIAL, NULL)); +} + +/* Dummy rigs, USB-native rigs and anything else we do not model must pass + * whatever the user wrote: we have no opinion, so we hold none. */ +void test_other_port_kinds_are_never_flagged(void) +{ + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_OTHER, "COM4")); + TEST_ASSERT_EQUAL(RADIO_PORT_OK, radio_port_check(RADIO_PORT_KIND_OTHER, "192.168.1.50")); + TEST_ASSERT_NULL(radio_port_advice(RADIO_PORT_OK)); +} + +/* --- the serial-side mirror -------------------------------------------- */ + +void test_serial_rig_with_an_address_is_flagged(void) +{ + TEST_ASSERT_EQUAL(RADIO_PORT_WANTS_SERIAL, + radio_port_check(RADIO_PORT_KIND_SERIAL, "192.168.1.50")); + TEST_ASSERT_NOT_NULL(radio_port_advice( + radio_port_check(RADIO_PORT_KIND_SERIAL, "192.168.1.50"))); +} + +/* --- the classifier itself --------------------------------------------- */ + +void test_com_prefix_matching_is_not_greedy(void) +{ + /* "COM" must be followed by digits and nothing else, or a hostname that + * merely starts with those letters gets misread as a serial port. */ + TEST_ASSERT_TRUE(radio_port_path_is_serial("COM4")); + TEST_ASSERT_TRUE(radio_port_path_is_serial("COM10")); + + TEST_ASSERT_FALSE(radio_port_path_is_serial("COM4x")); + TEST_ASSERT_FALSE(radio_port_path_is_serial("COM")); + TEST_ASSERT_FALSE(radio_port_path_is_serial("COMPUTER")); + TEST_ASSERT_FALSE(radio_port_path_is_serial("com.example.net")); + TEST_ASSERT_FALSE(radio_port_path_is_serial("commsrv:4992")); +} + +void setUp(void) {} +void tearDown(void) {} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_network_rig_with_a_com_port_is_flagged); + RUN_TEST(test_network_rig_with_a_tty_is_flagged); + RUN_TEST(test_network_rig_with_an_address_is_accepted); + RUN_TEST(test_serial_rig_with_a_port_name_is_accepted); + RUN_TEST(test_unset_device_is_never_flagged); + RUN_TEST(test_other_port_kinds_are_never_flagged); + RUN_TEST(test_serial_rig_with_an_address_is_flagged); + RUN_TEST(test_com_prefix_matching_is_not_greedy); + return UNITY_END(); +}