Make automatic braille display detection more pythonic#14147
Conversation
See test results for failed build of commit 57b7268556 |
See test results for failed build of commit cfefdb5e9b |
See test results for failed build of commit 2f2ac00002 |
See test results for failed build of commit 8a08c1db77 |
aca2479 to
da8b062
Compare
|
This was tested by @dkager. He reported that crashes on his system that happened on Alpha still occur with this branch, so it looks like there is no real benefit in this pr other than that it decouples auto detection from braille._BgTrhead. I leave it up to @feerrenrut or someone else from NV Access to decide about this pr. |
|
@LeonarddeR Have you considered creating an issue describing these crashes? I understand that you don't have a solution, but not reporting at all means that unless either you or @dkager would create a fix the problem will remain unsolved. |
|
@dkager Would you be able to do this? As far as I understood you correctly, the crashes are very rare and hard to reproduce reliably. |
|
While the code from @LeonarddeR does not fix everything, I did notice that it was much rarer for NVDA to fall over altogether. With 2022.3 I estimate there to be a 20-30% chance of NVDA crashing after waking the Brailliant BI 40X from sleep while on bluetooth. I haven't found any log entries pointing to the cause, though. |
|
Update: I got this straight after booting the PC with NVDA auto-starting. Obviously, Leonard's code fixes this. |
|
And interestingly, when disabling HID braille support: |
|
These permissions aren't that worrying. The access violation is much more concerning, though. |
|
An access violation in the Windows sleep function especially. :) |
I guess it is crashing because the sleep waits for a handle that has been destroyed. That's why I want to get rid of this APC stuff altogether, it requires a lot of specific Win32 knowledge while in contrast, the Pythonic implementation this pr is much more readable IMO. |
seanbudd
left a comment
There was a problem hiding this comment.
Is there a possibility of added unit tests?
Added @feerrenrut as a reviewer to confirm the new style/approach is appropriate.
|
Perhaps refactoring should be a separate step beyond implementing a fix for the very real issue of the display detection crashing. I imagine that making braille more deterministic could cause some interesting new issues at first. |
|
@seanbudd Not sure why you're marking this as draft? It is ready for review and currently under review. |
This comment is unresolved: #14147 (comment) |
|
I think this is ready again if you agree this case is too complex for unit tests. |
Other than API breaking change I raised, I don't believe this PR affects developers. |
Not likely. There will be a difference in that an automatically detected braille display driver will now be initialized on a different thread (i.e. the threadpool thread, not braille._BgThread). However:
|
|
As we don't expect any changes for developers, i.e. this is changes to internal code only, I am going to avoid adding a \change log entry. |
Closes #14130 Related to #14147 Summary: #14130 notes that the background thread for i/o is too tightly coupled too the braille module. This required the hwIo module to queue an APC to the braille background thread. User facing changes: None, apart from more safety checks that might impose stability improvements. Changes: 1. Moved the background thread from the braille module. - Instead make it a class on the hwIo module. - It is also no longer a singleton and therefore add-ons can create their own i/o thread. 2. The APC queue function now takes a callable or lambda. - Rather than all cases where APCs have to be queued to the background thread being responsible for wrapping themselves in a winKernel.PAPCFUNC. - The provided callable is converted to an APC when queued. - There is a cache on the thread class to keep references to wrapped APCs until they are finished. - The wrapper also ensures that the APC doesn't execute when the thread is about to exit. 3. hwIo.base.IoBase now contains a new `_initialRead` method that initiates reading on the background thread. - This method can be overridden on subclasses to outsource the reading to another thread.
Link to issue number:
Related to #14130
Summary of the issue:
Parts of the braille display detection code are hard to read and rely on Win32 APCs to be queued. A major drawback about APCS is that, as soon as theyre queued, they can not be canceled any more.
Description of user facing changes
None, though I hope things will be more stable after this change
Description of development approach
In an attempt to make the code more pythonic as well as changing the code in such a way that it no longer depends on the braille background thread, I changed the following:
Testing strategy:
This requires extensive testing in alpha for a while. Note that @dkager reported improvements, see #14147 (comment)
Known issues with pull request:
None known
Change log entries:
For Developers
Code Review Checklist: