Skip to content

fix: retain UART readline newline terminators - #319

Open
YashAnand69 wants to merge 2 commits into
fossasia:mainfrom
YashAnand69:fix/busio-uart-readline-newline
Open

YashAnand69 wants to merge 2 commits into
fossasia:mainfrom
YashAnand69:fix/busio-uart-readline-newline

Conversation

@YashAnand69

@YashAnand69 YashAnand69 commented Oct 12, 2026 •

Copy link
Copy Markdown

Fixes #318.

UART.readline() consumed LF before adding it to the returned bytes. A blank line therefore returned None, indistinguishable from a timeout, and CRLF lines lost their LF. Append the byte before checking for the line terminator, preserving complete line framing without consuming the next line.

This restores the documented CircuitPython-style newline contract. Complete lines now include LF; partial timeout results, raw reads, polling and constructor/parity handling are unchanged.

Validation: five framing regressions fail on unchanged main while five controls pass. All ten focused cases and 33 existing hardware-independent tests pass with the fix. Tests exercise the actual UART status/read command path with controlled firmware replies and clock. Configured source/test Black, Flake8, Bandit and pydocstyle, isolated wheel/sdist builds and Sphinx pass (seven existing docs warnings). Same-selection busio coverage remains 48.148%; this is not a whole-package or physical-hardware coverage claim.

Prepared with Codex assistance and reviewed against the method documentation and CircuitPython/pySerial line-reading contracts. No physical UART integration was performed. This read-path fix is separate from #317's constructor parity handling.

Summary by Sourcery

Restore complete UART line framing by retaining newline terminators while preserving timeout and raw-read behavior.

Bug Fixes:

  • Preserve LF terminators in UART line reads, including blank lines and CRLF sequences, without consuming bytes from subsequent lines.

Tests:

  • Add hardware-independent UART framing tests covering complete lines, partial timeouts, raw reads, readinto, byte limits, and zero-length reads.

Summary by CodeRabbit

  • Bug Fixes
    • UART line reads now include the terminating newline, making it easier to preserve complete lines when reading serial data.
    • Reading consecutive lines preserves each line ending, while blank lines remain distinct from cases where no data is available.
    • Reads that time out still return any data received so far, and byte-limited reads continue to respect the requested size.

@sourcery-ai

sourcery-ai Bot commented Oct 12, 2026

Copy link
Copy Markdown

Sorry @YashAnand69, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 6 hours and 57 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Oct 12, 2026

Copy link
Copy Markdown

Reviewer's Guide

Fix UART readline framing by appending bytes before checking for LF, so complete lines retain their newline terminators without consuming subsequent data. Add focused transport-level tests covering line framing, timeout semantics, raw reads, binary data, byte limits, and buffer preservation.

Sequence diagram for UART readline framing

sequenceDiagram
    participant Caller
    participant UART
    participant Transport

    Caller->>UART: readline()
    UART->>Transport: _read_byte()
    Transport-->>UART: byte
    UART->>UART: data.append(char)
    alt char == 0xA
        UART-->>Caller: complete line including LF
    else char != 0xA
        UART->>Transport: _read_byte()
        Transport-->>UART: next byte
        UART-->>Caller: line or timeout result
    end
Loading

File-Level Changes

Change Details Files
Preserve LF terminators when reading complete UART lines.
  • Append each received byte before evaluating the newline boundary.
  • Ensure blank lines return b"\n", CRLF lines retain both terminators, and the next line remains buffered.
pslab/bus/busio.py
Add hardware-independent regression coverage for UART framing and read behavior.
  • Mock the actual UART status/read command path with controlled buffering and deterministic timeout progression.
  • Cover blank, LF, CRLF, binary, partial-timeout, successive-line, raw-read, readinto, zero-length, and no-data cases.
  • Verify command counts and unread receive-buffer contents to detect over-reading.
tests/test_busio_uart_readline.py

Assessment against linked issues

Issue Objective Addressed Explanation
#318 Ensure UART.readline() retains the LF newline terminator in returned complete lines, including LF, CRLF, and binary-data lines. ✅
#318 Make blank lines such as b"\n" return the complete line rather than None, while preserving the distinction between a successful line read and a timeout. ✅
#318 Preserve existing UART read behavior outside line termination handling, including partial-line timeout results, successive-line framing, raw reads, readinto, and zero-length reads. ✅

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 69bcc38a-15b8-411f-87ea-602118183837

📥 Commits

Reviewing files that changed from the base of the PR and between 08de0d0 and 51bc003.


📒 Files selected for processing (2)
  • pslab/bus/busio.py
  • tests/test_busio_uart_readline.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



📝 Walkthrough

Walkthrough

UART._read_with_timeout now includes the received newline byte in its result. Hardware-free tests cover newline framing, partial reads, empty input, and byte-limited reads.

Changes

UART line-read behavior

Layer / File(s) Summary
Include newline bytes in UART reads
pslab/bus/busio.py, tests/test_busio_uart_readline.py
UART._read_with_timeout appends and counts each received byte before checking for a newline. Tests cover blank lines, LF and CRLF endings, successive lines, partial data on timeout, read(), readinto(), and read(0).

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low


Merge Risk | ⚪ Minimal · up to 51bc0

Merge Risk: ⚪ Minimal · up to 51bc0

UART line reads retain newline terminators. No actionable merge-blocking issue is established; merge after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 51bc0

The change restores complete line framing without expanding device access or privileges. No material security risk was found in the changed behavior; hardware integration and downstream application behavior were not validated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure change is confined to bytes returned to existing UART.readline() callers. The inspected change introduces no new entrypoint, device authority, or sensitive sink; downstream application interpretation of the retained terminator remains unvalidated.

Trust Boundaries and Controls

  • observed — The routed Clock and UARTTransport declarations are test-local helpers. Their command responses and clock substitution are applied through the pytest fixture, not added to production connection handling.

Resilience and Maintainability Implications

  • inferred — The change introduces no retained transaction state or new cleanup requirement. Read interruption and unsynchronized concurrent access remain pre-existing limitations rather than PR-introduced failure-containment concerns.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving newline terminators in UART.readline().
Linked Issues check Passed Issue #318 requires UART.readline() to retain LF, distinguish a blank LF line from timeout, preserve CRLF framing, and avoid consuming following data. The reviewed change in pslab/bus/busio.py app…
Out of Scope Changes check Passed The changes are limited to the UART read path and tests for the #318 framing contract. The tests also verify unchanged raw-read, timeout, byte-limit, and zero-length behavior. No unrelated production …

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CircuitPython UART readline drops LF and returns None for blank lines

1 participant