From 9235d761da54a2256cbeaa7cb908dabc66cb9c8e Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 23 Sep 2026 16:11:29 -0700 Subject: [PATCH] kiss_modem: keep FEM replies ordered and deliverable; drop Wio FEM control - Wio Tracker L1 1W no longer advertises TX gain control: its setter drives LORA_PWR_EN, the shared LDO for the SX1262 and the PA, so "gain off" would power down the radio. - A SetFemState that arrives mid-TX is now applied after TxDone is queued, so its reply follows TxDone. - SetFemState replies (immediate or deferred) are retained until they fit in the host output queue instead of being dropped, so a change that took effect is always acknowledged. - GetFemState replies queue behind an outstanding SetFemState reply; a further SetFemState returns TxBusy until it clears. --- docs/kiss_modem_protocol.md | 2 +- examples/kiss_modem/KissModem.cpp | 64 ++++++++++---- examples/kiss_modem/KissModem.h | 8 +- test/test_kiss_modem/test_hw_controls.cpp | 83 +++++++++++++++++-- .../wio-tracker-l1-1w/WioTrackerL1Board.h | 6 +- 5 files changed, 131 insertions(+), 32 deletions(-) diff --git a/docs/kiss_modem_protocol.md b/docs/kiss_modem_protocol.md index 9b8aaa4b..1a668cc4 100644 --- a/docs/kiss_modem_protocol.md +++ b/docs/kiss_modem_protocol.md @@ -279,7 +279,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. On success, SetFemState replies with the FemState response carrying the new state. A request received while a packet is on air is applied as soon as that transmission ends, and the FemState response is sent then; a second request during the same transmission returns `TxBusy`. Capabilities come from the board, so a board with a fixed, board-managed FEM (for example RAK3401) reports none. +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. On success, SetFemState replies with the FemState response carrying the new state. 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, so a board with a fixed, board-managed FEM (for example RAK3401) reports none. 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 5b61e83c..af678bbe 100644 --- a/examples/kiss_modem/KissModem.cpp +++ b/examples/kiss_modem/KissModem.cpp @@ -27,6 +27,9 @@ KissModem::KissModem(Stream& serial, mesh::LocalIdentity& identity, mesh::RNG& r _fem_deferred = false; _fem_deferred_apply = 0; _fem_deferred_value = 0; + _fem_reply_pending = false; + _fem_reply_ok = false; + _fem_deferred_gets = 0; resetOutputQueue(); } @@ -37,6 +40,8 @@ void KissModem::begin() { _has_pending_tx = false; _tx_state = TX_IDLE; _fem_deferred = false; + _fem_reply_pending = false; + _fem_deferred_gets = 0; _next_agc_reset_ms = millis() + (uint32_t)_agc_reset_interval_sec * 1000; resetOutputQueue(); } @@ -811,12 +816,16 @@ uint8_t KissModem::femValueMask() const { return mask; } -void KissModem::writeFemState() { +bool KissModem::queueFemReply(bool ok, bool mark_busy_error) { + if (!ok) { + const uint8_t err = HW_ERR_UNSUPPORTED; + return queueHardwareFrame(HW_RESP_ERROR, &err, 1, mark_busy_error); + } uint8_t buf[2] = { femCapabilityMask(), femValueMask() }; - writeHardwareFrame(HW_RESP(HW_CMD_GET_FEM_STATE), buf, 2); + return queueHardwareFrame(HW_RESP(HW_CMD_GET_FEM_STATE), buf, 2, mark_busy_error); } -void KissModem::applyFemState(uint8_t apply_mask, uint8_t value_mask) { +bool KissModem::applyFemState(uint8_t apply_mask, uint8_t value_mask) { bool ok = true; if (apply_mask & HW_FEM_RX_GAIN) { ok &= _board.setLoRaFemLnaEnabled((value_mask & HW_FEM_RX_GAIN) != 0); @@ -824,17 +833,26 @@ void KissModem::applyFemState(uint8_t apply_mask, uint8_t value_mask) { if (apply_mask & HW_FEM_TX_GAIN) { ok &= _board.setLoRaFemPaGainEnabled((value_mask & HW_FEM_TX_GAIN) != 0); } - if (ok) { - writeFemState(); - } else { - writeHardwareError(HW_ERR_UNSUPPORTED); - } + return ok; } void KissModem::processDeferredFem() { - if (!_fem_deferred || _tx_state == TX_SENDING) return; - _fem_deferred = false; - applyFemState(_fem_deferred_apply, _fem_deferred_value); + // 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) { + _fem_reply_ok = applyFemState(_fem_deferred_apply, _fem_deferred_value); + _fem_deferred = false; + _fem_reply_pending = true; + } + if (_fem_deferred) return; + // replies are retained until queued, and GetFemState replies stay behind the Set reply + if (_fem_reply_pending) { + if (!queueFemReply(_fem_reply_ok, false)) return; + _fem_reply_pending = false; + } + while (_fem_deferred_gets > 0) { + if (!queueFemReply(true, false)) return; + _fem_deferred_gets--; + } } void KissModem::handleSetFemState(const uint8_t* data, uint16_t len) { @@ -849,20 +867,30 @@ void KissModem::handleSetFemState(const uint8_t* data, uint16_t len) { writeHardwareError(HW_ERR_UNSUPPORTED); return; } - if (_tx_state == TX_SENDING) { + 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 - if (_fem_deferred) { - writeHardwareError(HW_ERR_TX_BUSY); - return; - } _fem_deferred = true; _fem_deferred_apply = apply_mask; _fem_deferred_value = value_mask; return; } - applyFemState(apply_mask, value_mask); + _fem_reply_ok = applyFemState(apply_mask, value_mask); + _fem_reply_pending = true; + processDeferredFem(); } void KissModem::handleGetFemState() { - writeFemState(); + if (isFemReplyQueued()) { + if (_fem_deferred_gets == UINT8_MAX) { + writeHardwareError(HW_ERR_TX_BUSY); + return; + } + _fem_deferred_gets++; + return; + } + queueFemReply(true, true); } diff --git a/examples/kiss_modem/KissModem.h b/examples/kiss_modem/KissModem.h index 751fb336..25184217 100644 --- a/examples/kiss_modem/KissModem.h +++ b/examples/kiss_modem/KissModem.h @@ -163,6 +163,9 @@ class KissModem { bool _fem_deferred; uint8_t _fem_deferred_apply; uint8_t _fem_deferred_value; + bool _fem_reply_pending; + bool _fem_reply_ok; + uint8_t _fem_deferred_gets; 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]; @@ -192,8 +195,9 @@ class KissModem { void maybeResetAgc(); uint8_t femCapabilityMask() const; uint8_t femValueMask() const; - void writeFemState(); - void applyFemState(uint8_t apply_mask, uint8_t value_mask); + bool queueFemReply(bool ok, bool mark_busy_error); + bool 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 handleGetIdentity(); diff --git a/test/test_kiss_modem/test_hw_controls.cpp b/test/test_kiss_modem/test_hw_controls.cpp index ad6f1e36..614ccc24 100644 --- a/test/test_kiss_modem/test_hw_controls.cpp +++ b/test/test_kiss_modem/test_hw_controls.cpp @@ -13,8 +13,9 @@ public: for (uint8_t b : bytes) _rx.push(b); } - int availableForWrite() override { return 4096; } + int availableForWrite() override { return blocked ? 0 : 4096; } size_t write(const uint8_t* buffer, size_t size) override { + if (blocked) return 0; _writes.insert(_writes.end(), buffer, buffer + size); return size; } @@ -49,6 +50,8 @@ public: return frames; } + bool blocked = false; + private: std::queue _rx; std::vector _writes; @@ -254,7 +257,7 @@ TEST_F(KissHwControlTest, FemShortPayloadIsInvalidLength) { EXPECT_EQ(hw1({HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN}), (std::vector{HW_RESP_ERROR, HW_ERR_INVALID_LENGTH})); } -TEST_F(KissHwControlTest, FemSetDuringTxIsDeferredUntilTxCompletes) { +TEST_F(KissHwControlTest, FemSetDuringTxIsDeferredUntilAfterTxDone) { board.has_lna = true; startTxAndHold(); serial.takeHardwareFrames(); @@ -264,15 +267,79 @@ TEST_F(KissHwControlTest, FemSetDuringTxIsDeferredUntilTxCompletes) { 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 + auto frames = serial.takeHardwareFrames(); + ASSERT_EQ(frames.size(), 2U); + 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(board.lna_sets, 1); +} + +TEST_F(KissHwControlTest, FemGetDuringDeferredSetIsAnsweredAfterSet) { + 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()); + + radio.send_complete = true; + modem.loop(); modem.loop(); auto frames = serial.takeHardwareFrames(); - ASSERT_EQ(frames.size(), 1U); - EXPECT_EQ(frames[0], (std::vector{0x9F, 0x01, 0x01})); - EXPECT_TRUE(board.lna); - modem.loop(); - frames = serial.takeHardwareFrames(); - ASSERT_EQ(frames.size(), 1U); + 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_EQ(frames[2], (std::vector{0x9F, 0x01, 0x01})); +} + +TEST_F(KissHwControlTest, DeferredFemRepliesSurviveHostBackpressure) { + board.has_lna = true; + hw({HW_CMD_SET_SIGNAL_REPORT, 0}); // one output frame per RX packet + 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()); + + // with writes blocked: RX packet takes slot 1, TxDone slot 2, so the Set reply cannot be queued + serial.blocked = true; + static constexpr uint8_t PKT[] = {0x01}; + modem.onPacketReceived(0, 0, PKT, sizeof(PKT)); + radio.send_complete = true; + for (int i = 0; i < 5; i++) modem.loop(); + EXPECT_TRUE(board.lna); // applied once TxDone was queued, reply still held + + serial.blocked = false; + for (int i = 0; i < 5; i++) modem.loop(); + EXPECT_EQ(board.lna_sets, 1); + + auto frames = serial.takeHardwareFrames(); + 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_EQ(frames[2], (std::vector{0x9F, 0x01, 0x01})); +} + +TEST_F(KissHwControlTest, ImmediateFemReplySurvivesHostBackpressure) { + board.has_lna = true; + hw({HW_CMD_SET_SIGNAL_REPORT, 0}); + + serial.blocked = true; + static constexpr uint8_t PKT[] = {0x01}; + modem.onPacketReceived(0, 0, PKT, sizeof(PKT)); + modem.onPacketReceived(0, 0, PKT, sizeof(PKT)); // output queue now full + serial.pushRx({KISS_FEND, KISS_CMD_SETHARDWARE, HW_CMD_SET_FEM_STATE, HW_FEM_RX_GAIN, HW_FEM_RX_GAIN, KISS_FEND}); + modem.loop(); + EXPECT_TRUE(board.lna); + + serial.blocked = false; + for (int i = 0; i < 5; i++) modem.loop(); + auto frames = serial.takeHardwareFrames(); + ASSERT_EQ(frames.size(), 1U); // FemState, not TxBusy + EXPECT_EQ(frames[0], (std::vector{0x9F, 0x01, 0x01})); + EXPECT_EQ(board.lna_sets, 1); } } // namespace diff --git a/variants/wio-tracker-l1-1w/WioTrackerL1Board.h b/variants/wio-tracker-l1-1w/WioTrackerL1Board.h index 7dde409d..798145db 100644 --- a/variants/wio-tracker-l1-1w/WioTrackerL1Board.h +++ b/variants/wio-tracker-l1-1w/WioTrackerL1Board.h @@ -10,14 +10,14 @@ protected: bool _is_pa_enabled = false; KeyValueStore* _prefs = NULL; + bool setLoRaFemPaGainEnabled(bool enable); + bool isLoRaFemPaGainEnabled() const { return _is_pa_enabled; } + public: WioTrackerL1Board() : NRF52Board("WioTrackerL1 OTA") {} void begin(); void attachDynamicPrefs(KeyValueStore* prefs); bool handleCommand(const char* command, uint32_t sender_timestamp, char* reply) override; - bool canControlLoRaFemPaGain() const override { return true; } - bool setLoRaFemPaGainEnabled(bool enable) override; - bool isLoRaFemPaGainEnabled() const override { return _is_pa_enabled; } #if defined(P_LORA_TX_LED) void onBeforeTransmit() override {