Add Bluetooth Low Energy support for DotPad braille displays#19122
Add Bluetooth Low Energy support for DotPad braille displays#19122bramd wants to merge 6 commits into
Conversation
| log.debug("Starting BLE scanner for background scan") | ||
| hwIo.ble.scanner.start() | ||
| # Give the scanner some time to get initial results | ||
| time.sleep(0.2) |
There was a problem hiding this comment.
Can we somehow wait non-blocking here?
There was a problem hiding this comment.
I was assuming that a background scan was always running on the hwIo thread and that a blocking sleep wouldn't matter that much. In theory, Windows is already scanning and caching results, but depending on hardware and BLE stack implementation we might end up with 0 results without a wait. So I found it more acceptable to have a small delay in a background scan then having no results and only having a chance to find BLE devices in the next scanning round. Especially when having Bluetooth classic devices that can match in a background scan and having Windows take 10-20 seconds to get a timeout when trying to connect a Bluetooth serial port, I think a 0.2 sec delay is acceptable.
If my assumptions are invalid or you see a way to wait non-blocking and still get results in the same scanning round, I would like to hear your thoughts.
| @param bluetooth: Whether the handler is expected to yield USB devices. | ||
| @param bluetooth: Whether the handler is expected to yield Bluetooth devices. | ||
| @type bluetooth: bool | ||
| @param ble: Whether the handler is expected to yield BLE devices. |
There was a problem hiding this comment.
Have you considered reusing the bluetooth parameter? Can't you just handle BLE in case a driver is enabled that supports BLE?
I assume scanning for BLE can be a bit costly. It makes not that much sense to scan for these devices when I don't have a device that supports it.
There was a problem hiding this comment.
I considered it, but chose a separate ble parameter because:
- BLE can connect to devices without Windows pairing them first and there might be a wish to disable the feature for example in an environment with any BLE braille displays, where Bluetooth classic pairing would still be acceptable for automatic detection. Note that we do not expose that in the UI at the moment
- Windows' documentation is unclear about the cost of BLE scanning. It seems Windows is doing some kind of scanning and caching anyway, but this might also be dependent on the hardware and Bluetooth drivers/stack and is hard to test without clear documentation and not much Bluetooth adapters to test with
However, if there is no wish to disable BLE separately, I'd be fine with merging the ble/bluetooth parameters.
| ports[portKey] = portName | ||
| except Exception: | ||
| # If BLE scanning fails, continue without BLE devices | ||
| pass |
There was a problem hiding this comment.
Silently discarding a bare except feels like bad practice. Could you either make the exception more specific or log a debug warning or the like?
| if isinstance(port, bdDetect.DeviceMatch): | ||
| yield port | ||
| elif isinstance(port, str): | ||
| # Check if this is a specific BLE device port (format: "ble:DeviceName@Address" or legacy "ble:DeviceName") |
There was a problem hiding this comment.
Can you split this out into a helper function?
| try: | ||
| self._processPacket(packet[4:]) | ||
| except Exception: | ||
| log.error("Error processing packet", exc_info=True) |
There was a problem hiding this comment.
This will re-enter the loop, right? Can't this create an infinite loop we'll never break from?
There was a problem hiding this comment.
Before processing the packet, it's removed from the buffer. So in most cases we would end up with an empty buffer and the loop would exit. If there somehow is a few bytes left in the buffer it will either be a valid packet and processed as such, or the loop will exit because there are not enough bytes. Where/how do you see the risk of the loop not being exited?
| else: | ||
| # No ports available - show helpful message | ||
| # Translators: Message shown when no devices are available for a braille display | ||
| noDevicesMessage = _("(No devices found - switch to another display and back to refresh)") |
There was a problem hiding this comment.
Could there be a better UX for this? Can't you just refocus or something?
There was a problem hiding this comment.
Can you clarify this? Do you want the ports list to be enabled so people can read this message better or do you want that the ports list dynamically updates in realtime if devices are discovered?
There was a problem hiding this comment.
Pull Request Overview
This PR adds Bluetooth Low Energy (BLE) support for DotPad braille displays, enabling automatic detection and manual selection of BLE-connected devices alongside existing USB support.
Key changes:
- Introduced Bleak 1.1.0 dependency and asyncio event loop infrastructure for BLE communication
- Extended bdDetect system to handle BLE device scanning and matching in parallel with USB/Bluetooth
- Updated GUI to automatically start BLE scanning when braille settings dialog opens, with user guidance for device refresh
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/userGuide.md | Added BLE connection documentation and usage instructions for DotPad |
| user_docs/en/changes.md | Documented BLE support as a new feature in changelog |
| tests/unit/test_hwIo_ble.py | Added comprehensive unit tests for Scanner, Ble I/O, and device lookup |
| tests/unit/test_bdDetect.py | Extended tests to cover BLE device registration and matching |
| tests/unit/brailleDisplayDrivers/test_dotPad.py | Added buffered receive tests for serial and BLE packet handling |
| source/hwIo/ble/_scanner.py | Implemented BLE scanner wrapper around Bleak with extension point for device discovery |
| source/hwIo/ble/_io.py | Created Ble I/O class for BLE communication with MTU-aware writes and notification handling |
| source/hwIo/ble/init.py | Exposed scanner singleton and device lookup utility functions |
| source/gui/settingsDialogs.py | Modified braille settings dialog to start/stop BLE scanner and display helpful messages |
| source/core.py | Integrated asyncio event loop initialization and termination |
| source/brailleDisplayDrivers/dotPad/driver.py | Enhanced driver to support BLE connections with buffered packet processing |
| source/brailleDisplayDrivers/dotPad/defs.py | Added BLE service and characteristic UUIDs |
| source/braille.py | Extended device detection and port enumeration to include BLE devices |
| source/bdDetect.py | Added BLE scanning, real-time discovery notifications, and device matching |
| source/asyncioEventLoop.py | Created asyncio event loop module for running async operations |
| pyproject.toml | Added Bleak dependency and WinRT package exceptions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
SaschaCowley
added
the
conceptApproved
|
@SaschaCowley I see you fixed the Chrome system tests yesterday, but in this PR the startup/shutdown and installer tests are still failing. Is this expected or could it indicate a problem introduced in this PR? |
|
Hi @bramd, sometimes these tests do just fail. However the fact that the tests on Windows 2022 and 2025 failed in the same way is suspicious. I've triggered the tests to rerun to see if it was just a fluke |
Unfortunately, they still seem to fail. I'm trying to get a VM going with NVDA source to test properly without a Bluetooth adapter, but have some issues getting the build environment set up for now. Pending the failing tests, I think this is not ready for merge. However, I would appreciate it if you can at least test this on your hardware, since there are not much users with Dot Pad hardware to try this PR. |
I will try out the PR tomorrow and let you know how it goes. Just so you're aware, this PR may take longer for us to get to as it is quite large, so I want to set aside a while to sit down with it and properly digest it. We definitely have not forgotten about it though :) |
|
Hi @bramd, I have just tested this PR with our DotPad. I performed the current steps:
This caused the DotPad to emit two long buzzes, pause for a few seconds, then emit two more long buzzes, and NVDA to report an error connecting. |
|
it seems like this is causing serious system test errors around NVDA exiting safely |
No problem, I'm also still having weird issues (not related to this PR) building on my test machine without a Bluetooth adapter and would like to do a good test run on such a machine. That might also give some more insight on the failing system tests. For me this work is partly ported from what was in the Dot Pad add-on and partly rewritten to better fit NVDA core or because I saw room for improvement since the last iteration. Given the complexity of introducing asyncio and the Bleak library and touching quite some points in bdDetect I really appreciate a thorough code review on this. I've quite some time to work on this the coming weeks, so I should be able to address comments quickly. |
The first 2 buzzes are expected. The Dot Pad double buzzes every time a BLE connection is made or a connection is closed. So the second buzzes you see match the closing of the connection after the driver fails to initialize. Could you please try the following:
If you can't get it working, I'd be glad to help you out here or by email if we want to keep the PR comments relevant. |
There was a problem causing the asyncio loop to hang for 5 secs on shutdown. This has been fixed and now the tests pass as well. |
|
is this ready for re-review? |
Almost, I finally have a working VM with no Bluetooth adapter to do some more tests (took me some time due to #19189). It seems the scanner fails silently without an adapter (e.g. just does not return any results). So I will remove some checks for Bluetooth availability that will never trigger. The BLE APIs have been added in Windows 10 creator's update, which is no longer actively supported by NVDA. So we may consider the BLE stack to be always available, even without any Bluetooth hardware in the machine. |
243346f to
8085691
Compare
|
@seanbudd Just in case you missed the status change from draft to normal PR, this is ready for re-review. @SaschaCowley Have you been able to get this to work with your Dot Pad hardware? If not, I'd be glad to help you debug. |
|
Hi @bramd - thanks for your work here and sorry on the delays.
|
…round thread Split from nvaccess#19122. Provides initialize/terminate lifecycle and runCoroutine/runCoroutineSync helpers for scheduling async work from synchronous code. Wired into core startup/shutdown. Includes 6 unit tests.
…round thread Split from nvaccess#19122. Provides initialize/terminate lifecycle and runCoroutine/runCoroutineSync helpers for scheduling async work from synchronous code. Wired into core startup/shutdown. Includes 6 unit tests.
N/A. This is a follow-up from #19122 (DotPad BLE support), splitting out the asyncio event loop module as requested during review. Summary of the issue: NVDA currently has no built-in way to run asyncio coroutines. Components that need async functionality (e.g. BLE communication) require a managed asyncio event loop running on a background thread. Description of user facing changes: N/A. This is a developer-facing module only. Description of developer facing changes: Added a private _asyncioEventLoop module that provides: initialize() / terminate() — lifecycle management, wired into NVDA core startup/shutdown runCoroutine(coro) — schedule a coroutine on the event loop (fire-and-forget) runCoroutineSync(coro, timeout) — schedule a coroutine and block until completion The module is prefixed with an underscore to keep it internal to NVDA core and not part of the public add-on API. Description of development approach: The asyncio event loop runs on a daemon thread, started during NVDA initialization (after appModuleHandler) and terminated during shutdown (after hwIo). The terminate() function cancels all pending tasks before stopping the loop. runCoroutineSync wraps asyncio.run_coroutine_threadsafe with optional timeout support and converts asyncio.TimeoutError to the built-in TimeoutError for a cleaner API.
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 - do you think the |
|
@seanbudd Yes, that would give us the scanner and low-level BLE hwIo
interface. We could run the unit tests for that and do manual testing by
starting a scanner and checking results from the Python console.
Communicating with a device is a bit harder to test manually as well,
since you need to know the services and commands to send and what the
device should give you back. And of course, this is dependent on the
physical devices you have available for testing. But still, the current
diff of this PR is substantial, so splitting it up further makes sense.
Any ideas on testing hwIo.ble if we split it off?
|
|
I think the unit tests should be fine for testing the ble module - we will get proper manual integration testing when the rest of this PR goes in |
|
@seanbudd Thanks, I'll try to split it up like that. Just to be sure, we will get:
Please let me know if that was what you intended. If so, I'll update/abandon #19838 and come through with the BLE module + tests first. |
|
yes that would be perfect |
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. Ble — IoBase 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.
Replace the fragile _onReceive that did inline blocking reads with a buffered-receive approach that accumulates bytes and extracts complete packets. This handles resynchronization on bad sync bytes, buffer overflow protection, and partial-packet buffering. Works with both byte-at-a-time delivery (Serial) and packet-based delivery (BLE). Extract _processPacket from the old inline parsing for clarity. Add 9 unit tests covering: complete packet, byte-at-a-time, partial chunks, multiple packets, resync, overflow, incomplete, empty, and partial header. All pass. Add 5 skipped BLE-specific DotPad tests (TDD skeleton for PR C): _isBleDotPad, check(), addBleDevices registration, _tryConnect BLE. Also fixes "Received responce" typo → "Received response".
Two correctness fixes to the buffered-receive path: - Clear _receiveBuffer at the start of each _tryConnect probe. The buffer is instance-level and was reused across auto-detect probes; the USB match is a generic FTDI VID, so stray bytes from a non-DotPad device on an earlier port could prefix and corrupt the real device's first response. - Validate the declared packet length at parse time instead of capping the accumulation buffer. Real DotPad frames are small, so a declared length beyond DP_MAX_PACKET_SIZE means a false/garbled header: discard one byte and resync. This drops the old pre-extraction buffer wipe, which (a) discarded multi-packet bursts exceeding 512 bytes before processing any of them and (b) could never assemble a frame larger than 512 bytes. It also fixes a stall where a false sync followed by a large bogus length waited indefinitely (recoverable only by wiping queued real packets). Tests: replace test_bufferOverflow_cleared (obsolete semantics) with test_implausibleLength_resynchronize, and assert extracted packet body content in test_completePacketAtOnce to cover the header-stripping slice.
Replace the old "covered by the GNU General Public License / See the file COPYING" reference with the current two-line license reference documented in projectDocs/dev/copyrightHeaders.md, across the files touched in this PR.
Replace "frame" with "packet" in the comments/docstrings touched in this PR to match the terminology used in the code (packet, packetBody, packetLength).
- Make DP_MAX_PACKET_SIZE a docstring and add DP_MIN_PACKET_SIZE - Resync on implausibly small declared lengths, not just oversized ones - Use log.exception for packet-processing errors - Hoist bdDetect import to module top in tests; clarify length-guard tests
Integrate BLE braille display detection and connection on top of the hwIo.ble transport that now lives in master. This change contains only the integration layer; the Bleak/hwIo.ble stack is no longer part of this branch. - bdDetect: scan for and match BLE devices, register BLE match functions via DriverRegistrar.addBleDevices, and expose getBleDevicesForDriver. Guard all hwIo.ble.scanner access for the case where BLE is not initialized (e.g. unit tests), so device detection degrades gracefully instead of crashing. - braille: thread a `ble` scan flag through detection enable/rescan, match detected displays by the "ble" provider, and surface individual BLE devices as selectable "ble:Name@Address" ports. - DotPad driver: add check(), _isBleDotPad matcher, BLE automatic-detection registration, and a BLE branch in _tryConnect that opens an hwIo.ble.Ble device. Builds on the buffered-receive packet handling. - Braille settings dialog: start/stop the shared BLE scanner while the display selection dialog is open to populate the device list. - Enable the DotPad BLE unit tests (matcher, check, registration, _tryConnect) that were previously skeleton-skipped pending this integration. Stacked on the buffered-receive branch (nvaccess#19942); retarget to master once that merges.
N/A. This is a follow-up to #19122 (DotPad BLE support) and #19838 (hwIo.ble module). As discussed in those PRs, the work is being split into three smaller PRs. This is "PR B" — the buffered-receive implementation and skipped BLE tests in preparation for the final BLE driver integration in PR C. There is no standalone issue tracking this change. Summary of the issue: The DotPad driver's _onReceive method reads remaining packet bytes synchronously via self._dev.read() and raises RuntimeError on any malformed byte, which can kill the display connection. This approach also cannot handle BLE's packet-based delivery model where entire packets arrive in a single callback. The buffered-receive replaces this with a more robust approach as a prerequisite for BLE support. Description of user facing changes: DotPad braille displays now handle malformed serial data more gracefully. Instead of crashing the display connection on a single corrupted byte, the driver resynchronizes and continues operating. In practice, since DotPad devices currently only connect over USB serial (which is a stable connection), users are unlikely to notice this change. The real benefit comes when BLE support lands in PR C, where packet-based delivery makes the buffered approach essential. Description of developer facing changes: Replaced _onReceive(self, header1: bytes) with a buffered-receive approach _onReceive(self, data: bytes) that accepts variable-length input (1 byte from Serial, full packets from BLE). Extracted _processPacket(self, packetBody: bytes) from the old inline parsing for clarity. Added DP_MAX_PACKET_SIZE protocol constant to defs.py. The buffered-receive handles: resynchronization on bad sync bytes, buffer overflow protection, partial-packet buffering, and multiple packets in a single call. Description of development approach: Ported the buffered-receive logic from the dotpad-ble branch (#19122), keeping only the transport-agnostic parts. No BLE-specific code is included — that lands in PR C. The approach replaces synchronous self._dev.read() calls within the callback with a _receiveBuffer that accumulates bytes across callbacks. Complete packets are extracted from the buffer as they become available.
DotPad braille displays support both USB Serial and Bluetooth Low Energy (BLE) connections. NVDA previously only supported USB connections.
DotPad Driver:
GUI Integration:
Link to issue number:
Fixes #18584
Summary of the issue:
DotPad braille displays support both USB Serial and Bluetooth Low Energy (BLE) connections. NVDA previously only supported USB connections.
Description of user facing changes:
Description of developer facing changes:
Description of development approach:
Added Bleak for BLE communication. Since Bleak is an asyncio library, I had to implement an asyncio event loop as well. I chose to run this on it's separate thread and not integrate it in NVDA's WX main loop in any way.
bdDetect has been modified to scan for and match BLE devices with their drivers. Since with BLE in Windows the app is responsible for all the scanning and connecting, the scanner starts as soon as the braille display selection is opened. Otherwise, we never would have devices to connect to in the port selection list. If there are no ports, the ports list shows a message to help the user that they can switch to another display and back to refresh the list.
The DotPad driver has been modified to accept BLE devices to connect to. BLE devices are stored in the configuration as
ble:name@address, since we need the address to connect if there is no scanner result. If there is a scanner result, we can match on name as well.The onReceive method of the driver has been altered so it accepts both input one byte at a time (for serial) or as whole packets (BLE).
The BLE scanner implements a new Action (extension point) that fires when a BLE device is discovered. Therefore, connection after turning on the device should be very quickly established.
Testing strategy:
Wrote extensive unit tests and tested with actual hardware.
Things to test:
Known issues with pull request:
Since BLE does not require any pairing, the bdDetect system now connects to any DotPad it discovers. Since DotPad is disabled for automatic detection by default and users can set a specific BLE device to connect to, I think this is acceptable. If not, we would have to implement a GUI for selecting devices and keep a list of selected devices that are allowed to be discovered automatically.
Code Review Checklist: