More robust handling and parsing of Bluetooth notification data #9
Conversation
WalkthroughBumps package version to 0.1.4 and refactors BlueLinkDevice.receive_data to add typing, return parsed status dicts (or None), perform payload length checks, log sender UUIDs, and add exception handling while still using start_notify/_TX_UUID. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BlueLinkDevice
participant BleakClient
participant Device
Caller->>BlueLinkDevice: receive_data(timeout)
BlueLinkDevice->>BleakClient: start_notify(_TX_UUID, notification_handler)
Device-->>BleakClient: Notification(bytearray)
BleakClient-->>BlueLinkDevice: notification_handler(sender, data)
BlueLinkDevice->>BlueLinkDevice: convert to bytes, check len>=9
BlueLinkDevice->>BlueLinkDevice: status = parse_status_data(bytes[4:9])
BlueLinkDevice->>BlueLinkDevice: set data_event with parsed status
BlueLinkDevice-->>Caller: return parsed status dict (or None on timeout)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pyproject.toml(1 hunks)src/neosmartblue/py/device.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/neosmartblue/py/device.py (2)
src/neosmartblue/py/parse_status.py (1)
parse_status_data(1-50)src/neosmartblue/py/scan.py (2)
detection_callback(18-42)scan_for_devices(6-48)
🔇 Additional comments (2)
pyproject.toml (1)
3-3: Version bump aligned with API change.0.1.4 makes sense given the behavioral change in receive_data. No issues spotted with the bump itself.
src/neosmartblue/py/device.py (1)
3-3: Bleak import is acceptable — no change requiredVerified: pyproject.toml pins "bleak >=1.0.1" and PyPI shows latest 1.1.0, so importing BleakGATTCharacteristic from bleak.backends.characteristic is consistent with the project's Bleak v1.x dependency.
- Location to note:
- src/neosmartblue/py/device.py —
from bleak.backends.characteristic import BleakGATTCharacteristic(used innotification_handlersignature around lines ~105–109)Short guidance: keep as-is given the dependency constraint. If you intend to support older bleak versions (<1.0) or allow looser dependency ranges, adopt the TYPE_CHECKING + Any fallback pattern suggested in the original comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/neosmartblue/py/device.py (3)
95-103: Return type + docstring clarification: LGTM; aligns with API behavior changeNice job annotating the return type and clarifying the docstring to reflect the parsed status dict (or None) behavior. This addresses the previously raised concern about the breaking change.
131-139: Wrap start_notify in the try/finally and log timeout explicitlyMinor structure tweak: placing start_notify inside the try pairs the setup/teardown more cleanly and allows a consistent timeout log.
- await self.client.start_notify(_TX_UUID, notification_handler) - try: + try: + await self.client.start_notify(_TX_UUID, notification_handler) try: await asyncio.wait_for(data_event.wait(), timeout=timeout) except asyncio.TimeoutError: - # No data within timeout; return None - pass + logger.debug("No notification received within %.1fs", timeout) return data
2-3: Consistent logging across the class (replace prints)Follow-up from earlier feedback: consider replacing print calls in connect, disconnect, and send_command with logger calls for consistency with receive_data and for better library ergonomics.
🧹 Nitpick comments (5)
src/neosmartblue/py/device.py (5)
2-3: Adopt a module-level logger for consistency and efficiencyYou already import logging; prefer defining a module-level logger once instead of creating it per-call inside receive_data.
Apply within this function (see next comment for removal), and add at module top (outside selected range):
# top-level, once per module logger = logging.getLogger(__name__)
104-107: Avoid per-call getLogger; use the module-level loggerCreate the logger at module level and remove the per-call instantiation.
- logger = logging.getLogger(__name__) data: Optional[Dict[str, Any]] = None data_event = asyncio.Event()
111-127: Reduce log noise; prefer debug level for high-frequency notifications and use clearer namingNotifications can be chatty; log at debug to avoid flooding info-level logs. Also, rename “printable” to “payload” for clarity.
- printable = bytes(received_data) - logger.info( + payload = bytes(received_data) + logger.debug( "Notification from characteristic %s: %s", getattr(sender, 'uuid', sender), - printable.hex().upper(), + payload.hex().upper(), ) - if len(printable) >= 9: - status_payload = printable[4:9] + if len(payload) >= 9: + status_payload = payload[4:9] status = parse_status_data(status_payload) data = status - logger.info("Parsed status: %s", status) + logger.debug("Parsed status: %s", status) data_event.set() else: - logger.info( + logger.debug( "Received data too short to parse status payload (len=%d)", - len(printable), + len(payload), )
140-143: Nit: include characteristic in stop-notify failure logHelps correlate failures in multi-characteristic scenarios.
- except Exception: # pragma: no cover - defensive cleanup - logger.exception("Failed to stop notifications") + except Exception: # pragma: no cover - defensive cleanup + logger.exception("Failed to stop notifications for %s", _TX_UUID)
5-5: Optional: guard BleakGATTCharacteristic import to avoid runtime dependency on internal pathbleak.backends.characteristic can shift across Bleak versions. Since it’s only used for type annotation, consider guarding with TYPE_CHECKING and a forward reference to avoid runtime import errors if Bleak internals change.
Example (outside selected range):
from typing import TYPE_CHECKING if TYPE_CHECKING: from bleak.backends.characteristic import BleakGATTCharacteristic # then annotate as: def notification_handler(sender: "BleakGATTCharacteristic", received_data: bytearray): ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/neosmartblue/py/device.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/neosmartblue/py/device.py (2)
src/neosmartblue/py/parse_status.py (1)
parse_status_data(1-50)src/neosmartblue/py/scan.py (2)
detection_callback(18-42)scan_for_devices(6-48)
🔇 Additional comments (2)
src/neosmartblue/py/device.py (2)
108-108: More precise callback signature: goodTyping the notification handler’s sender as BleakGATTCharacteristic matches Bleak’s callback signature and improves readability.
128-129: Robust exception logging inside handler: LGTMCatching and logging exceptions with logger.exception ensures tracebacks are preserved without crashing the callback.
This pull request updates the version of the
neosmartblue.pylibrary and improves thereceive_datamethod in thedevice.pyfile to provide more robust handling and parsing of Bluetooth notification data. The most important changes are grouped below:Version Update:
0.1.3to0.1.4inpyproject.tomlto reflect new changes.Bluetooth Notification Handling Improvements:
receive_datamethod indevice.pyto useBleakGATTCharacteristicfor the notification handler's sender argument, and improved the notification handler to handle and log errors gracefully, check data length before parsing, and print more informative debug messages. [1] [2]receive_datato clarify that it returns a parsed status object (fromparse_status_data) instead of raw bytes.Summary by CodeRabbit