Skip to content

fix: avoid double-closing serial transport#175

Open
eburi wants to merge 1 commit into
meshtastic:mainfrom
eburi:fix-serial-double-close
Open

fix: avoid double-closing serial transport#175
eburi wants to merge 1 commit into
meshtastic:mainfrom
eburi:fix-serial-double-close

Conversation

@eburi

@eburi eburi commented Jul 3, 2026

Copy link
Copy Markdown

Summary

  • let the serial transport own shutdown of the underlying pyserial object
  • remove the direct raw serial close after StreamWriter.close()

Why

When a serial device disconnects or times out, SerialTransport.close() schedules its own async connection-lost cleanup, including flushing and closing the serial object. The integration was also immediately closing the raw pyserial object, which can leave the transport cleanup trying to flush a port that is already closed.

On Home Assistant 2026.7 this shows up as repeated log errors like:

Exception in callback SerialTransport._call_connection_lost()
serial.serialutil.PortNotOpenError: Attempting to use a port that is not open

This was observed with a CP210x-backed Meshtastic serial connection where the host kernel was also reporting cp210x ttyUSB1: failed set request 0x12 status: -110 during the same reconnect cadence.

This PR intentionally does not change the pyserial-asyncio dependency, since #161 already covers the pyserial-asyncio-fast migration.

Testing

  • .venv/bin/python -m ruff check .
  • .venv/bin/python -m ruff format . --check
  • git diff --check

Note: full requirements.txt install was not possible in this local environment because its Python version does not satisfy the pinned Home Assistant version; the checks above were run with the repo-pinned Ruff version 0.11.8.

Summary by CodeRabbit

  • Bug Fixes
    • Improved connection shutdown handling to avoid unnecessary low-level serial port closure.
    • Streamlined cleanup so disconnects rely on the standard shutdown flow, reducing the chance of lingering state issues.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e3b20db9-ee2d-493d-96b3-5811febe02d9

📥 Commits

Reviewing files that changed from the base of the PR and between 3594f35 and e6a741a.

📒 Files selected for processing (1)
  • custom_components/meshtastic/aiomeshtastic/connection/serial.py
💤 Files with no reviewable changes (1)
  • custom_components/meshtastic/aiomeshtastic/connection/serial.py

📝 Walkthrough

Walkthrough

Removed an unused typing.cast import and eliminated an explicit close of the underlying serial transport in SerialConnection._disconnect, relying solely on the StreamWriter.close() call for shutdown cleanup.

Changes

Serial Disconnect Cleanup

Layer / File(s) Summary
Remove explicit serial transport close
custom_components/meshtastic/aiomeshtastic/connection/serial.py
Removed unused cast import and the explicit cast(...).serial.close() call in _disconnect, leaving StreamWriter.close() to handle shutdown.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Poem

A close call removed, tidy and neat,
One less line for the writer to greet.
The stream still shuts with a graceful sigh,
No cast needed, no need to pry.
Hop, hop, hooray — cleaner code nearby! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: removing the extra serial transport close during disconnect.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

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.

2 participants