Skip to content

Add hwIo.ble module with unit tests#19838

Open
bramd wants to merge 5 commits intonvaccess:masterfrom
bramd:ble-tests-tdd
Open

Add hwIo.ble module with unit tests#19838
bramd wants to merge 5 commits intonvaccess:masterfrom
bramd:ble-tests-tdd

Conversation

@bramd
Copy link
Copy Markdown
Contributor

@bramd bramd commented Mar 23, 2026

Link to issue number:

N/A. This is "PR A" of the agreed split of #19122 (DotPad BLE support).

Summary of the issue:

PR #19122 adds Bluetooth Low Energy (BLE) support for DotPad braille displays. As discussed in #19122, that PR is being split into smaller, easier-to-review pieces:

  • PR A (this PR): the hwIo.ble module + its unit tests
  • PR B: tests for the DotPad BLE driver
  • PR C: bdDetect BLE integration + DotPad BLE driver implementation

This PR was previously a TDD-style "skipped tests only" PR. After feedback from @seanbudd it has been re-scoped to include the actual hwIo.ble module so the tests can be unskipped. The bdDetect-related test additions and the DotPad-specific tests have been removed; they will return in PRs C and B respectively.

Description of user facing changes:

None. hwIo.ble is a developer-facing API. End users see no changes until PRs B and C land.

Description of developer facing changes:

  • New hwIo.ble submodule providing:
    • Scanner — synchronous wrapper around Bleak's BleakScanner, with a deviceDiscovered extension point that fires on each advertisement and indicates whether the device is new.
    • BleIoBase subclass for raw I/O against a BLE peripheral via a configurable read/write GATT service-and-characteristic pair, with automatic MTU chunking on writes.
    • findDeviceByAddress(address, timeout, pollInterval) — helper that checks already-discovered devices and starts a scan if needed.
    • Module-level scanner singleton.
  • Builds on top of the existing _asyncioEventLoop module (Add private _asyncioEventLoop module #19816).
  • Adds bleak==3.0.0 as a runtime dependency.
  • No changes to bdDetect, no changes to any driver — those land in PR C.

Description of development approach:

Ported the hwIo.ble submodule and its unit tests from the dotpad-ble branch. The tests use unittest.mock.patch to mock bleak and runCoroutine, so they run without any BLE hardware or asyncio loop.

While unskipping, several issues with the existing tests were fixed:

  • They patched runCoroutineSync, but the module uses runCoroutine. Now patching the correct symbol.
  • MagicMock.__len__ was being assigned a lambda; Python looks up dunder methods on the type, not the instance, so the assignment had no effect. Now configured via __len__.return_value. (Spotted by Copilot.)
  • Coroutine objects passed to mocked runCoroutine are explicitly closed in the patch's side_effect to avoid RuntimeWarning: coroutine was never awaited.
  • Ble instances created in tests are tracked and closed in tearDown so the __del__ path doesn't hit a torn-down event loop.
  • Type hints added to the nested onReceive and mockResults helpers @seanbudd flagged.

Testing strategy:

Unit tests: rununittests.bat runs all 1094 unit tests cleanly (no warnings, no errors). The 13 BLE tests across TestScanner, TestBle, and TestFindDeviceByAddress are no longer skipped and all pass.

Static checks: ruff, pyright, and markdownlint all pass. runcheckpot.bat reports 0 errors.

Manual smoke test with real hardware: Ran NVDA from source and, from the Python console (NVDA+ctrl+z), exercised the scanner directly:

from hwIo import ble
ble.scanner.start()
import time; time.sleep(3)
print(ble.scanner.results())
ble.scanner.stop()

The scanner returned the BLE devices advertising nearby, confirming the module loads, the asyncio event loop integration works, and Bleak-based scanning behaves as expected end-to-end. Full bdDetect integration and connection-level testing will be exercised by PR C, where the wiring lands.

Known issues with pull request:

None.

Code Review Checklist:

  • Documentation:
    • Change log entry — developer-facing entry covering the new public submodule
    • User Documentation: N/A (no user-visible changes)
    • Developer / Technical Documentation: docstrings on all public classes/methods
    • Context sensitive help for GUI changes: N/A
  • Testing:
    • Unit tests
    • System (end to end) tests: N/A
    • Manual testing
  • UX of all users considered: N/A (developer API only)
  • API is compatible with existing add-ons.
  • Security precautions taken.

TDD approach: introduce all tests with skip decorators ahead of implementation.

- tests/unit/test_hwIo_ble.py: BLE scanner, BLE I/O, and findDeviceByAddress
- tests/unit/brailleDisplayDrivers/test_dotPad.py: buffered receive logic
- tests/unit/test_bdDetect.py: BLE device registration and matching
@bramd bramd requested a review from a team as a code owner March 23, 2026 09:46
@bramd bramd requested a review from seanbudd March 23, 2026 09:46
@bramd bramd marked this pull request as draft March 23, 2026 09:48
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 24, 2026
@bramd bramd marked this pull request as ready for review March 24, 2026 13:03
@seanbudd seanbudd requested a review from Copilot March 25, 2026 01:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds developer-facing, currently-skipped unit tests intended to drive upcoming Bluetooth Low Energy (BLE) support work (scanner/I/O, DotPad receive buffering, and bdDetect BLE matching), plus a changelog entry noting the new tests.

Changes:

  • Added a new skipped unit test module for the planned hwIo.ble scanner, BLE I/O, and findDeviceByAddress.
  • Added skipped unit tests for DotPad buffered receive logic (serial byte-at-a-time and BLE packet-at-once).
  • Extended existing bdDetect unit tests with skipped BLE registration/matching tests and documented the addition in changes.md.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

File Description
user_docs/en/changes.md Adds a developer changelog bullet noting the new skipped BLE-related unit tests.
tests/unit/test_hwIo_ble.py New skipped unit tests for future hwIo.ble scanner/I/O and device discovery helpers.
tests/unit/test_bdDetect.py Adds skipped tests for BLE device registration and matching in bdDetect.
tests/unit/brailleDisplayDrivers/test_dotPad.py New skipped tests for DotPad buffered receive logic supporting both serial and BLE receive patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

self.mockServices = MagicMock()
self.mockServices.get_service.return_value = self.mockService
mockServicesDict = MagicMock()
mockServicesDict.__len__ = lambda self: 1
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mockServicesDict.__len__ = lambda self: 1 won’t affect len(mockServicesDict) reliably because Python looks up special methods like __len__ on the type, not the instance. Configure the MagicMock’s __len__ return value instead (e.g. set mockServicesDict.__len__.return_value).

Suggested change
mockServicesDict.__len__ = lambda self: 1
mockServicesDict.__len__.return_value = 1

Copilot uses AI. Check for mistakes.
Comment on lines +119 to +120
packet2 = self._createPacket(dest=0, cmd=0x0102, seqNum=2, data=b"B")
packet3 = self._createPacket(dest=0, cmd=0x0103, seqNum=3, data=b"C")
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These packets use command values 0x0102 and 0x0103, which aren’t defined in brailleDisplayDrivers.dotPad.defs.DP_Command (valid values include 0x0100/0x0101, 0x0110/0x0111, 0x0200/0x0201, etc.). If the driver parses commands via DP_Command(...) (as it does today), this will raise ValueError when these tests are un-skipped. Use real DP_Command enum values in the test packets.

Suggested change
packet2 = self._createPacket(dest=0, cmd=0x0102, seqNum=2, data=b"B")
packet3 = self._createPacket(dest=0, cmd=0x0103, seqNum=3, data=b"C")
packet2 = self._createPacket(dest=0, cmd=0x0100, seqNum=2, data=b"B")
packet3 = self._createPacket(dest=0, cmd=0x0110, seqNum=3, data=b"C")

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +136
type=bdDetect.ProtocolType.BLE,
id="DotPad320",
port="AA:BB:CC:DD:EE:FF",
deviceInfo={"name": "DotPad320", "address": "AA:BB:CC:DD:EE:FF"},
)

non_matching_device = bdDetect.DeviceMatch(
type=bdDetect.ProtocolType.BLE,
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bdDetect.DeviceMatch.type is currently a ProtocolType (HID/SERIAL/CUSTOM) while the transport (USB/BLUETOOTH) is represented separately via CommunicationType. Encoding BLE as ProtocolType.BLE in these tests is inconsistent with the existing bdDetect model and may force an awkward API change. Consider keeping type as an existing protocol (likely ProtocolType.CUSTOM) and representing BLE via a new CommunicationType.BLE instead.

Suggested change
type=bdDetect.ProtocolType.BLE,
id="DotPad320",
port="AA:BB:CC:DD:EE:FF",
deviceInfo={"name": "DotPad320", "address": "AA:BB:CC:DD:EE:FF"},
)
non_matching_device = bdDetect.DeviceMatch(
type=bdDetect.ProtocolType.BLE,
type=bdDetect.ProtocolType.CUSTOM,
id="DotPad320",
port="AA:BB:CC:DD:EE:FF",
deviceInfo={"name": "DotPad320", "address": "AA:BB:CC:DD:EE:FF"},
)
non_matching_device = bdDetect.DeviceMatch(
type=bdDetect.ProtocolType.CUSTOM,

Copilot uses AI. Check for mistakes.
@seanbudd seanbudd self-assigned this Mar 25, 2026
@@ -0,0 +1,179 @@
# A part of NonVisual Desktop Access (NVDA)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the init file get copyright headers?

@@ -0,0 +1,407 @@
# A part of NonVisual Desktop Access (NVDA)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think this could be moved into a PR with the the planned hwIo/ble module?

Use the individual test commands instead: `runcheckpot.bat`, `rununittests.bat`, `runsystemtests.bat`, `runlint.bat`. (#19606, #19676, @bramd)
* Updated Python 3.13.11 to 3.13.12 (#19572, @dpy013)
* Added a private `_asyncioEventLoop` module that provides an asyncio event loop running on a background thread for use by NVDA components. (#19816, @bramd)
* Added skipped unit tests for upcoming BLE support: hwIo.ble scanner/IO, DotPad buffered receive, and bdDetect BLE device matching. (#19122, @bramd)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a changelog for added tests

Suggested change
* Added skipped unit tests for upcoming BLE support: hwIo.ble scanner/IO, DotPad buffered receive, and bdDetect BLE device matching. (#19122, @bramd)

Comment on lines +25 to +30
from brailleDisplayDrivers.dotPad.driver import BrailleDisplayDriver
from brailleDisplayDrivers.dotPad.defs import (
DP_Command,
DP_PacketSyncByte,
DP_CHECKSUM_BASE,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you plan on moving the imports out in #19122 ?

# Bind the actual _onReceive method
self.driver._onReceive = BrailleDisplayDriver._onReceive.__get__(self.driver, type(self.driver))

def _createPacket(self, dest=0, cmd=0x0101, seqNum=0, data=b""):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add type hints


def matchFunc(match: bdDetect.DeviceMatch) -> bool:
return True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think this could be moved into a PR with the the planned hwIo/ble module?


receivedData = []

def onReceive(data):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a type hint


callCount = 0

def mockResults():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a return type

@seanbudd seanbudd marked this pull request as draft March 25, 2026 04:32
Re-scopes this PR from the TDD-style "skipped tests only" approach
into "PR A" of the agreed split: ports the hwIo.ble module from
the dotpad-ble branch and unskips its unit tests.

- New source/hwIo/ble/ submodule (Scanner, Ble, findDeviceByAddress)
  built on _asyncioEventLoop and bleak.
- Adds bleak==3.0.0 runtime dependency.
- Adds a Changes for Developers entry covering the new public submodule.
- Unskips and fixes test_hwIo_ble.py:
  - Patches the real runCoroutine entry point used by the module
    (was incorrectly patching runCoroutineSync).
  - Configures MagicMock.__len__ via return_value (Python looks up
    dunder methods on the type, not the instance).
  - Closes coroutine objects passed to mocked runCoroutine to avoid
    "coroutine was never awaited" warnings.
  - Tracks Ble instances per test and closes them in tearDown so the
    __del__ path does not hit a torn-down event loop.
  - Adds type hints to the nested onReceive and mockResults helpers.
- Drops the bdDetect BLE test additions and the DotPad buffered-receive
  tests; they will land in the upcoming PR C alongside the bdDetect
  CommunicationType.BLE / addBleDevices integration and the DotPad
  driver work respectively.
@bramd bramd changed the title Add skipped unit tests for upcoming BLE support Add hwIo.ble module with unit tests Apr 11, 2026
bramd added 3 commits April 11, 2026 22:48
- Update copyright years to 2025-2026 in __init__.py and _io.py.
- Add a docstring to the module-level scanner singleton explaining why
  it is shared across callers.
- Add a docstring to queueReader describing its role as the BLE receive
  dispatcher.
- Narrow broad `except Exception` clauses:
  - findDeviceByAddress: catch only (BleakError, OSError) around
    scanner.start; drop the unnecessary outer try around the poll loop.
  - queueReader: catch only OSError around ioThread.queueAsApc; nothing
    else in the loop should raise.
- Switch Ble from the runCoroutine(...).result() + .exception() pattern
  to runCoroutineSync(...), which already blocks for the result and
  re-raises any exception. Affects _initAndConnect, write, and close.
- Update TestBle to patch hwIo.ble._io.runCoroutineSync to match.
Matches NVDA's camelCase convention and the signature of the
IoBase.read method it overrides, so polymorphic calls via an
IoBase reference work correctly.
@bramd bramd marked this pull request as ready for review April 12, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants