Avoid stopping unstarted nRF UARTs during bridge wake

.github/workflows/run-unit-tests.yml

@@ -222,6 +222,7 @@ jobs:
           python3 -B test/test_rs232_runtime.py -v
           python3 -B test/test_rs232_mqtt_runtime.py -v
           python3 -B test/test_rs232_driver_lifecycle.py -v
+          python3 -B test/test_rs232_nrf_uart_lifecycle.py -v
           python3 -B test/test_rs232_heartbeat.py -v
           python3 -B test/test_rs232_heartbeat_owner.py -v
           python3 -B test/test_rs232_power_settings.py -v

src/helpers/bridges/RS232UartUtils.h

@@ -5,7 +5,10 @@ namespace bridge {
 
 template <typename UartType, typename PinType>
 inline void prepareNrfUart(UartType& uart, PinType rx, PinType tx) {
-  uart.end();
+  // Adafruit Uart::end() requires a begun peripheral and a live TX semaphore.
+  // Its public bool reports that state; a fresh or already stopped UART must
+  // not wait for stop events or delete an unallocated semaphore.
+  if (uart) uart.end();
   uart.setPins(rx, tx);
 }
 

test/fixtures/nrf52_uart_lifecycle/core_uart.inc

@@ -0,0 +1,76 @@
+/*
+  Copyright (c) 2015 Arduino LLC. All right reserved.
+  Copyright (c) 2016 Sandeep Mistry. All right reserved.
+
+  This library is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or (at
+  your option) any later version.
+
+  This library 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 Lesser General Public License
+  for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with this library; if not, write to the Free Software Foundation,
+  Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+// Frozen narrow core methods, independent of a CI package installation.
+// Source: framework-arduinoadafruitnrf52 1.10701.0, cores/nRF5/Uart.cpp.
+// Repository: https://github.com/meshcore-dev/Adafruit_nRF52_Arduino
+// Complete source SHA256:
+// 369f8847272f7884aabacce10a395099616212df614f7728931e212f3fce9df5
+// Uart.h public bool returns _begun; complete header SHA256:
+// f26b2a82c40925d5ebcdaa5806dd7767941098389d22aa55379c7ba7c190dacf
+// Constructor, pin setter and end body below are copied without changes.
+Uart::Uart(NRF_UARTE_Type *_nrfUart, IRQn_Type _IRQn, uint8_t _pinRX, uint8_t _pinTX)
+{
+  nrfUart = _nrfUart;
+  IRQn = _IRQn;
+  uc_pinRX = g_ADigitalPinMap[_pinRX];
+  uc_pinTX = g_ADigitalPinMap[_pinTX];
+  uc_hwFlow = 0;
+
+  _end_tx_sem = NULL;
+  _begun = false;
+}
+
+void Uart::setPins(uint8_t pin_rx, uint8_t pin_tx)
+{
+  uc_pinRX = g_ADigitalPinMap[pin_rx];
+  uc_pinTX = g_ADigitalPinMap[pin_tx];
+}
+
+void Uart::end()
+{
+  NVIC_DisableIRQ(IRQn);
+
+  nrfUart->INTENCLR = UARTE_INTENSET_ENDRX_Msk | UARTE_INTENSET_ENDTX_Msk;
+
+  nrfUart->EVENTS_RXTO = 0;
+  nrfUart->EVENTS_TXSTOPPED = 0;
+
+  nrfUart->TASKS_STOPRX = 0x1UL;
+  nrfUart->TASKS_STOPTX = 0x1UL;
+
+  // Wait for TXSTOPPED event and for RXTO event
+  // This is required before disabling UART to fully power down transceiver PHY.
+  // Otherwise transceiver will continue to consume ~900uA
+  while ( !(nrfUart->EVENTS_TXSTOPPED && nrfUart->EVENTS_RXTO) ) yield();
+
+  nrfUart->ENABLE = UARTE_ENABLE_ENABLE_Disabled;
+
+  nrfUart->PSEL.TXD = 0xFFFFFFFF;
+  nrfUart->PSEL.RXD = 0xFFFFFFFF;
+
+  nrfUart->PSEL.RTS = 0xFFFFFFFF;
+  nrfUart->PSEL.CTS = 0xFFFFFFFF;
+
+  rxBuffer.clear();
+
+  vSemaphoreDelete(_end_tx_sem);
+  _end_tx_sem = NULL;
+  _begun = false;
+}

test/fixtures/nrf52_uart_lifecycle/main.cpp

@@ -0,0 +1,111 @@
+#include <cassert>
+#include <cstdint>
+#include <cstdio>
+#include <stdexcept>
+#include <helpers/bridges/RS232UartUtils.h>
+
+using IRQn_Type = int;
+struct NRF_UARTE_Type {
+  uint32_t INTENCLR = 0, EVENTS_RXTO = 0, EVENTS_TXSTOPPED = 0;
+  uint32_t TASKS_STOPRX = 0, TASKS_STOPTX = 0, ENABLE = 0;
+  struct { uint32_t TXD = 0, RXD = 0, RTS = 0, CTS = 0; } PSEL;
+};
+constexpr uint32_t UARTE_INTENSET_ENDRX_Msk = 1, UARTE_INTENSET_ENDTX_Msk = 2;
+constexpr uint32_t UARTE_ENABLE_ENABLE_Disabled = 0;
+const uint32_t g_ADigitalPinMap[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+static NRF_UARTE_Type* peripheral = nullptr;
+static bool hardware_completes = false;
+static unsigned stops = 0, yields = 0, deleted = 0, null_deletes = 0;
+void NVIC_DisableIRQ(IRQn_Type) { ++stops; }
+void yield() {
+  // Disabled hardware does not promise stop events. Bound the regression
+  // instead of hanging a test process inside the real core's stop loop.
+  if (hardware_completes) {
+    peripheral->EVENTS_RXTO = peripheral->EVENTS_TXSTOPPED = 1;
+  } else if (++yields == 3) {
+    throw std::runtime_error("unstarted UART stop wait");
+  }
+}
+void vSemaphoreDelete(void* semaphore) {
+  // FreeRTOS vQueueDelete asserts a non-null queue. Even an optimistic
+  // disabled-peripheral completion cannot make end() valid before begin().
+  if (!semaphore) {
+    ++null_deletes;
+    throw std::runtime_error("FreeRTOS null semaphore assertion");
+  }
+  ++deleted;
+}
+struct Uart {
+  NRF_UARTE_Type* nrfUart = nullptr;
+  IRQn_Type IRQn = 0;
+  uint8_t uc_pinRX = 0, uc_pinTX = 0, uc_hwFlow = 0;
+  void* _end_tx_sem = nullptr;
+  bool _begun = false;
+  struct { unsigned clears = 0; void clear() { ++clears; } } rxBuffer;
+  Uart(NRF_UARTE_Type*, IRQn_Type, uint8_t, uint8_t);
+  void end();
+  void setPins(uint8_t, uint8_t);
+  // The public accessor's body is the real Uart.h contract.
+  operator bool ()
+  {
+    return _begun;
+  }
+  // UART start is an explicit I/O boundary, not an emulation of baud/DMA.
+  void startForTest() {
+    assert(!_begun && !_end_tx_sem);
+    nrfUart->ENABLE = 8;
+    nrfUart->PSEL.RXD = uc_pinRX;
+    nrfUart->PSEL.TXD = uc_pinTX;
+    _end_tx_sem = this;
+    _begun = true;
+  }
+};
+#include "core_uart.inc"
+
+int main() try {
+  NRF_UARTE_Type registers;
+  peripheral = &registers;
+  Uart uart(&registers, 1, 0, 1);
+  assert(!uart && !uart._end_tx_sem && !registers.ENABLE);
+
+  // Negative controls exercise the frozen end() precondition itself. They
+  // prove why a harmless-looking unconditional stop is not valid cleanup.
+  bool failed = false;
+  try { uart.end(); }
+  catch (const std::runtime_error&) { failed = true; }
+  assert(failed && yields == 3 && stops == 1 && !deleted && !null_deletes);
+  hardware_completes = true;
+  failed = false;
+  try { uart.end(); }
+  catch (const std::runtime_error&) { failed = true; }
+  assert(failed && stops == 2 && !deleted && null_deletes == 1);
+
+  // Fresh first enable and an already-stopped standby resume do not call
+  // end(), touch the stop IRQ, or delete a semaphore that was never created.
+  for (unsigned retry = 0; retry < 2; ++retry) {
+    mesh::bridge::prepareNrfUart(uart, uint8_t(2), uint8_t(3));
+    assert(!uart && !uart._end_tx_sem && stops == 2 && !deleted);
+    assert(null_deletes == 1 && uart.uc_pinRX == 2 && uart.uc_pinTX == 3);
+  }
+
+  // A begun UART still must finish stop before pin remapping. All four PSEL
+  // values and the semaphore reflect the actual core's complete shutdown.
+  for (unsigned cycle = 0; cycle < 100; ++cycle) {
+    uart.startForTest();
+    assert(uart && registers.ENABLE == 8 && uart._end_tx_sem);
+    const uint8_t rx = uint8_t(cycle % 4 + 2), tx = uint8_t(cycle % 4 + 6);
+    mesh::bridge::prepareNrfUart(uart, rx, tx);
+    assert(!uart && !uart._end_tx_sem && !registers.ENABLE);
+    assert(deleted == cycle + 1 && stops == cycle + 3 && null_deletes == 1);
+    assert(registers.PSEL.RXD == UINT32_MAX && registers.PSEL.TXD == UINT32_MAX);
+    assert(registers.PSEL.RTS == UINT32_MAX && registers.PSEL.CTS == UINT32_MAX);
+    assert(uart.uc_pinRX == rx && uart.uc_pinTX == tx);
+    const unsigned stopped = stops, freed = deleted;
+    mesh::bridge::prepareNrfUart(uart, rx, tx);
+    assert(!uart && stops == stopped && deleted == freed);
+  }
+  puts("PASS: nRF UART lifecycle; fresh/stopped guards and 100 running remaps");
+} catch (const std::exception& error) {
+  std::fprintf(stderr, "%s\n", error.what());
+  return 2;
+}

test/test_rs232_nrf_uart_lifecycle.py

@@ -0,0 +1,56 @@
+#!/usr/bin/env python3
+"""Exercise the real helper with frozen Adafruit UART lifecycle methods.
+
+Only register completion and semaphore deletion are host boundaries. The
+constructor, public started accessor, pin setter and stop logic preserve the
+pinned framework's semantics without downloads, PlatformIO or hardware.
+"""
+from pathlib import Path
+import shutil
+import subprocess
+import tempfile
+import unittest
+
+ROOT = Path(__file__).resolve().parents[1]
+FIXTURE = ROOT / "test/fixtures/nrf52_uart_lifecycle"
+
+
+class RS232NrfUartLifecycleTests(unittest.TestCase):
+    def compile_and_run(self, old_helper=False):
+        compiler = shutil.which("g++") or shutil.which("clang++")
+        self.assertIsNotNone(compiler, "a host C++ compiler is required")
+        with tempfile.TemporaryDirectory(prefix="rs232-nrf-uart-") as directory:
+            work = Path(directory)
+            binary = work / "test"
+            include = []
+            if old_helper:
+                source = (ROOT / "src/helpers/bridges/RS232UartUtils.h").read_text(encoding="ascii")
+                self.assertEqual(source.count("if (uart) uart.end();"), 1)
+                header = work / "helpers/bridges/RS232UartUtils.h"
+                header.parent.mkdir(parents=True)
+                header.write_text(source.replace("if (uart) uart.end();", "uart.end();"), encoding="ascii")
+                include = ["-I" + str(work)]
+            built = subprocess.run([
+                compiler, "-std=c++17", "-Wall", "-Wextra", "-Werror",
+                "-fsanitize=address,undefined", "-fno-sanitize-recover=all",
+                "-fno-pie", "-no-pie", *include, "-I" + str(ROOT / "src"),
+                str(FIXTURE / "main.cpp"), "-o", str(binary),
+            ], capture_output=True, text=True, timeout=60)
+            self.assertEqual(built.returncode, 0, built.stdout + built.stderr)
+            checked = subprocess.run([str(binary)], capture_output=True,
+                                     text=True, timeout=10)
+            return checked
+
+    def test_fresh_stopped_running_and_repeated_remapping(self):
+        checked = self.compile_and_run()
+        self.assertEqual(checked.returncode, 0, checked.stdout + checked.stderr)
+        self.assertIn("PASS: nRF UART lifecycle", checked.stdout)
+
+    def test_unconditional_stop_negative_control_reaches_null_semaphore(self):
+        checked = self.compile_and_run(old_helper=True)
+        self.assertNotEqual(checked.returncode, 0, checked.stdout + checked.stderr)
+        self.assertIn("FreeRTOS null semaphore assertion", checked.stderr)
+
+
+if __name__ == "__main__":
+    unittest.main()

test/test_rs232_uart/test_rs232_uart.cpp

@@ -6,7 +6,9 @@
 
 struct FakeUart {
   std::vector<int> calls;
-  void end() { calls.push_back(1); }
+  bool begun = false;
+  explicit operator bool() const { return begun; }
+  void end() { calls.push_back(1); begun = false; }
   void setPins(int rx, int tx) {
     calls.push_back(2);
     calls.push_back(rx);
@@ -16,12 +18,27 @@ struct FakeUart {
 
 TEST(RS232Uart, StopsPeripheralBeforeChangingPins) {
   FakeUart uart;
+  uart.begun = true;
   mesh::bridge::prepareNrfUart(uart, 7, 8);
   ASSERT_EQ(uart.calls.size(), 4u);
   EXPECT_EQ(uart.calls[0], 1);
   EXPECT_EQ(uart.calls[1], 2);
   EXPECT_EQ(uart.calls[2], 7);
   EXPECT_EQ(uart.calls[3], 8);
+  EXPECT_FALSE(uart.begun);
+}
+
+TEST(RS232Uart, FreshAndAlreadyStoppedUartsOnlyChangePins) {
+  FakeUart uart;
+  for (int attempt = 0; attempt < 2; ++attempt) {
+    mesh::bridge::prepareNrfUart(uart, 7, 8);
+    ASSERT_EQ(uart.calls.size(), 3u);
+    EXPECT_EQ(uart.calls[0], 2);
+    EXPECT_EQ(uart.calls[1], 7);
+    EXPECT_EQ(uart.calls[2], 8);
+    EXPECT_FALSE(uart.begun);
+    uart.calls.clear();
+  }
 }
 
 int main(int argc, char** argv) {