Trim comments

esphome/components/uart/software_serial_rx_decoder.h

@@ -7,29 +7,20 @@
 
 namespace esphome::uart {
 
-/// Bit timing decoder for a software serial RX pin.
-///
-/// on_edge() is fed from the pin change interrupt with the cycle count of the edge
-/// and the level the line changed to; it counts how many bit times the previous
-/// level lasted and runs them through a start/data/parity/stop state machine,
-/// so the interrupt never waits for the line. Decoded bytes land in a ring buffer.
-/// A byte whose trailing bits are idle high has no closing edge; the main loop
-/// completes it with finalize() once enough time has passed.
-///
-/// Kept free of platform headers so it can be unit tested on the host. The hot
-/// methods are force inlined so the platform ISR that calls them stays in IRAM.
+/// Bit timing decoder for a software serial RX pin: on_edge() decodes from the
+/// time between edges so the ISR never waits for the line; a byte with an idle
+/// high tail has no closing edge and is completed by finalize() from the loop.
+/// Platform free for host tests; hot methods force inlined to stay in IRAM.
 class SoftwareSerialRxDecoder {
  public:
   static constexpr uint8_t RX_IDLE = 0xFF;
 
-  /// Configure the framing and buffer. Drops buffered bytes and any partial frame and
-  /// assumes an idle high line; call reset() afterwards with the real line level.
+  /// Configure framing and buffer; drops all state. Follow with reset().
   void setup(uint32_t bit_cycles, uint8_t data_bits, bool parity, uint8_t stop_bits, uint8_t *buffer,
              size_t buffer_size) {
     this->bit_cycles_ = bit_cycles;
     this->data_bits_ = data_bits;
     this->stop_bit_ = data_bits + (parity ? 1 : 0);
-    // Runs longer than a whole frame plus one bit are idle; cap them there.
     this->max_run_cycles_ = bit_cycles * (this->stop_bit_ + stop_bits + 2);
     this->buffer_ = buffer;
     this->buffer_size_ = buffer_size;
@@ -46,18 +37,15 @@ class SoftwareSerialRxDecoder {
     this->last_edge_ = now;
   }
 
-  /// ISR: the line changed to `level` at cycle `now`.
-  /// Returns true when the main loop should be woken: a byte was pushed, or a frame
-  /// is open and the line is high, in which case finalize() may be needed. That is
-  /// deliberately conservative; a data 1 and the idle tail are indistinguishable at
-  /// the edge, and repeat wakes are cheap because the wake flag is already set.
+  /// ISR: the line changed to `level` at cycle `now`. Returns true to wake the
+  /// loop: a byte was pushed, or a frame is open with the line high (finalize()
+  /// may be needed; a data 1 and the idle tail look the same at the edge).
   bool ESPHOME_ALWAYS_INLINE on_edge(uint32_t now, bool level) {
     const bool last_level = this->last_level_;
-    // Two edges collapsed into one interrupt: skip it so the run is still
-    // measured from the last real edge and the frame stays aligned.
+    // Collapsed edges: skip to keep the frame aligned to the last real edge.
     if (level == last_level)
       return false;
-    // Bits since the last edge, rounded to nearest; no hardware divider on the LX106, so count.
+    // Bits since the last edge, rounded; LX106 has no divider, so count.
     uint32_t delta = now - this->last_edge_;
     if (delta > this->max_run_cycles_)
       delta = this->max_run_cycles_;
@@ -73,10 +61,10 @@ class SoftwareSerialRxDecoder {
     return pushed || (level && this->bit_ != RX_IDLE);
   }
 
-  /// True while a frame is open and the line sits idle high, so a byte may be waiting on finalize().
+  /// A frame is open with the line idle high: a byte may be waiting on finalize().
   bool pending() const { return this->bit_ != RX_IDLE && this->last_level_; }
 
-  /// Cheap unlocked check whether the pending byte's tail has elapsed by `now`.
+  /// Unlocked check whether the pending byte's tail has elapsed.
   bool finalize_due(uint32_t now) const {
     const uint8_t bit = this->bit_;
     if (bit == RX_IDLE)
@@ -92,7 +80,7 @@ class SoftwareSerialRxDecoder {
     this->consume_run_(this->stop_bit_ + 1 - bit, true);
   }
 
-  /// Store a decoded byte, dropping it when the buffer is full. Also used by the start bit sampler.
+  /// Store a byte, dropping it when full. Also used by the start bit sampler.
   bool ESPHOME_ALWAYS_INLINE push_byte(uint8_t data) {
     size_t in = this->in_pos_;
     size_t next = in + 1;
@@ -127,19 +115,18 @@ class SoftwareSerialRxDecoder {
   }
 
  protected:
-  /// Cycles the line must stay high after the last edge to hold every bit up to the first stop bit.
+  /// Cycles of high line needed to finish the frame through the first stop bit.
   uint32_t tail_cycles_(uint8_t bit) const {
     return (this->stop_bit_ + 1 - bit) * this->bit_cycles_ + this->bit_cycles_ / 2;
   }
 
-  /// Feed `bits` consecutive bits at `level` into the frame. Returns true when a byte was pushed.
+  /// Feed `bits` bits at `level` into the frame; true when a byte was pushed.
   bool ESPHOME_ALWAYS_INLINE consume_run_(uint32_t bits, bool level) {
     uint8_t bit = this->bit_;
     uint8_t cur = this->cur_byte_;
     bool pushed = false;
     while (bits > 0) {
       if (bit == RX_IDLE) {
-        // Idle line, or what is left of a run after a framing error.
         if (level)
           break;
         // Start bit
@@ -159,7 +146,7 @@ class SoftwareSerialRxDecoder {
         bit++;
         bits--;
       } else {
-        // Stop bit; a low level here is a framing error, drop the byte.
+        // Stop bit; low is a framing error, drop the byte.
         if (level)
           pushed = this->push_byte(cur);
         bit = RX_IDLE;
@@ -179,7 +166,7 @@ class SoftwareSerialRxDecoder {
   size_t buffer_size_{0};
   volatile size_t in_pos_{0};
   volatile size_t out_pos_{0};
-  /// Index of the next frame bit after the start bit (data, then parity, then stop at stop_bit_) or RX_IDLE.
+  /// Next frame bit (data, parity, then stop at stop_bit_) or RX_IDLE.
   volatile uint8_t bit_{RX_IDLE};
   volatile uint8_t cur_byte_{0};
   volatile bool last_level_{true};

esphome/components/uart/uart_component_esp8266.cpp

@@ -15,8 +15,7 @@
 namespace esphome::uart {
 
 static const char *const TAG = "uart";
-// Edge decoder up to this baud rate; above it the start bit sampler, whose
-// whole-byte block in the ISR is short there and whose timing still holds up.
+// Edge decoder up to this baud rate, start bit sampler above it.
 static constexpr uint32_t SW_SERIAL_EDGE_MODE_MAX_BAUD = 38400;
 bool ESP8266UartComponent::serial0_in_use = false;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
 
@@ -239,7 +238,7 @@ UARTFlushResult ESP8266UartComponent::flush() {
 void ESP8266SoftwareSerial::setup(InternalGPIOPin *tx_pin, InternalGPIOPin *rx_pin, uint32_t baud_rate,
                                   uint8_t stop_bits, uint32_t data_bits, UARTParityOptions parity,
                                   size_t rx_buffer_size) {
-  // load_settings() re-enters here: stop the RX interrupt before touching anything it reads.
+  // load_settings() re-enters here: detach the ISR before touching its state.
   if (this->gpio_rx_pin_ != nullptr)
     this->gpio_rx_pin_->detach_interrupt();
   this->bit_time_ = F_CPU / baud_rate;
@@ -315,8 +314,7 @@ void IRAM_ATTR ESP8266SoftwareSerial::gpio_intr_edge(ESP8266SoftwareSerial *arg)
 void ESP8266SoftwareSerial::rx_finalize_pending_() {
   if (!this->rx_.finalize_due(arch_get_cpu_cycle_count())) {
 #ifdef USE_UART_WAKE_LOOP_ON_RX
-    // Not old enough yet: once the caller has drained what is there, run the loop
-    // again right away instead of after a full loop_interval_.
+    // Byte not old enough yet: re-run the loop once the buffer is drained.
     if (this->rx_.available() == 0)
       wake_loop_threadsafe();
 #endif

tests/components/uart/test_software_serial_rx_decoder.cpp

@@ -14,9 +14,8 @@ namespace {
 // 80 MHz ESP8266 clock
 constexpr uint32_t CPU_HZ = 80000000;
 
-// Drives a SoftwareSerialRxDecoder with a simulated line: frames are turned into
-// edges at their ideal cycle counts (plus optional jitter), and the main loop is
-// emulated by poll(), which finalizes a pending byte and drains the buffer.
+// Simulated line: frames become edges at ideal cycle counts plus jitter;
+// poll() emulates the main loop (finalize pending byte, drain buffer).
 class LineSim {
  public:
   LineSim(uint32_t baud, uint8_t data_bits, bool parity, bool odd, uint8_t stop_bits, size_t buffer_size = 64)
@@ -45,8 +44,7 @@ class LineSim {
       this->received_.push_back(this->dec_.read_byte());
   }
 
-  // Emit one frame starting at cycle `start`; `jitter` is the per edge offset in cycles
-  // (positive or negative) supplied by the caller. Returns the cycle at which the frame ends.
+  // Emit one frame at `start` with per edge `jitter` in cycles; returns the end cycle.
   uint32_t send(
       uint8_t value, uint32_t start, const std::function<int32_t()> &jitter = [] { return 0; }) {
     std::vector<bool> bits;
@@ -123,8 +121,7 @@ TEST(SoftwareSerialRxDecoder, DecodesBackToBackFramesAtCommonBaudRates) {
 }
 
 TEST(SoftwareSerialRxDecoder, ToleratesEdgeJitterUpToAQuarterBit) {
-  // Consecutive edges can each be off by up to a quarter bit in either direction,
-  // so the measured run length is within half a bit of the true one.
+  // A quarter bit per edge keeps each run within the half bit rounding budget.
   run_stream({9600, 8, false, false, 1}, 0.24, 0, 2000, 1);
   run_stream({9600, 8, false, false, 1}, 0.24, 7, 2000, 2);
   run_stream({38400, 8, false, false, 1}, 0.24, 4, 2000, 3);
@@ -141,11 +138,10 @@ TEST(SoftwareSerialRxDecoder, AllOnesByteCompletesOnlyByFinalize) {
   LineSim sim(9600, 8, false, false, 1);
   const uint32_t bit = sim.bit_cycles();
   uint32_t t = 5000;
-  // Start bit, then the line goes high and stays there: 0xFF has no closing edge.
+  // 0xFF: start bit, then the line stays high with no closing edge.
   sim.edge(t, false);
   sim.edge(t + bit, true);
   EXPECT_TRUE(sim.decoder().pending());
-  // Eight data bits plus the stop bit must elapse after the rising edge.
   sim.poll(t + bit + 8 * bit);
   EXPECT_TRUE(sim.received().empty());
   EXPECT_FALSE(sim.decoder().finalize_due(t + bit + 8 * bit));
@@ -191,9 +187,7 @@ TEST(SoftwareSerialRxDecoder, BreakConditionIsDroppedAndResyncs) {
 TEST(SoftwareSerialRxDecoder, CollapsedEdgeIsIgnoredAndStreamRealignsAtIdle) {
   LineSim sim(9600, 8, false, false, 1);
   const uint32_t bit = sim.bit_cycles();
-  // 0x31 = 0b00110001: start, 1, 0, 0, 0, 1, 1, 0, 0, stop. Lose the rising edge
-  // of data bit 4 (ISR delayed past two edges), so the next edge arrives at the
-  // same level as the last one the decoder saw.
+  // 0x31: lose the rising edge of bit 4, so the next edge repeats the last level.
   uint32_t t = 5000;
   sim.edge(t, false);                         // start
   sim.edge(t + 1 * bit, true);                // bit0 = 1
@@ -220,8 +214,7 @@ TEST(SoftwareSerialRxDecoder, DropsBytesWhenBufferIsFullAndKeepsOldest) {
 }
 
 TEST(SoftwareSerialRxDecoder, SetupAgainDropsStaleStateAndUsesNewBufferAndFraming) {
-  // Mirrors load_settings(): bytes buffered and a frame left open under 8N1 in a
-  // 64 byte buffer, then setup() again with 5E2 in a 4 byte buffer.
+  // Mirrors load_settings(): buffered 8N1 bytes and an open frame, then setup() as 5E2.
   LineSim sim(9600, 8, false, false, 1);
   uint32_t t = 5000;
   for (int n = 0; n < 10; n++)
@@ -237,8 +230,7 @@ TEST(SoftwareSerialRxDecoder, SetupAgainDropsStaleStateAndUsesNewBufferAndFramin
   EXPECT_FALSE(dec.pending());
   EXPECT_EQ(dec.read_byte(), 0);
 
-  // 5E2 frames fed straight into the reconfigured decoder: capacity is 3, the
-  // rest are dropped and nothing is written past the end of the new buffer.
+  // Capacity of the 4 byte buffer is 3; the spare slot must stay untouched.
   auto send_5e2 = [&](uint8_t value, uint32_t start) {
     bool line = true;
     uint32_t at = start;