From b27f5e062580d344699f4358960c789f82c1be5f Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:22:24 +0200 Subject: [PATCH] x1 duty cycle debug --- zephcore/adapters/radio/LR2021Radio.cpp | 10 + .../adapters/radio/lr20xx/lr20xx_hal_zephyr.c | 28 ++- .../adapters/radio/lr20xx/lr20xx_hal_zephyr.h | 9 +- .../drivers/lora/lr20xx/lr20xx_lora.c | 208 +++++++++++++++--- .../drivers/lora/lr20xx/lr20xx_lora.h | 12 + 5 files changed, 227 insertions(+), 40 deletions(-) diff --git a/zephcore/adapters/radio/LR2021Radio.cpp b/zephcore/adapters/radio/LR2021Radio.cpp index c8f2dc1..ec556c9 100644 --- a/zephcore/adapters/radio/LR2021Radio.cpp +++ b/zephcore/adapters/radio/LR2021Radio.cpp @@ -73,6 +73,16 @@ int16_t LR2021Radio::hwGetCurrentRSSI() return lr20xx_get_rssi_inst(_dev); } +/* Deliberately no hwIsChipBusy() override, matching the LR11xx. + * + * It backs LoRaRadioBase::isRadioReady(), which gates TX in startSendRaw() as + * well as the probes. Wiring BUSY into the transmit path stalled sends until + * the dispatcher's 4 s CAD timeout, because the event-driven loop has nothing + * to re-wake a deferred send. The duty-cycle sleep window is handled where it + * belongs instead: skipped in the two incidental pollers + * (lr20xx_is_receiving, lr20xx_get_rssi_inst) and stood down deliberately for + * TX inside the driver. */ + bool LR2021Radio::hwIsReceiving() { /* MUST be non-destructive: never clear IRQ bits from this path. diff --git a/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.c b/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.c index 7034256..63506e3 100644 --- a/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.c +++ b/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.c @@ -76,13 +76,27 @@ static lr20xx_hal_status_t wait_on_busy(struct lr20xx_hal_context *ctx) static lr20xx_hal_status_t check_device_ready(struct lr20xx_hal_context *ctx) { if (!ctx->radio_is_sleeping) { - /* In RX duty cycle the chip sleeps between windows on its own, so - * the flag above is false while BUSY is high and the radio is not - * listening. Waiting would just burn the BUSY timeout; wake it the - * same way an explicit sleep is woken. */ - if (!ctx->auto_sleeps || !gpio_pin_get_dt(&ctx->busy)) { - return wait_on_busy(ctx); - } + /* Always wait — never second-guess a high BUSY. + * + * There used to be an "if a duty cycle is armed and BUSY is high + * the chip must have self-slept, so wake it" short-circuit here. + * It could not tell self-sleep apart from a command still running, + * and BUSY is high for both: CAD holds it for the CAD duration, + * SetTx until the PA has ramped, SetRxDutyCycle and the FE + * calibration for their own spans. So after every slow command + * the next one skipped the wait and fired an NSS pulse instead — + * a frame with no clock cycles, which the chip reads as a + * malformed command (CMD_PERR, DS Table 6-38) and which latches + * CmdError. That is the rejection storm and reset loop seen on + * the MeshTracker X1 whenever rxduty was on, and why rxduty off + * always worked. + * + * Waiting costs nothing worth saving: a duty-cycle sleep window is + * bounded by cycle_time - rx_max_time (25 ms at the presets we + * use) and the chip drops BUSY by itself on waking into its RX + * window. There is no timeout to burn. The LR11xx driver has + * never had such a branch, for the same reason. */ + return wait_on_busy(ctx); } /* Wake from sleep: the chip leaves Sleep when NSS is held low for 100us diff --git a/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.h b/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.h index f0ce8bd..3dbcd10 100644 --- a/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.h +++ b/zephcore/adapters/radio/lr20xx/lr20xx_hal_zephyr.h @@ -35,11 +35,12 @@ struct lr20xx_hal_context { struct gpio_dt_spec busy; /* BUSY: high = chip processing command */ struct gpio_dt_spec dio1; /* DIO1 interrupt */ + /* Tracks an explicit host SetSleep only. There is deliberately no + * companion flag for the autonomous duty-cycle sleep: BUSY cannot + * distinguish that from a command still executing, so acting on it + * corrupted transactions (see check_device_ready). A duty-cycle sleep + * is simply waited out. */ volatile bool radio_is_sleeping; - /* Set while RX duty cycling: the chip parks itself in sleep between - * windows without the host ever sending SetSleep, so radio_is_sleeping - * cannot be trusted to spot it. */ - volatile bool auto_sleeps; }; /** diff --git a/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.c b/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.c index 4d09c60..f61b982 100644 --- a/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.c +++ b/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.c @@ -113,6 +113,11 @@ struct lr20xx_data { /* Deferred hardware init */ bool hw_initialized; + /* Chip-side DIO carrying the IRQ line, cached from the devicetree at + * hw init. Reporting only — so the debug dump can name the pin this + * board actually wired instead of assuming one. */ + uint8_t irq_dio; + /* DIO1 stuck-HIGH detection */ int dio1_stuck_count; bool tcxo_disabled; /* set when the TCXO fallback has already fired */ @@ -134,26 +139,43 @@ struct lr20xx_data { /* ── Debug: dump full chip state (log builds only) ──────────────────── */ #if IS_ENABLED(CONFIG_LOG) -static void dump_chip_state(void *ctx, struct lr20xx_hal_context *hal, - const char *label) +/* One line of chip state. Takes the whole driver instance: the old signature + * took a command context and a HAL context separately, and every one of the + * nine call sites passed the same object twice. */ +static void dump_chip_state(struct lr20xx_data *data, const char *label) { + struct lr20xx_hal_context *hal = &data->hal_ctx; + void *ctx = hal; lr20xx_system_stat1_t s1 = {0}; lr20xx_system_stat2_t s2 = {0}; lr20xx_system_irq_mask_t irq = 0; lr20xx_system_errors_t err = 0; + /* Sample the pins BEFORE issuing any SPI. DS §5: "The BUSY pin is + * automatically asserted on the falling edge of the NSS" — read after + * the two commands below and this dump reports its own footprint, not + * the chip's state. Every BUSY=1 in the X1 bring-up logs came from + * that, and it cost a whole (wrong) conclusion about this chip holding + * BUSY high during Rx. It does not: "In Rx mode, BUSY goes low as soon + * as the chip is ready to receive data." */ + int busy = gpio_pin_get_dt(&hal->busy); + int dio = gpio_pin_get_dt(&hal->dio1); + lr20xx_system_get_status(ctx, &s1, &s2, &irq); lr20xx_system_get_errors(ctx, &err); - int busy = gpio_pin_get_dt(&hal->busy); - int dio9 = gpio_pin_get_dt(&hal->dio1); - - LOG_INF("[%s] cmd=%d mode=%d err=0x%04x irq=0x%08x BUSY=%d DIO9=%d", - label, s1.command_status, s2.chip_mode, err, irq, busy, dio9); + /* Name the DIO the board actually uses. This was hardcoded "DIO9", + * which is right on promicro_lr2021 and wrong on meshtracker_x1 (DIO8, + * per irq-dio in its DTS and the Seeed block diagram) — a label that is + * correct on one board and silently lying on another is worse than one + * that is obviously generic. */ + LOG_INF("[%s] cmd=%d mode=%d err=0x%04x irq=0x%08x BUSY=%d DIO%u=%d", + label, s1.command_status, s2.chip_mode, err, irq, busy, + (unsigned)data->irq_dio, dio); } -#define DUMP_CHIP_STATE(ctx, hal, label) dump_chip_state(ctx, hal, label) +#define DUMP_CHIP_STATE(data, label) dump_chip_state(data, label) #else -#define DUMP_CHIP_STATE(ctx, hal, label) do { } while (0) +#define DUMP_CHIP_STATE(data, label) do { } while (0) #endif /* IS_ENABLED(CONFIG_LOG) */ /* ── Helpers ────────────────────────────────────────────────────────── */ @@ -382,7 +404,13 @@ static void lr20xx_hardware_reset(struct lr20xx_data *data, tcxo_start_time_periods(cfg->tcxo_startup_delay_ms)); } - /* LDO mode — no cfg_lfclk, no set_reg_mode, no DCDC workarounds */ + /* LDO mode — no cfg_lfclk, no set_reg_mode, no DCDC workarounds. + * + * cfg_lfclk(LF_RC) was tried here on the theory that LF_XOSC_START_ERR + * meant the LF domain needed an explicit enable-and-wait (DS §6.11.1). + * It changed nothing on hardware — the error still appears — so it is + * not the cause and the command is not carried. RC is the reset + * default and the X1 routes no DIO11, so RC is what we get either way. */ lr20xx_configure_rfswitch(ctx, cfg); @@ -694,7 +722,7 @@ static void lr20xx_apply_modem_config(struct lr20xx_data *data, * them anyway). */ lr20xx_apply_side_detectors(data, !tx_mode); - DUMP_CHIP_STATE(ctx, &data->hal_ctx, tx_mode ? "modem-TX" : "modem-RX"); + DUMP_CHIP_STATE(data, tx_mode ? "modem-TX" : "modem-RX"); } /* ── RX duty cycle ──────────────────────────────────────────────────── */ @@ -711,20 +739,104 @@ static bool lr20xx_apply_rx_duty_cycle(struct lr20xx_data *data) if (data->dc_rx_ms == 0 || data->dc_sleep_ms == 0) { LOG_WRN("No duty-cycle timing stored — continuous RX"); data->rx_duty_cycle_enabled = false; - data->hal_ctx.auto_sleeps = false; lr20xx_radio_common_set_rx_with_timeout_in_rtc_step( ctx, 0xFFFFFF); return false; } - lr20xx_radio_common_set_rx_duty_cycle(ctx, data->dc_rx_ms, - data->dc_sleep_ms, LR20XX_RADIO_COMMON_RX_DUTY_CYCLE_MODE_RX); + /* Clear the error state the wake itself provokes, before re-arming. + * + * Reaching here almost always means the cycle was just broken by a host + * command landing in a sleep window — DS §6.3.8: "wake up by the device + * with a falling edge of NSS" terminates the loop, and every SPI + * transaction asserts NSS. The HAL's wake pulse is a frame with no + * clock cycles, which the chip reads as a malformed command and answers + * with CMD_PERR (DS Table 6-38, "wrong Opcode, arguments") plus a + * latched CmdError IRQ. + * + * Left set, that IRQ holds DIO1 high; the work handler finds no event + * it recognises, the safety path restarts RX, this re-arm is refused + * again, and after five strikes the driver hardware-resets — the loop + * seen on the X1, where the node never recovers and never gives up. + * The error is self-inflicted by the wake and says nothing about the + * command about to be issued, so clear it here rather than letting it + * poison the re-arm. The SX126x driver survives the same race by the + * same principle: re-arm unconditionally from the broken state. */ + lr20xx_system_clear_errors(ctx); + lr20xx_system_clear_irq_status(ctx, LR20XX_SYSTEM_IRQ_CMD_ERROR); - LOG_DBG("RX duty cycle re-armed: rx=%ums sleep=%ums", - data->dc_rx_ms, data->dc_sleep_ms); + /* The second SetRxDutyCycle field is cycle_time, NOT sleep time — + * DS §6.3.8 Table 6-14: "cycle_time specifies the duration of the period + * between the start of consecutive Rx windows", and the chip sleeps for + * cycle_time - rx_max_time. The vendor SDK calls the argument + * `sleep_period_in_ms` and marshals it straight into those bytes, which + * is correct for the LR11xx (whose field really is SleepPeriod) and + * wrong here — the LR2021 redefined it. Passing the sleep time raw + * makes cycle_time < rx_max_time for every preset we use, and the + * datasheet is explicit: "If cycle_time is lower than rx_max_time a + * CMD_ERR is returned in the status of the next command." That is the + * CMD_ERROR IRQ and the rejected commands seen on the X1 the moment a + * duty cycle armed. Convert here rather than at the call sites so + * dc_sleep_ms keeps its plain meaning everywhere else. */ + lr20xx_radio_common_set_rx_duty_cycle(ctx, data->dc_rx_ms, + data->dc_rx_ms + data->dc_sleep_ms, + LR20XX_RADIO_COMMON_RX_DUTY_CYCLE_MODE_RX); + + LOG_DBG("RX duty cycle re-armed: rx=%ums sleep=%ums (cycle=%ums)", + data->dc_rx_ms, data->dc_sleep_ms, + data->dc_rx_ms + data->dc_sleep_ms); return true; } +/* GPIO-only "can the host talk to this chip right now" check, no SPI. + * + * Gated on an armed duty cycle: unlike the SX126x, the LR2021 holds BUSY high + * during ordinary continuous RX, so reporting raw BUSY would tell the caller + * the radio is permanently unavailable — which gates TX as well as the probes + * and mutes the node. With a cycle armed, BUSY high does mean the chip is in + * its own sleep window and must not be disturbed. */ +bool lr20xx_is_chip_busy(const struct device *dev) +{ + struct lr20xx_data *data = dev->data; + + return data->rx_duty_cycle_enabled && + gpio_pin_get_dt(&data->hal_ctx.busy); +} + +/* Take an armed duty cycle down deliberately, for a path that MUST command the + * chip rather than skip when it is asleep — i.e. TX. + * + * Polling BUSY and waiting for a gap does not work for transmit. The main + * loop is event-driven, so a deferred send has nothing to re-wake it: one + * "asleep" sample and the packet sits until the dispatcher's 4 s CAD timeout + * fires, which is what the X1 logged as `isRadioReady=0`. The LR11xx driver + * guards its two incidental pollers and pointedly leaves TX alone for the + * same reason. + * + * DS §6.3.8 makes the sequence explicit: an NSS falling edge ends the + * duty-cycle loop, and "a SetStandby command should also be sent, to avoid + * the race conditions". The wake arrives via the HAL's pulse in + * check_device_ready(); the SetStandby is here. Clearing errors + CmdError + * afterwards drops the CMD_PERR the clockless wake frame provokes, so it + * cannot poison the TX commands that follow. RX is re-armed by restart_rx() + * on TX completion, and by start_rx() on the CAD-busy path. */ +static void lr20xx_dc_takeover(struct lr20xx_data *data) +{ + void *ctx = &data->hal_ctx; + + if (!data->rx_duty_cycle_enabled) { + return; + } + + k_mutex_lock(&data->spi_mutex, K_FOREVER); + lr20xx_system_set_standby_mode(ctx, LR20XX_SYSTEM_STANDBY_MODE_RC); + lr20xx_system_clear_errors(ctx); + lr20xx_system_clear_irq_status(ctx, LR20XX_SYSTEM_IRQ_CMD_ERROR); + k_mutex_unlock(&data->spi_mutex); + + LOG_DBG("duty cycle stood down for TX"); +} + /* ── Start RX (internal) ────────────────────────────────────────────── */ static void lr20xx_start_rx(struct lr20xx_data *data, @@ -768,7 +880,7 @@ static void lr20xx_start_rx(struct lr20xx_data *data, /* DEBUG: dump state AFTER SET_RX — should show mode=4 (RX). The * "modem-RX" dump inside apply_modem_config is taken before SET_RX. */ - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "post-SET_RX"); + DUMP_CHIP_STATE(data, "post-SET_RX"); } /* ── Lightweight RX restart (no modem reconfig) ─────────────────────── */ @@ -1120,7 +1232,7 @@ static int lr20xx_lora_config(const struct device *dev, lr20xx_calibrate_front_end(&data->hal_ctx, config->frequency); - DUMP_CHIP_STATE(&data->hal_ctx, &data->hal_ctx, "config-FEcal"); + DUMP_CHIP_STATE(data, "config-FEcal"); k_mutex_unlock(&data->spi_mutex); LOG_DBG("config: %uHz SF%d BW%d CR%d pwr=%d tx=%d", @@ -1193,6 +1305,10 @@ static int lr20xx_lora_send_async(const struct device *dev, if (data->tx_active) return -EBUSY; if (data_len > 255 || data_len == 0) return -EINVAL; + /* Own the chip for the whole transmit, LBT CAD included — the CAD below + * commands it too, so this has to come first. */ + lr20xx_dc_takeover(data); + /* LBT: perform blocking CAD before transmitting. On CAD-busy, restore * RX in-driver before returning -EBUSY so the C++ layer doesn't have * to do a full cancel-then-restart round-trip. lr20xx_lora_cad @@ -1270,7 +1386,7 @@ static int lr20xx_lora_send_async(const struct device *dev, /* Command status here is SET_TX's own (2=accepted, 1=rejected, * 0=not executed) and the mode should have left standby. */ - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "post-SET_TX"); + DUMP_CHIP_STATE(data, "post-SET_TX"); k_mutex_unlock(&data->spi_mutex); @@ -1354,7 +1470,25 @@ int16_t lr20xx_get_rssi_inst(const struct device *dev) int16_t rssi = 0; uint8_t half_dbm = 0; - k_mutex_lock(&data->spi_mutex, K_FOREVER); + /* GPIO BUSY read first, before any SPI: issuing GetRssiInst into a + * duty-cycle sleep window ends the cycle (DS §6.3.8 — an NSS falling + * edge terminates the loop). Best-effort, not airtight: the chip can + * re-enter sleep between this read and the command below. That is why + * the re-arm in lr20xx_apply_rx_duty_cycle() has to be able to recover + * rather than this guard having to be perfect — same division of labour + * as sx126x_get_rssi_inst and its RxTimeout re-arm. + * + * -128 is the busy/contended sentinel the sampler understands + * (LoRaRadioBase::triggerNoiseFloorCalibrate retries on the next tick); + * returning a plausible-looking number would feed the floor EMA a + * reading nobody took. */ + if (lr20xx_is_chip_busy(dev)) { + return -128; + } + if (k_mutex_lock(&data->spi_mutex, K_NO_WAIT) != 0) { + return -128; + } + lr20xx_radio_common_get_rssi_inst(&data->hal_ctx, &rssi, &half_dbm); k_mutex_unlock(&data->spi_mutex); @@ -1369,6 +1503,18 @@ bool lr20xx_is_receiving(const struct device *dev) return false; } + /* Sleep-window guard, and the one that matters most: this path is the + * TX gate and runs on every send attempt, far more often than the + * 15 s probe. Asking a sleeping chip "are you receiving?" would end + * the cycle being asked about (DS §6.3.8 — an NSS falling edge + * terminates the loop, and every SPI transaction asserts NSS). A + * sleeping chip is also by definition not mid-packet, so not-receiving + * is the truthful answer, not just the safe one. CAD still gates TX, + * so the channel check is not lost. Same guard the LR11xx applies. */ + if (lr20xx_is_chip_busy(dev)) { + return false; + } + /* Use non-destructive get_status to check for preamble/header * without racing the DIO1 work handler. */ if (k_mutex_lock(&data->spi_mutex, K_NO_WAIT) != 0) { @@ -1770,10 +1916,12 @@ static int lr20xx_lora_recv_duty_cycle(const struct device *dev, data->dc_rx_ms = rx_ms; data->dc_sleep_ms = slp_ms; data->rx_duty_cycle_enabled = true; - data->hal_ctx.auto_sleeps = true; - lr20xx_radio_common_set_rx_duty_cycle(ctx, rx_ms, slp_ms, - LR20XX_RADIO_COMMON_RX_DUTY_CYCLE_MODE_RX); - LOG_INF("recv_duty_cycle: rx=%ums sleep=%ums", rx_ms, slp_ms); + /* Arm through the shared helper so the cycle_time conversion lives in + * exactly one place — this site and the re-arm path had to agree, and + * two copies of that conversion is how they would stop agreeing. */ + lr20xx_apply_rx_duty_cycle(data); + LOG_INF("recv_duty_cycle: rx=%ums sleep=%ums (cycle=%ums)", + rx_ms, slp_ms, rx_ms + slp_ms); lr20xx_system_clear_irq_status(ctx, LR20XX_SYSTEM_IRQ_ALL_MASK); data->in_rx_mode = true; @@ -1792,6 +1940,8 @@ static int lr20xx_hw_init(struct lr20xx_data *data, LOG_INF("LR20xx hardware init starting"); + data->irq_dio = cfg->irq_dio; + lr20xx_system_version_t ver; bool found = false; @@ -1855,7 +2005,7 @@ static int lr20xx_hw_init(struct lr20xx_data *data, ver.major, ver.minor); } - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "post-reset"); + DUMP_CHIP_STATE(data, "post-reset"); /* SIMO DC-DC workaround REMOVED — datasheet §22.6 says it's only * needed when SetRegMode simo_usage=0x02 (SIMO_NORMAL). @@ -1897,7 +2047,7 @@ static int lr20xx_hw_init(struct lr20xx_data *data, st = lr20xx_radio_common_set_rx_tx_fallback_mode(ctx, LR20XX_RADIO_FALLBACK_STDBY_RC); - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "pre-cal"); + DUMP_CHIP_STATE(data, "pre-cal"); lr20xx_system_clear_errors(ctx); lr20xx_system_clear_irq_status(ctx, LR20XX_SYSTEM_IRQ_ALL_MASK); @@ -1918,7 +2068,7 @@ static int lr20xx_hw_init(struct lr20xx_data *data, } } - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "post-cal"); + DUMP_CHIP_STATE(data, "post-cal"); /* The 32 MHz reference is what calibration needs, so this is where a * wrong clock source shows up. One retry only — if XTAL fails too, the @@ -1941,7 +2091,7 @@ static int lr20xx_hw_init(struct lr20xx_data *data, * raw_value = ceil(868000000/4000000) = 217 = 0x00D9 */ st = lr20xx_calibrate_front_end(ctx, 868000000); - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "post-FEcal"); + DUMP_CHIP_STATE(data, "post-FEcal"); /* Verify: set packet type to LoRa and read it back */ st = lr20xx_radio_common_set_pkt_type(ctx, LR20XX_RADIO_COMMON_PKT_TYPE_LORA); @@ -1951,7 +2101,7 @@ static int lr20xx_hw_init(struct lr20xx_data *data, lr20xx_radio_common_pkt_type_t pkt_readback = 0xFF; lr20xx_radio_common_get_pkt_type(ctx, &pkt_readback); - DUMP_CHIP_STATE(ctx, &data->hal_ctx, "init-done"); + DUMP_CHIP_STATE(data, "init-done"); #if IS_ENABLED(CONFIG_LOG) /* Does merely polling status raise CmdError? get_status is a bare diff --git a/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.h b/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.h index 5e3de6a..76f08ac 100644 --- a/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.h +++ b/zephcore/patches/zephyr-new/drivers/lora/lr20xx/lr20xx_lora.h @@ -45,6 +45,18 @@ bool lr20xx_is_receiving(const struct device *dev); */ void lr20xx_set_rx_boost(const struct device *dev, bool enable); +/** + * @brief GPIO-only check for "chip is in a duty-cycle sleep window" (no SPI) + * + * True only while an RX duty cycle is armed and BUSY is high. Deliberately + * not raw BUSY: this chip holds BUSY high during continuous RX too, so a raw + * reading would report the radio permanently unavailable. + * + * @param dev LoRa device + * @return true if the host should not issue commands right now + */ +bool lr20xx_is_chip_busy(const struct device *dev); + /** * @brief Configure LoRa side detectors (multi-SF receive) *