From dc539d2dd62c520b3c59ebbea3fdeedaadda690b Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 23 Sep 2026 17:41:14 -0700 Subject: [PATCH] kiss_modem: answer FEM requests strictly in arrival order A second SetFemState sent while the first was held for a TX got an immediate TxBusy, which reached the host before the first request's reply. With no request IDs, the host could not tell which Set failed. FEM requests now go through a small ordered queue (Set, Get and their error replies). Sets still wait for TxDone to be queued and apply once. Each reply is kept until it can be queued. Capability checks happen in order at processing time. Only an overflow (more than 4 waiting) replies TxBusy immediately. --- docs/kiss_modem_protocol.md | 2 +- examples/kiss_modem/KissModem.cpp | 107 ++++++++++------------ examples/kiss_modem/KissModem.h | 30 ++++-- test/test_kiss_modem/test_hw_controls.cpp | 50 +++++++++- 4 files changed, 117 insertions(+), 72 deletions(-) diff --git a/docs/kiss_modem_protocol.md b/docs/kiss_modem_protocol.md index a37c4561..5eb09738 100644 --- a/docs/kiss_modem_protocol.md +++ b/docs/kiss_modem_protocol.md @@ -283,7 +283,7 @@ FemState response data: Example: enable RX gain without changing TX gain: apply `0x01`, value `0x01`. -If any bit in the apply mask is not in the board's capability mask (including reserved bits), the modem replies `Unsupported` and changes nothing. Otherwise SetFemState replies with the FemState response carrying the state read back from the board; if a bit differs from the request, that change did not take effect. A SetFemState received while a packet is on air (before its TxDone is queued) is held and applied after TxDone, so its FemState response follows TxDone. While a SetFemState is held or its response is still waiting for output space, GetFemState responses are queued behind it in order, and a further SetFemState returns `TxBusy`. Capabilities come from the board's own `get/set radio.fem.rxgain` and `radio.fem.txgain` CLI commands (see [cli_commands.md](./cli_commands.md)): a gain is controllable when the board answers `get` with `> on` or `> off`. Any board that implements those commands is supported without KISS changes, and a board with a fixed, board-managed FEM (for example RAK3401) reports none. Changes made over KISS are not saved to the board's preferences. +If any bit in the apply mask is not in the board's capability mask (including reserved bits), the modem replies `Unsupported` and changes nothing. Otherwise SetFemState replies with the FemState response carrying the state read back from the board; if a bit differs from the request, that change did not take effect. SetFemState and GetFemState requests, including their error replies, are answered strictly in the order received. A SetFemState received while a packet is on air (before its TxDone is queued) is held and applied after TxDone, so its reply follows TxDone, and FEM requests that arrive after it wait behind it. Up to 4 FEM requests can wait; beyond that the modem replies `TxBusy` immediately. Capabilities come from the board's own `get/set radio.fem.rxgain` and `radio.fem.txgain` CLI commands (see [cli_commands.md](./cli_commands.md)): a gain is controllable when the board answers `get` with `> on` or `> off`. Any board that implements those commands is supported without KISS changes, and a board with a fixed, board-managed FEM (for example RAK3401) reports none. Changes made over KISS are not saved to the board's preferences. The setting is not persisted; the board's power-on default applies after reboot. diff --git a/examples/kiss_modem/KissModem.cpp b/examples/kiss_modem/KissModem.cpp index a6950bff..4d9cdbdb 100644 --- a/examples/kiss_modem/KissModem.cpp +++ b/examples/kiss_modem/KissModem.cpp @@ -28,11 +28,8 @@ KissModem::KissModem(Stream& serial, mesh::LocalIdentity& identity, mesh::RNG& r _signal_report_enabled = true; _agc_reset_interval_sec = KISS_AGC_RESET_DEFAULT_SEC; _next_agc_reset_ms = 0; - _fem_deferred = false; - _fem_deferred_apply = 0; - _fem_deferred_value = 0; - _fem_reply_pending = false; - _fem_deferred_gets = 0; + _fem_op_head = 0; + _fem_op_count = 0; resetOutputQueue(); } @@ -42,9 +39,8 @@ void KissModem::begin() { _rx_active = false; _has_pending_tx = false; _tx_state = TX_IDLE; - _fem_deferred = false; - _fem_reply_pending = false; - _fem_deferred_gets = 0; + _fem_op_head = 0; + _fem_op_count = 0; _next_agc_reset_ms = millis() + (uint32_t)_agc_reset_interval_sec * 1000; resetOutputQueue(); } @@ -255,7 +251,7 @@ void KissModem::loop() { } processTx(); - processDeferredFem(); + processFemOps(); maybeResetAgc(); tryFlushFrames(); queuePendingBusyError(); @@ -858,10 +854,10 @@ void KissModem::readFemState(uint8_t* caps, uint8_t* values) { } } -bool KissModem::queueFemReply(bool mark_busy_error) { +bool KissModem::queueFemReply() { uint8_t buf[2]; readFemState(&buf[0], &buf[1]); - return queueHardwareFrame(HW_RESP(HW_CMD_GET_FEM_STATE), buf, 2, mark_busy_error); + return queueHardwareFrame(HW_RESP(HW_CMD_GET_FEM_STATE), buf, 2, false); } // The reply always carries the read-back state, so a bit that failed to apply shows its real value. @@ -874,65 +870,58 @@ void KissModem::applyFemState(uint8_t apply_mask, uint8_t value_mask) { } } -void KissModem::processDeferredFem() { - // wait for TxDone to be queued so the host sees the TX result before the FEM change - if (_fem_deferred && _tx_state != TX_SENDING && _tx_state != TX_DONE_PENDING) { - applyFemState(_fem_deferred_apply, _fem_deferred_value); - _fem_deferred = false; - _fem_reply_pending = true; +void KissModem::enqueueFemOp(FemOpKind kind, uint8_t apply_mask, uint8_t value_mask) { + if (_fem_op_count >= KISS_FEM_OP_QUEUE_DEPTH) { + writeHardwareError(HW_ERR_TX_BUSY); // too many FEM requests waiting + return; } - if (_fem_deferred) return; - // replies are retained until queued, and GetFemState replies stay behind the Set reply - if (_fem_reply_pending) { - if (!queueFemReply(false)) return; - _fem_reply_pending = false; - } - while (_fem_deferred_gets > 0) { - if (!queueFemReply(false)) return; - _fem_deferred_gets--; + FemOp& op = _fem_ops[(_fem_op_head + _fem_op_count) % KISS_FEM_OP_QUEUE_DEPTH]; + op.kind = kind; + op.apply_mask = apply_mask; + op.value_mask = value_mask; + op.applied = false; + _fem_op_count++; + processFemOps(); +} + +// Every FEM reply goes out in request order. A Set waits until TxDone is queued, since board +// setters drive FEM pins directly; each reply is retained until there is room to queue it. +void KissModem::processFemOps() { + while (_fem_op_count > 0) { + FemOp& op = _fem_ops[_fem_op_head]; + if (op.kind == FEM_OP_ERROR) { + if (!queueHardwareFrame(HW_RESP_ERROR, &op.value_mask, 1, false)) return; + } else { + if (op.kind == FEM_OP_SET && !op.applied) { + if (_tx_state == TX_SENDING || _tx_state == TX_DONE_PENDING) return; + uint8_t caps, values; + readFemState(&caps, &values); + if (op.apply_mask & ~caps) { + // all-or-nothing: any bit the board cannot control rejects the whole request + op.kind = FEM_OP_ERROR; + op.value_mask = HW_ERR_UNSUPPORTED; + continue; + } + applyFemState(op.apply_mask, op.value_mask); + op.applied = true; + } + if (!queueFemReply()) return; + } + _fem_op_head = (uint8_t)((_fem_op_head + 1) % KISS_FEM_OP_QUEUE_DEPTH); + _fem_op_count--; } } void KissModem::handleSetFemState(const uint8_t* data, uint16_t len) { if (len < 2) { - writeHardwareError(HW_ERR_INVALID_LENGTH); + enqueueFemOp(FEM_OP_ERROR, 0, HW_ERR_INVALID_LENGTH); return; } - uint8_t apply_mask = data[0]; - uint8_t value_mask = data[1]; - // validate every requested bit before touching hardware so a request is all-or-nothing - uint8_t fem_caps, fem_values; - readFemState(&fem_caps, &fem_values); - if (apply_mask & ~fem_caps) { - writeHardwareError(HW_ERR_UNSUPPORTED); - return; - } - if (isFemReplyQueued()) { - writeHardwareError(HW_ERR_TX_BUSY); - return; - } - if (_tx_state == TX_SENDING || _tx_state == TX_DONE_PENDING) { - // board setters drive FEM pins directly; changing them mid-packet would corrupt the TX - _fem_deferred = true; - _fem_deferred_apply = apply_mask; - _fem_deferred_value = value_mask; - return; - } - applyFemState(apply_mask, value_mask); - _fem_reply_pending = true; - processDeferredFem(); + enqueueFemOp(FEM_OP_SET, data[0], data[1]); } void KissModem::handleGetFemState() { - if (isFemReplyQueued()) { - if (_fem_deferred_gets == UINT8_MAX) { - writeHardwareError(HW_ERR_TX_BUSY); - return; - } - _fem_deferred_gets++; - return; - } - queueFemReply(true); + enqueueFemOp(FEM_OP_GET, 0, 0); } void KissModem::handleSetRxBoostedGain(const uint8_t* data, uint16_t len) { diff --git a/examples/kiss_modem/KissModem.h b/examples/kiss_modem/KissModem.h index 572999c4..24021bdb 100644 --- a/examples/kiss_modem/KissModem.h +++ b/examples/kiss_modem/KissModem.h @@ -103,6 +103,9 @@ /* Matches the CLI reply buffer boards write into from handleCommand() */ #define KISS_BOARD_REPLY_SIZE 160 +/* FEM requests are answered strictly in arrival order; this many may wait (e.g. during a TX) */ +#define KISS_FEM_OP_QUEUE_DEPTH 4 + #define KISS_AGC_RESET_DEFAULT_SEC 30 #define KISS_AGC_RESET_MAX_SEC 1020 #define KISS_AGC_RESET_STEP_SEC 4 @@ -125,6 +128,19 @@ struct RadioConfig { uint8_t tx_power; }; +enum FemOpKind : uint8_t { + FEM_OP_SET, + FEM_OP_GET, + FEM_OP_ERROR +}; + +struct FemOp { + FemOpKind kind; + uint8_t apply_mask; // FEM_OP_SET + uint8_t value_mask; // FEM_OP_SET; error code for FEM_OP_ERROR + bool applied; // FEM_OP_SET: hardware changed, reply still to be queued +}; + enum TxState { TX_IDLE, TX_WAIT_CLEAR, @@ -172,11 +188,9 @@ class KissModem { bool _signal_report_enabled; uint16_t _agc_reset_interval_sec; uint32_t _next_agc_reset_ms; - bool _fem_deferred; - uint8_t _fem_deferred_apply; - uint8_t _fem_deferred_value; - bool _fem_reply_pending; - uint8_t _fem_deferred_gets; + FemOp _fem_ops[KISS_FEM_OP_QUEUE_DEPTH]; + uint8_t _fem_op_head; + uint8_t _fem_op_count; uint8_t _tx_frame_buf[KISS_TX_FRAME_QUEUE_DEPTH][KISS_MAX_ENCODED_FRAME_SIZE]; uint16_t _tx_frame_len[KISS_TX_FRAME_QUEUE_DEPTH]; uint16_t _tx_frame_written[KISS_TX_FRAME_QUEUE_DEPTH]; @@ -207,10 +221,10 @@ class KissModem { bool queryFemGain(uint8_t bit, bool* enabled); bool setFemGain(uint8_t bit, bool enable); void readFemState(uint8_t* caps, uint8_t* values); - bool queueFemReply(bool mark_busy_error); + bool queueFemReply(); void applyFemState(uint8_t apply_mask, uint8_t value_mask); - bool isFemReplyQueued() const { return _fem_deferred || _fem_reply_pending || _fem_deferred_gets > 0; } - void processDeferredFem(); + void enqueueFemOp(FemOpKind kind, uint8_t apply_mask, uint8_t value_mask); + void processFemOps(); void handleGetIdentity(); void handleGetRandom(const uint8_t* data, uint16_t len); diff --git a/test/test_kiss_modem/test_hw_controls.cpp b/test/test_kiss_modem/test_hw_controls.cpp index 16ef50df..e0fdd729 100644 --- a/test/test_kiss_modem/test_hw_controls.cpp +++ b/test/test_kiss_modem/test_hw_controls.cpp @@ -330,20 +330,62 @@ TEST_F(KissHwControlTest, FemSetDuringTxIsDeferredUntilAfterTxDone) { serial.takeHardwareFrames(); EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, HW_FEM_RX_GAIN}).empty()); + EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, 0}).empty()); // queued behind the first EXPECT_EQ(board.lna_sets, 0); - EXPECT_EQ(hw1({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, 0}), (std::vector{HW_RESP_ERROR, HW_ERR_TX_BUSY})); radio.send_complete = true; modem.loop(); // TX done -> TX_DONE_PENDING; FEM still held - modem.loop(); // TxDone queued -> idle; FEM applied + modem.loop(); // TxDone queued -> idle; FEM applied in order + for (int i = 0; i < 3; i++) modem.loop(); auto frames = serial.takeHardwareFrames(); - ASSERT_EQ(frames.size(), 2U); + ASSERT_EQ(frames.size(), 3U); EXPECT_EQ(frames[0], (std::vector{HW_RESP_TX_DONE, 0x01})); EXPECT_EQ(frames[1], (std::vector{0x9F, 0x01, 0x01})); - EXPECT_TRUE(board.lna); + EXPECT_EQ(frames[2], (std::vector{0x9F, 0x01, 0x00})); + EXPECT_FALSE(board.lna); + EXPECT_EQ(board.lna_sets, 2); +} + +TEST_F(KissHwControlTest, PipelinedFemRequestsAreAnsweredInArrivalOrder) { + board.has_lna = true; + startTxAndHold(); + serial.takeHardwareFrames(); + + EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, HW_FEM_RX_GAIN}).empty()); + EXPECT_TRUE(hw({HW_CMD_GET_FEM_STATE}).empty()); + EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_TX_GAIN, HW_FEM_TX_GAIN}).empty()); // unsupported bit + EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN}).empty()); // short payload + + radio.send_complete = true; + for (int i = 0; i < 6; i++) modem.loop(); + auto frames = serial.takeHardwareFrames(); + ASSERT_EQ(frames.size(), 5U); + EXPECT_EQ(frames[0], (std::vector{HW_RESP_TX_DONE, 0x01})); + EXPECT_EQ(frames[1], (std::vector{0x9F, 0x01, 0x01})); + EXPECT_EQ(frames[2], (std::vector{0x9F, 0x01, 0x01})); + EXPECT_EQ(frames[3], ERR_UNSUPPORTED); + EXPECT_EQ(frames[4], (std::vector{HW_RESP_ERROR, HW_ERR_INVALID_LENGTH})); EXPECT_EQ(board.lna_sets, 1); } +TEST_F(KissHwControlTest, FemQueueOverflowRepliesTxBusyImmediately) { + board.has_lna = true; + startTxAndHold(); + serial.takeHardwareFrames(); + for (int i = 0; i < KISS_FEM_OP_QUEUE_DEPTH; i++) { + EXPECT_TRUE(hw({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, HW_FEM_RX_GAIN}).empty()); + } + EXPECT_EQ(hw1({HW_CMD_GET_FEM_STATE}), (std::vector{HW_RESP_ERROR, HW_ERR_TX_BUSY})); + EXPECT_EQ(board.lna_sets, 0); +} + +TEST_F(KissHwControlTest, FemGetWithNothingQueuedIsAnsweredDuringTx) { + board.has_lna = true; + startTxAndHold(); + serial.takeHardwareFrames(); + EXPECT_EQ(hw1({HW_CMD_GET_FEM_STATE}), (std::vector{0x9F, 0x01, 0x00})); +} + TEST_F(KissHwControlTest, FemGetDuringDeferredSetIsAnsweredAfterSet) { board.has_lna = true; startTxAndHold();