From 8faafcebb0f09c1bd4c31bcbf764235be3da7905 Mon Sep 17 00:00:00 2001 From: Tom Harkness Date: Sat, 3 Oct 2026 11:30:25 +1000 Subject: [PATCH] PM5 CEP: recover from AT32 SPI1 errata, bound all SPI waits Problem With PR #3672/#3674 landed, CEP handshakes reliably but the SPI transport hangs PM5 hard (needs a physical reset, no watchdog on this platform) after some number of attach/reconnect cycles - sometimes the first cycle, sometimes dozens of pings in. Logic-analyzer capture of a hang in progress showed a ~1.2s burst of rapid CS-gated retries, and in one capture, three periodic transactions after that burst got progressively slower (2.1ms -> 3.9ms -> 3.2ms -> 5.2ms) instead of recovering. Root causes (two, compounding) 1. cep_spi_write_sync() had seven bare `while (flag == RESET);` spins waiting on SPI1's TDBE/BF flags, with no timeout. PM5 is the SPI slave - TDBE only clears when the Flipper is actively clocking, on its own ~20-120ms schedule, completely async to when PM5's reply is ready. Queue a reply between two master polls and the wait never returns - full main-loop death. 2. Artery's own errata sheet for the AT32F435/437 (ES0003) documents that in SPI slave hardware-CS mode, the internal clock sync is not guaranteed to reset on every CS falling edge - a bad sync can persist across transactions with no self-correction. This is PM5's exact CEP configuration (SPI_CS_HARDWARE_MODE, slave mode). Change armsrc/pm5_cep.c: - cep_spi_write_sync() rewritten around a bounded cep_spi_wait_tdbe() helper (same spin-iteration-count pattern already used for reads) - returns PM3_EIO on timeout instead of hanging. - New cep_spi_resync(): hardware-resets SPI1 via crm_periph_reset() (a register re-init alone doesn't clear the errata's bad internal state) and brings it back up. This is Artery's own documented recovery for ES0003 ("reset SPI and restart communication"), triggered by our existing NG-frame CRC/length checks instead of the SPI peripheral's hardware CRC feature. - cep_do_handshake()'s SPI-stall recovery path was itself unbounded (retried forever as long as the CC controller reported "still attached") and missing the StartTicks()/I2C_init(true) guard every other I2C call site in this file has (same bug class as #3672) - now bounded (CEP_HANDSHAKE_REPLY_MAX_RETRIES) with the guard added. - cep_spi_data_available() now discards the Flipper's 0x55 idle-poll filler bytes before reading the real length header, instead of ever interpreting one as (part of) a frame length. armsrc/cmd.c: wire cep_spi_resync() into every CEP-reachable error exit in receive_ng_internal() (short/overflow/CRC-mismatch reads) and both reply_old()/reply_ng_internal()'s write-failure paths, so any detected corruption resets SPI1 before the next transaction rather than letting a bad sync persist. Add a WITH_CEP-gated inline stub for cep_spi_resync() so receive_ng_internal()'s call sites don't need per-site #ifdef guards on non-CEP platforms. Behaviour change None for USB/FPC/BWM. CEP: a corrupted or stalled SPI1 transaction now self-heals (one dropped reply, then normal operation) instead of wedging the device. Testing - make clean && make PLATFORM=PM5 PLATFORM_EXTRAS=BWM and make clean && make PLATFORM=PM3RDV4 - both zero warnings. - Hardware: flashed to a real PM5 paired with a real Flipper Zero FAP. - 60s continuous-connection capture with zero USB activity (isolates CEP from any client-side interaction): clean, no anomalies. - 110s capture with 18 real `hw status` calls interspersed: all 18 succeeded, CEP stayed attached throughout. - 5 full attach/detach cycles with `hf search` against a real NTAG213 card before/during/after each attach (240s, logic-analyzer capture throughout): all 15 reads returned the identical UID regardless of CEP state, zero hangs, final liveness check succeeded with no reset needed. - Not tested: clang toolchain (firmware-only diff). Checklist - [x] Proper style of own code, no unrelated reformatting included - [x] Builds warning-free with gcc (PM5 and RDV4) - [ ] clang not tried (firmware-only diff) Co-Authored-By: Claude Sonnet 5 --- armsrc/cmd.c | 21 ++++++++- armsrc/pm5_cep.c | 109 ++++++++++++++++++++++++++++++++++++++++------- armsrc/pm5_cep.h | 11 ++++- 3 files changed, 123 insertions(+), 18 deletions(-) diff --git a/armsrc/cmd.c b/armsrc/cmd.c index 7e9edb1e3..a2d210a0d 100644 --- a/armsrc/cmd.c +++ b/armsrc/cmd.c @@ -22,6 +22,11 @@ #endif #ifdef WITH_CEP #include "pm5_cep.h" +#else +// receive_ng_internal()'s `cep` parameter is always false without WITH_CEP, +// so this is never actually called - just keeps those call sites free of +// per-site #if defined(WITH_CEP) guards. +static inline void cep_spi_resync(void) {} #endif #include "crc16.h" #include "string.h" @@ -91,6 +96,9 @@ int reply_old(uint64_t cmd, uint64_t arg0, uint64_t arg1, uint64_t arg2, const v if (g_reply_via_cep) { #if defined(WITH_CEP) resultcep = cep_spi_write_sync((uint8_t *)&txcmd, sizeof(PacketResponseOLD)); + if (resultcep != PM3_SUCCESS) { + cep_spi_resync(); + } #else return PM3_EDEVNOTSUPP; #endif @@ -173,6 +181,9 @@ static int reply_ng_internal(uint16_t cmd, int8_t status, uint8_t reason, const if (g_reply_via_cep) { #if defined(WITH_CEP) resultcep = cep_spi_write_sync((uint8_t *)tx, txBufferNGLen); + if (resultcep != PM3_SUCCESS) { + cep_spi_resync(); + } #else return PM3_EDEVNOTSUPP; #endif @@ -214,6 +225,7 @@ static int receive_ng_internal(PacketCommandNG *rx, uint32_t read_ng(uint8_t *da } if (bytes != sizeof(PacketCommandNGPreamble)) { + if (cep) cep_spi_resync(); return PM3_EIO; } @@ -225,27 +237,33 @@ static int receive_ng_internal(PacketCommandNG *rx, uint32_t read_ng(uint8_t *da if (rx->magic == COMMANDNG_PREAMBLE_MAGIC) { // New style NG command if (length > PM3_CMD_DATA_SIZE) { + if (cep) cep_spi_resync(); return PM3_EOVFLOW; } // Get the core and variable length payload bytes = read_ng((uint8_t *)&rx_raw.data, length); if (bytes != length) { + if (cep) cep_spi_resync(); return PM3_EIO; } // Get the postamble bytes = read_ng((uint8_t *)&rx_raw.foopost, sizeof(PacketCommandNGPostamble)); if (bytes != sizeof(PacketCommandNGPostamble)) { + if (cep) cep_spi_resync(); return PM3_EIO; } - // Check CRC, accept MAGIC as placeholder + // Check CRC, accept MAGIC as placeholder - a mismatch here is the + // AT32 errata's own recommended trigger to reset SPI (see + // cep_spi_resync()'s comment). rx->crc = rx_raw.foopost.crc; if (rx->crc != COMMANDNG_POSTAMBLE_MAGIC) { uint8_t first, second; compute_crc(CRC_14443_A, (uint8_t *)&rx_raw, sizeof(PacketCommandNGPreamble) + length, &first, &second); if ((first << 8) + second != rx->crc) { + if (cep) cep_spi_resync(); return PM3_EIO; } } @@ -264,6 +282,7 @@ static int receive_ng_internal(PacketCommandNG *rx, uint32_t read_ng(uint8_t *da } else { uint64_t arg[3] = {0}; if (length < sizeof(arg)) { + if (cep) cep_spi_resync(); return PM3_EIO; } diff --git a/armsrc/pm5_cep.c b/armsrc/pm5_cep.c index d3a976f3f..ac9f425dc 100644 --- a/armsrc/pm5_cep.c +++ b/armsrc/pm5_cep.c @@ -47,6 +47,13 @@ #define CEP_SPI_BYTE_TIMEOUT 100000 // spin-wait iterations per byte (matches source) #define CEP_SPI_RESPONSE_TIMEOUT_US (1000 * 1000) // 1s, matches source #define CEP_HANDSHAKE_RX_TIMEOUT_MS 2000 // bail out of the receive loop rather than ever hang the main loop +#define CEP_HANDSHAKE_REPLY_MAX_RETRIES 3 // bound the stall-recovery loop below - still-attached isn't still-working forever + +// 0x55 filler the Flipper sends while its own SPI poll finds nothing new - +// discard it so the real length header is recognized as soon as it starts, +// instead of ever reading a 0x55 byte as (part of) a frame length. +#define CEP_SYNC_BYTE 0x55 +#define CEP_SYNC_MAX_DISCARD 600 static bool s_cep_active = false; // handshake completed, SPI transport live @@ -155,6 +162,20 @@ static void cep_spi_init(void) { spi_enable(SPI1, TRUE); } +// AT32F435/437 errata ES0003, "CS falling edge not synchronized in SPI +// slave hardware CS mode": the peripheral's internal clock sync isn't +// guaranteed to reset on every CS edge, so a bad sync can persist across +// transactions. Artery's documented recovery is to detect the corruption +// (we already do, via the NG frame's CRC) and reset SPI before retrying - +// a register re-init alone isn't enough, the peripheral needs resetting +// through the clock/reset controller to actually clear that state. +void cep_spi_resync(void) { + spi_enable(SPI1, FALSE); + crm_periph_reset(CRM_SPI1_PERIPH_RESET, TRUE); + crm_periph_reset(CRM_SPI1_PERIPH_RESET, FALSE); + cep_spi_init(); +} + void cep_init(void) { cep_usart_init(); cep_spi_init(); @@ -208,13 +229,23 @@ static bool cep_do_handshake(void) { uint8_t response[] = {0x04, 0x00, 'y', 'e', 's', 0x00}; for (size_t i = 0; i < sizeof(response); i++) { uint32_t wait_start = GetTicks(); + int recovery_attempts = 0; while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET) { if (GetTicks() - wait_start > CEP_SPI_RESPONSE_TIMEOUT_US) { + // Bound this: "still attached" isn't "still working" forever, + // and I2C_BufferReadRaw() needs the ticks timer running like + // every other call site (see cep_attach_poll()'s comment). + if (++recovery_attempts > CEP_HANDSHAKE_REPLY_MAX_RETRIES) { + return false; + } + StartTicks(); + I2C_init(true); uint8_t cc_ctrl_data; bool ret = I2C_BufferReadRaw(&cc_ctrl_data, 1, CEP_CC_STATUS_REG, CEP_CC_CONTROLLER_ADDR << 1); - if (ret && ((cc_ctrl_data >> 6 & 0x03) == 0)) { - return false; // disconnected mid-reply + if (!ret || ((cc_ctrl_data >> 6 & 0x03) == 0)) { + return false; // disconnected mid-reply, or can't tell - bail either way } + wait_start = GetTicks(); } } spi_i2s_data_transmit(SPI1, response[i]); @@ -277,16 +308,33 @@ void cep_attach_poll(void) { // SPI NG-frame transport (verbatim from the unit test - already correct) // --------------------------------------------------------------------------- +static bool cep_spi_read_byte(uint8_t *out) { + uint64_t timeout = 0; + while (spi_i2s_flag_get(SPI1, SPI_I2S_RDBF_FLAG) == RESET) { + if (timeout++ > CEP_SPI_BYTE_TIMEOUT) { + return false; + } + } + *out = spi_i2s_data_receive(SPI1); + return true; +} + bool cep_spi_data_available(void) { uint8_t len_header[2] = {0x00}; - for (size_t i = 0; i < sizeof(len_header); ++i) { - uint64_t timeout = 0; - while (spi_i2s_flag_get(SPI1, SPI_I2S_RDBF_FLAG) == RESET) { - if (timeout++ > CEP_SPI_BYTE_TIMEOUT) { - return false; - } + size_t discarded = 0; + while (1) { + if (!cep_spi_read_byte(&len_header[0])) { + return false; } - len_header[i] = spi_i2s_data_receive(SPI1); + if (len_header[0] != CEP_SYNC_BYTE) { + break; + } + if (++discarded > CEP_SYNC_MAX_DISCARD) { + return false; + } + } + if (!cep_spi_read_byte(&len_header[1])) { + return false; } uint16_t data_len = (len_header[1] << 8) | len_header[0]; if (data_len > PM3_CMD_DATA_SIZE * 2) { @@ -308,25 +356,56 @@ uint32_t cep_spi_read_ng(uint8_t *data, size_t len) { return len; } +// PM5 is the SPI slave: TDBE only clears when the master is actively +// clocking, which happens on the Flipper's own ~20-120ms poll schedule, +// completely async to when PM5 has a reply ready. An unbounded wait here +// hangs the main loop forever (no recovery, not even a watchdog reset) the +// moment a reply is queued between two master polls - bound every wait so +// a slow/absent master degrades to a dropped reply, not a dead device. +static bool cep_spi_wait_tdbe(void) { + uint64_t timeout = 0; + while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET) { + if (timeout++ > CEP_SPI_BYTE_TIMEOUT) { + return false; + } + } + return true; +} + int cep_spi_write_sync(uint8_t *data, size_t len) { - while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET); + if (!cep_spi_wait_tdbe()) { + return PM3_EIO; + } spi_i2s_data_transmit(SPI1, len & 0xFF); - while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET); + if (!cep_spi_wait_tdbe()) { + return PM3_EIO; + } spi_i2s_data_transmit(SPI1, (len >> 8) & 0xFF); for (size_t i = 0; i < len; ++i) { - while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET); + if (!cep_spi_wait_tdbe()) { + return PM3_EIO; + } spi_i2s_data_transmit(SPI1, data[i]); } // Two trailing zero bytes: the agreed "no more data" idle marker. - while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET); + if (!cep_spi_wait_tdbe()) { + return PM3_EIO; + } spi_i2s_data_transmit(SPI1, 0x00); - while (spi_i2s_flag_get(SPI1, SPI_I2S_TDBE_FLAG) == RESET); + if (!cep_spi_wait_tdbe()) { + return PM3_EIO; + } spi_i2s_data_transmit(SPI1, 0x00); - while (spi_i2s_flag_get(SPI1, SPI_I2S_BF_FLAG) == SET); + uint64_t timeout = 0; + while (spi_i2s_flag_get(SPI1, SPI_I2S_BF_FLAG) == SET) { + if (timeout++ > CEP_SPI_BYTE_TIMEOUT) { + return PM3_EIO; + } + } return PM3_SUCCESS; } diff --git a/armsrc/pm5_cep.h b/armsrc/pm5_cep.h index fafbd33d1..d040bd1ef 100644 --- a/armsrc/pm5_cep.h +++ b/armsrc/pm5_cep.h @@ -56,8 +56,15 @@ bool cep_spi_data_available(void); uint32_t cep_spi_read_ng(uint8_t *data, size_t len); // Write `len` raw NG bytes (a whole PacketResponseNG/OLD frame) out over -// SPI1 as one length-prefixed packet. Always returns PM3_SUCCESS today (the -// underlying spi_i2s_data_transmit() calls are blocking-until-ready). +// SPI1 as one length-prefixed packet. Returns PM3_EIO if the master isn't +// clocking within CEP_SPI_BYTE_TIMEOUT at any point - this can legitimately +// happen (PM5's reply is ready on its own schedule, not the master's), the +// caller just loses this one reply rather than hanging forever. int cep_spi_write_sync(uint8_t *data, size_t len); +// Hardware-reset SPI1 and bring it back up (see AT32F435/437 errata ES0003 +// in pm5_cep.c). Call on any CEP frame error so a corrupted CS-edge sync +// can't persist into the next transaction. +void cep_spi_resync(void); + #endif // __PM5_CEP_H