Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give NVDA the ability to automatically detect braille displays. #1271

Closed
nvaccessAuto opened this issue Dec 11, 2010 · 64 comments · Fixed by #7741
Closed

Give NVDA the ability to automatically detect braille displays. #1271

nvaccessAuto opened this issue Dec 11, 2010 · 64 comments · Fixed by #7741

Comments

@nvaccessAuto
Copy link

Reported by jkenn337 on 2010-12-11 15:50
When a braille display is connected to the computer and brltty is installed, NVDA should scan for and automatically detect and start using the connected USB or bluetooth display. This way the user does not have to mess about with choosing their display and things. Orca and the mac already do this. This enhancement should be across all operating systems.
Blocking #1555, #3648

@nvaccessAuto
Copy link
Author

Comment 1 by ragb on 2013-09-02 10:33
Hi,

Recently, I have been encountering some users with difficulties understanding what braille display driver they should use, either because they don't really know the display they are using, or don't know the manufacture or something else.

So, I'll research something about automatically detecting USB / bluetooth displays (brltty may be a good start)

Some questions I can think of:

  1. Do we want probing/active scanning, e.g. user presses a button and NVDA scans available devices, presenting possible connectable ones, or connects to the first found?
  2. Do we want autodetect / passive scan, e.g. NVDA listens to devices being plugged and checks if it is a braille display or not?
  3. Do we want both? :)

Both options are great of course, but implies more complexity and changes to the braille subsystem and display drivers.

#426 has some interesting discussion about this.

@nvaccessAuto
Copy link
Author

Comment 2 by jteh on 2013-09-03 01:05
I think both probing and detection would be nice. Probing would be used for serial displays and should warn the user about possible side effects.

Bluetooth is a tricky one. You can't passively detect Bluetooth displays. However, probing Bluetooth is safer than serial because Bluetooth ports are paired to a specific device. Still, it seems ugly to constantly probe for any paired Bluetooth displays.

@nvaccessAuto
Copy link
Author

Comment 3 by ragb (in reply to comment 2) on 2013-09-05 10:24
Replying to jteh:

I think both probing and detection would be nice. Probing would be used for serial displays and should warn the user about possible side effects.

Agreed.

Bluetooth is a tricky one. You can't passively detect Bluetooth displays. However, probing Bluetooth is safer than serial because Bluetooth ports are paired to a specific device. Still, it seems ugly to constantly probe for any paired Bluetooth displays.

Unless we can find a way to by notified about bluetooth displays becoming available (that is without constant pooling) I guess probing is the best we can get.

As most drivers already implement some sort of automatic connection, I believe probing won't be very hard to do. Basic design (based on already discussed stuff on #426 and others):

  1. Display drivers have a probe method and an associated canProbe property.
  2. probe concret implementations should return a list of (port, description) pairs of any displays found (connection was successful). Description here is driver-dependent, but it should contain display name and number of cells, if possible. Note: unless one has two displays or a display can be used in to different communication ports, this list may always contain one element.
    3 . When probing is requested (by the user) NVDA checks all displays for which canProbe is true and shows to the user all displays found for various drivers. (if just one is found we can simply choose to it, I guess).

Regarding the interface: I believe a "probe" button on the braille settings dialog, next to the display selection, which pops a "probing" dialog would be the least confusing solution for now. This dialog can alos be accessible from the wellcome screen.
Before probing starts the user should be warned of any possible dangers probing may have, such as contacting serial ports and so on.

As regarding autodetection, I'll have to research a bit more about windows plug and play and so on to be able to propose something detailed.

@nvaccessAuto
Copy link
Author

Comment 4 by ragb on 2013-09-24 10:29
Hi,

Should probing be used only for serial ports or we try also to connect to other available ports (USB and bluetooth, usually)?. With autodetection implemented, probing those may be useless, but we may not be able to implement auto detection for all drivers.

@nvaccessAuto
Copy link
Author

Comment 5 by jteh on 2013-09-24 10:34
Probing all ports is fine.

@nvaccessAuto
Copy link
Author

Comment 6 by leonarddr on 2013-09-24 11:11
If this functionality is added, i think it should be optional, as non-braille users don't care about it at all. Furthermore, it will probably create a lag on start-up.

@nvaccessAuto
Copy link
Author

Comment 7 by ragb on 2013-09-24 11:37
Hi,

At first I was convinced that the best implementation was to have probing at the driver level. Here I think of two alternatives: Drivers have a probe method which, at the driver specific implementation, tries to connect to its ports. On the other hand, one could have a portsToProbe method which returns tuples of (port, priority) so we can probe "faster" ports firstly (USB mainly) for all drivers, before probing bluetooth and serial ports.

However, a much simpler alternative is to implement probing at the braille subsystem module, taking advantage of the (possibly implemented) getPossiblePorts} method in braille display driver classes. The probing procedure would be as follows:

  1. Retrieve all available braille display drivers
  2. For each driver, try to connect to all possible ports, except automatic (this is because automatic may contain some of the other ports, so we avoid duplication)
    2.1. If the driver has no getPossiblePorts method, just try to initialize it and see if one can connect.
  3. When a successful connection is made, report it to the user along with braille display type and cell count, asking if he wants to use this display. If so, end probing and set the found display in configuration.
  4. If no connection was possible, or the user rejected all found displays, just return to braille settings dialog, or wherever probing was started from.

In theory, this solution makes all drivers "probing-ready", but we loose priorities and driver specific probing methods, which, at second thought, are not so so important due to probing being a rare procedure, I believe.

Comments?

@nvaccessAuto
Copy link
Author

Comment 8 by ragb (in reply to comment 6) on 2013-09-24 11:42
Replying to leonarddr:

If this functionality is added, i think it should be optional, as non-braille users don't care about it at all. Furthermore, it will probably create a lag on start-up.

This won't be called at startup at all. It must be always requested by the user.

@nvaccessAuto
Copy link
Author

Comment 9 by jteh on 2013-09-24 12:46
I don't think there's any need to implement a portsToProbe method, as getPossiblePorts tends to be ordered such that more common/faster ports are first anyway.

There are two problems with doing as you suggest:

  1. Probing could be very slow (particularly if it touches Bluetooth ports), so this probably needs to be done in a background thread so the user can cancel it. You'll run into cross-thread issues if you initialise a display in a background thread and then use it from the main thread. I guess you could initialise it in the background thread just for the purposes of probing, terminate and then re-initialise in the main thread, but that's pretty ugly.
  2. It doesn't give us a nice way to perhaps do background polling for Bluetooth displays. I guess we could solve this in the same way, though.

@nvaccessAuto
Copy link
Author

Comment 10 by ragb (in reply to comment 9) on 2013-09-24 14:01
Replying to jteh:

I don't think there's any need to implement a portsToProbe method, as getPossiblePorts tends to be ordered such that more common/faster ports are first anyway.

My idea was more of trying faster ports from all drivers first, and then slower ones from all drivers, so the priorities thing. But honestly it is kind of over-complicating.

There are two problems with doing as you suggest:

  1. Probing could be very slow (particularly if it touches Bluetooth ports), so this probably needs to be done in a background thread so the user can cancel it.

I forgot to mention this. Probably we spawn an IndeterminatedProgressDialog and do the probing in a background thread, allowing the user to cancel it.

You'll run into cross-thread issues if you initialise a display in a background thread and then use it from the main thread. I guess you could initialise it in the background thread just for the purposes of probing, terminate and then re-initialise in the main thread, but that's pretty ugly.

It is ugly indeed, but honestly I can't find a cleaner solution :$.

  1. It doesn't give us a nice way to perhaps do background polling for Bluetooth displays. I guess we could solve this in the same way, though.

Maybe... We could restrict probing for bluetooth ports and having a background thread polling them from time to time, when no braille display is selected.

@nvaccessAuto
Copy link
Author

Comment 11 by ragb (in reply to comment 9) on 2013-09-24 17:02
Remember something else:

Replying to jteh:

  1. It doesn't give us a nice way to perhaps do background polling for Bluetooth displays. I guess we could solve this in the same way, though.

We could do bluetooth pooling this way. Moreover ,we could also do USB pooling in the background, avoiding dealing with plug and play, USB ids, etc (has I believe OS X and brltty in linux are doing). It's lesse eficient but way simpler.

@nvaccessAuto
Copy link
Author

Comment 12 by jteh (in reply to comment 11) on 2013-09-24 21:57
Replying to ragb:

Moreover ,we could also do USB pooling in the background, avoiding dealing with plug and play, USB ids, etc (has I believe OS X and brltty in linux are doing).

Pretty sure BRLtty uses udev to handle plug and play for USB.

It's certainly simpler, but it has the disadvantage that it can only be done periodically to avoid performance issues. If I plug in a USB display, I would expect it to be detected immediately, rather than having to wait. With Bluetooth, you don't expect this so much because Bluetooth is wireless.

@nvaccessAuto
Copy link
Author

Comment 13 by jteh on 2013-09-25 05:42
I'm starting to warm to this idea a bit more. It's at least a starting solution which we can improve upon later if necessary.

For auto detection, we can periodically try to create all displays with the default port in a background thread. Windows notifies about device connection via the WM_DEVICECHANGE message. Eventually, we could detect this and trigger a search instead of waiting.

The only problem is that there are some drivers which succeed even if a device isn't connected such as brltty and handyTech. Brltty only succeeds if brltty is running, but that could still be an issue. I guess we could have an attribute which disables automatic detection. We probably need to fix handyTech so that it doesn't do this; it really shouldn't.

@nvaccessAuto
Copy link
Author

Comment 14 by ragb (in reply to comment 13) on 2013-10-06 17:36
Replying to jteh:

For auto detection, we can periodically try to create all displays with the default port in a background thread. Windows notifies about device connection via the WM_DEVICECHANGE message. Eventually, we could detect this and trigger a search instead of waiting.

I've been playing with this lately (see branch 51271, for a start).
What would be the best approach To listen to WM_DEVICECHANGE? In the FS driver, for instance, we create a dummy window to listen to windows messages, would something like this work or there is a better lace? Windows API IPC is not one of my favorite topics in programing, definitely :).

The only problem is that there are some drivers which succeed even if a device isn't connected such as brltty and handyTech. Brltty only succeeds if brltty is running, but that could still be an issue. I guess we could have an attribute which disables automatic detection. We probably need to fix handyTech so that it doesn't do this; it really shouldn't.

Agreed. See canProbe attribute on branch t1271. I used that for nobraaille but brltty and Handitech should have it as well. On a side note, it would be very very nice from handitech to contribute the code they seem to be developing, whatever that is :).

@nvaccessAuto
Copy link
Author

Comment 15 by jteh (in reply to comment 14) on 2013-10-07 23:39
Replying to ragb:

I've been playing with this lately (see branch 51271, for a start).

That's a very nice start. :)

What would be the best approach To listen to WM_DEVICECHANGE? In the FS driver, for instance, we create a dummy window to listen to windows messages, would something like this work or there is a better lace?

I wonder whether we need to have something that manages a window for this and other things in future. This probably needs to be handled in a separate ticket.

Windows API IPC is not one of my favorite topics in programing, definitely :).

Nor one of mine. :)

The only problem is that there are some drivers which succeed even if a device isn't connected such as brltty and handyTech.

Agreed. See canProbe attribute on branch t1271. I used that for nobraaille but brltty and Handitech should have it as well.

I just realised those drivers return 0 for numCells when they didn't detect a display, so we could make those drivers throw an exception if this occurs. That way, they can still support probing.

On a side note, it would be very very nice from handitech to contribute the code they seem to be developing, whatever that is :).

Yeah. It might be time to follow this up. They definitely need to release the code to honour the license and it has been several months now.

Once you get to background automatic detection, we need to think about UI and persistence.

  1. How does the user specify that they want automatic detection? Is it a checkbox or an additional option in the braille display list? A checkbox perhaps seems nicer, but it makes the next question a bit tricky.
  2. Once a display is detected, does this cause the config to be modified? I guess it probably shouldn't, as that will make that display permanent.
  3. Once a display is detected, what should appear as the configured display in the Braille Settings dialog? Currently, the selected display is the one in the config. If we want the detected display to be selected, this will require some changes depending on the answer to (2).
  4. We probably need to fix Repeated exceptions when active braille display is disconnected #1555 to make automatic detection truly useful.

@nvaccessAuto
Copy link
Author

Comment 16 by ragb (in reply to comment 15) on 2013-10-09 21:54
Replying to jteh:

Replying to ragb:

What would be the best approach To listen to WM_DEVICECHANGE? In the FS driver, for instance, we create a dummy window to listen to windows messages, would something like this work or there is a better lace?

I wonder whether we need to have something that manages a window for this and other things in future. This probably needs to be handled in a separate ticket.

Agreed. We may need to register various handlers from many places in NVDA's code, so a central window where one registers handlers for various messages may be useful. I'll submit a ticket for this.

The only problem is that there are some drivers which succeed even if a device isn't connected such as brltty and handyTech.

Agreed. See canProbe attribute on branch t1271. I used that for nobraaille but brltty and Handitech should have it as well.

I just realised those drivers return 0 for numCells when they didn't detect a display, so we could make those drivers throw an exception if this occurs. That way, they can still support probing.

I like this option. It makes all drivers more consistent.

Once you get to background automatic detection, we need to think about UI and persistence.

  1. How does the user specify that they want automatic detection? Is it a checkbox or an additional option in the braille display list? A checkbox perhaps seems nicer, but it makes the next question a bit tricky.

I like the checkbox option. Do we need to have autodetection always running or only when nobraille is selected?

  1. Once a display is detected, does this cause the config to be modified? I guess it probably shouldn't, as that will make that display permanent.

I'm not really sure about this. If we don't set this, ton the next restart, the display may not be detected right away and the user will have to wait for probbing (if probbing is fast enough it may not be a problem). However, if we set it in the config, the user will have an error next time NVDA starts and the display is disconnected. If probing is not that fast, I'd say that setting detected displays in the config would be my choice.

  1. Once a display is detected, what should appear as the configured display in the Braille Settings dialog? Currently, the selected display is the one in the config. If we want the detected display to be selected, this will require some changes depending on the answer to (2).

As you say, it depends on 2 :).

  1. We probably need to fix Repeated exceptions when active braille display is disconnected #1555 to make automatic detection truly useful.

Yes. If a display starts throwing errors it should be disconnected and auto detection put to work.

Regards,

Rui Batista

@nvaccessAuto
Copy link
Author

Comment 17 by jteh (in reply to comment 16) on 2013-10-15 04:30
Replying to ragb:

  1. How does the user specify that they want automatic detection? Is it a checkbox or an additional option in the braille display list?

I like the checkbox option. Do we need to have autodetection always running or only when nobraille is selected?

On further thought, I'm not sure it makes sense to allow the user to manually select a display while automatic detection is enabled; see below.

  1. Once a display is detected, does this cause the config to be modified?

I'm not really sure about this. If we don't set this, ton the next restart, the display may not be detected right away and the user will have to wait for probbing (if probbing is fast enough it may not be a problem). However, if we set it in the config, the user will have an error next time NVDA starts and the display is disconnected.

Are you saying that automatic detection would resume if the previously configured display threw an error? To me, this seems odd and difficult to explain. It's one of the reasons I think manual and auto should be mutually exclusive.

My feeling is that plug-and-play detection should be mutually exclusive with manual selection to avoid confusion. For example, if the user manually selects a display but then plugs in another one, what should NVDA do? If they are mutually exclusive, then it probably makes most sense for this to be a choice in the braille display list.

Of course, the user might want to probe for/detect their display just once; e.g. if they want to save the selection but don't know which display they have or which driver/port to use. My only concern is that manual probing will intrusively probe arbitrary serial ports, which the user might not want. This suggests we need an option to just probe USB and Bluetooth, but then this starts to get confusing for the user. Arrrg!

@nvaccessAuto
Copy link
Author

Comment 19 by jteh on 2013-11-19 05:23
Rui, are you likely to be able to work on this in the next month or so? If not, are you happy for me to take it?

@nvaccessAuto
Copy link
Author

Comment 20 by ragb (in reply to comment 19) on 2013-11-24 12:18
Replying to jteh:

Rui, are you likely to be able to work on this in the next month or so? If not, are you happy for me to take it?

Hi,
I'd like to say yes, I will do it, but I'm not really sure if I have the time (software estimates, software estimates...). As yo know, I already have a branch with some code. I'll try hacking into it a bit more next weeks, at least the non-gui part (we both hate that). If you want to take it, of course I'll be happy with it, as I' really not able to promise anything.

Regards,

Rui Batista

@nvaccessAuto
Copy link
Author

Comment 22 by jteh on 2013-12-04 08:28
Rui, I'm taking this for now, but feedback and code is still very much welcome!

@nvaccessAuto
Copy link
Author

Comment 23 by jteh on 2013-12-05 05:25
How important is manual probing for displays; e.g. serial? it's a valid feature, but I think we should implement it separately once this is complete. The reasons are that i think it is less important (so lower priority) and I don't see any reason to block plug-and-play for it.

@nvaccessAuto
Copy link
Author

Comment 24 by ragb (in reply to comment 23) on 2013-12-05 11:28
Replying to jteh:

How important is manual probing for displays; e.g. serial? it's a valid feature, but I think we should implement it separately once this is complete. The reasons are that i think it is less important (so lower priority) and I don't see any reason to block plug-and-play for it.

Plug-and-play is far more important, IMO. Serial ports are an older technology, although I believe we should support it, in this case I don't see it as a huge priority.

BTW, thanks for taking this, I'm really struggling with IOS development these days...

Regards,

Rui Batista

@nvaccessAuto
Copy link
Author

Comment 25 by ragb on 2013-12-11 11:39
Hi,

Have just read the last commits. Here is some feedback:

  1. I was expecting many more changes to my initial probing implementation :). I see you are caching all ports when _probe is called. I suppose it is to be used only for checking if there are ports to poll, and poll at time intervals when there are. However, imagine the following situation with the FS driver, as an example:
  • You don't have any display connected via bluetooth, so no ports are to be polled.
  • You pair the display with the computer, or it gets connected somehow (user enables bluetooth for instance).
  • AS there were no ports to poll, the prober is not triggered unless a device notification is sent (which may happen somehow when you pair or enable bluetooth, if so my problem doesn't exist).
  • In this situation, does the user have to re-enable automatic detection, restart NVDA, or something? Or am I missing something obvious? :).
  1. I like the port refactoring. We had many duplication in automatic port handling in some drivers, and this makes this responsibility of the braille subsystem itself, so code is not scattered. It also simplifies probing of course.
  2. There are some strings drivers (handitech for instance) which maybe tricky to rewrite. I may be able to hack on the braille note driver, if my device gets to work :).

The overall code seems very very good to me :). Manual probing, if gets to be done, is very straight forward with this.

@nvaccessAuto
Copy link
Author

Comment 26 by jteh on 2013-12-11 12:29
Pairing a device will add a Bluetooth com port, which will be treated as a device change, thus triggering a notify/poll probe, so that bit should be fine.

Stupidly, now that I've pretty much got all of the core code working, I've realised there is one major problem I'd never really considered. This means that all drivers will be imported whenever automatic detection is enabled. Since we want to do this by default, this is probably pretty bad. It seems to only increase memory footprint by a few mb, but this is still not nice.

So, believe it or not, I'm considering throwing all of this code away and starting from scratch. My current thinking is to have a central module which maps all relevant USB IDs and Bluetooth names to their corresponding drivers. This way, the driver only needs to be imported if a matching device is detected. To make things easier, the driver will contain this info in a specific format and the build system will scan all drivers and generate this central module. This should also be more efficient than constantly calling drivers.

If we do go with this, it pretty much eliminates the need for most of my current changes. The ports stuff is nice, but it probably doesn't serve any actual purpose with this proposed approach. This also avoids issues with strange drivers such as handyTech. Of course, this doesn't take manual probing into account. We'll probably go back to something more like your original code for that.

Thoughts?

@nvaccessAuto
Copy link
Author

Comment 27 by ragb (in reply to comment 26) on 2013-12-11 13:55
Hi,
Replying to jteh:

Pairing a device will add a Bluetooth com port, which will be treated as a device change, thus triggering a notify/poll probe, so that bit should be fine.

Ok, nice.

Stupidly, now that I've pretty much got all of the core code working, I've realised there is one major problem I'd never really considered. This means that all drivers will be imported whenever automatic detection is enabled. Since we want to do this by default, this is probably pretty bad. It seems to only increase memory footprint by a few mb, but this is still not nice.

It depends on how many are "few MB" :). As far as I know one can't properly unload modules in Python....

So, believe it or not, I'm considering throwing all of this code away and starting from scratch. My current thinking is to have a central module which maps all relevant USB IDs and Bluetooth names to their corresponding drivers. This way, the driver only needs to be imported if a matching device is detected. To make things easier, the driver will contain this info in a specific format and the build system will scan all drivers and generate this central module. This should also be more efficient than constantly calling drivers.

In theory it is more efficient and avoids the unneeded imports. However, can we know all the device ids for all drivers somehow? Won't it be kind of a maintenance nightmare?

If we do go with this, it pretty much eliminates the need for most of my current changes. The ports stuff is nice, but it probably doesn't serve any actual purpose with this proposed approach. This also avoids issues with strange drivers such as handyTech. Of course, this doesn't take manual probing into account. We'll probably go back to something more like your original code for that.

Yes. Note that I like your approach, as far as I know bratty uses something similar in Linux, but I'm afraid of complexities like manufactures don't providing proper USB ids (and we having no access to devices or windows device drivers to get them).

Thoughts?

Above :).

Regards,

Rui Batista

@nvaccessAuto
Copy link
Author

Comment 29 by bramd on 2014-12-21 09:42
What's the status of this? I would like to see this implemented and would like to write some code to get it done.

I use a Brailliant display on the road and a Handytech display at the office. The Handytech driver probes quite aggressively (might have something to do with my current driver settings as well...). So, a system that only initializes that driver if a Handytech device is connected and switches to my brailliant automagically is very welcome.

@nvaccessAuto
Copy link
Author

Comment 30 by jteh on 2014-12-21 23:36
The current code is in the branch t1271a2. I got quite far with it, but got pulled away to other work and haven't had a chance to revisit it.

To be honest, I actually can't quite remember what problems still exist, though I know there was still work to do. I think one major problem is that there are some displays which stupidly use generic USB ids which might also be used by other displays, so the code needs to support trying more than one driver per USB id, which it doesn't currently support.

@nvaccessAuto
Copy link
Author

Comment 31 by jteh on 2014-12-21 23:38
Oh, and of course, some of the drivers still need to be updated and detection data added. In particular, auto detection of Handy Tech is not yet supported and I also need to make the driver fail properly when no display is connected instead of succeeding and silently waiting for a display. This is all fairly tricky since I don't have access to a Handy Tech device.

@nvaccessAuto
Copy link
Author

Comment 32 by dkager on 2015-08-09 11:09
This is another really useful feature. I still have to look at the diff, but there's some good stuff in the comments for sure.

I find this most interesting for the use case of unplugging your display, say to move your laptop somewhere, then re-attaching it and being able to continue working without restarting NVDA. Thus even if you were to turn off auto-detection it would be nice if individual drivers supported re-connecting. Or of course you could leave auto-detection enabled and that should take care of things. This leads to the suggestion that auto-detection always starts with the previously active display.

Keeping the above in mind I think the configuration file should save the last detected display, if only to avoid probing for other ones. It is likely that the previous display is still the current one, e.g. on a desktop PC with a braille display.

Another way to avoid importing all drivers (and maybe also some confusion) is to allow users to check the displays they use and only probe for those. The default would be to probe all displays.

Just a few initial thoughts. :)

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Sep 7, 2017

After some thinking, I'm not sure whether this fixes our issue.

When initializing a display from the background thread, that thread is currently busy with initializing, and in the initializing process, it calls waitForRead, which makes the background thread itself block. I assume that when queueing an overlapped completion routine when initializing, the initialization (which is the current apc) should have returned in order for the overlapped completion routine to be executed. However, we want to execute that routine during initialization, and not after it, as at that point, initialization has already failed.

Or, and probably I''ve just missed this, will the completion routine just interrupt the current running APC? I should study computer science one day.

@jcsteh
Copy link
Contributor

jcsteh commented Sep 7, 2017

@leonardder commented on 7 Sep 2017, 19:26 GMT+10:

Is there something better than subtracting time.time() before the wait call from time.time() after the wait call to calculate the remaining time for a next wait?

That's the only way I can think of.

@leonardder commented on 7 Sep 2017, 20:14 GMT+10:

When initializing a display from the background thread, that thread is currently busy with initializing, and in the initializing process, it calls waitForRead, which makes the background thread itself block. I assume that when queueing an overlapped completion routine when initializing, the initialization (which is the current apc) should have returned in order for the overlapped completion routine to be executed.

That's a good question. I actually had to go look this up myself. From the documentation for QueueUserAPC:

If you perform an alertable wait inside an APC, it will recursively dispatch the APCs. This can cause a stack overflow.

I think the stack overflow bit is only relevant if you have too many levels of this, but we should only ever go two APCs deep, so we should be fine. There's no reason we should ever run another init function within the first init function.

Btw, the documentation confirms that this also applies to IO completion functions:

Note that the ReadFileEx, SetWaitableTimer, and WriteFileEx functions are implemented using an APC as the completion notification callback mechanism.

I should study computer science one day.

This probably wouldn't be taught in computer science. :)

@LeonarddeR
Copy link
Collaborator

Thanks for elaborating!

I've successfully ported probing to the bg thread, only one nut left to crack:

DEBUGWARNING - WX Widgets (18:59:17.417):
..\..\src\common\timerimpl.cpp, line 60:
assert wxThread::IsMain(): timer can only be started from the main thread
Stack trace:
  File "C:\Python27\lib\threading.py", line 774, in __bootstrap
    self.__bootstrap_inner()
  File "C:\Python27\lib\threading.py", line 801, in __bootstrap_inner
    self.run()
  File "C:\Python27\lib\threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "braille.py", line 1923, in func
    winUser.MWMO_ALERTABLE,
  File "bdDetect.py", line 213, in _bgScan
    self._callLater.Start(POLL_INTERVAL, self._startBgScan, bluetooth=True)
  File "D:\NVDA_source\nvda\miscDeps\python\wx\_core.py", line 16816, in Start
    self.timer.Start(self.millis, wx.TIMER_ONE_SHOT)
  File "D:\NVDA_source\nvda\miscDeps\python\wx\_misc.py", line 1324, in Start
    return _misc_.Timer_Start(*args, **kwargs)
  File "core.py", line 233, in OnAssert
    log.debugWarning(message,codepath="WX Widgets",stack_info=True)

This is the wx.CallLater that executes a back ground scan every 10 seconds. Although it should only be started from the main thread, it seems to run just fine, but it shows this debug warning every 10 seconds, which is well, not what we want.

Do we want to have the timer run in the background thread? If not, which is the easiest method, we can just wrap the wx.CallLater in a wx.CallAfter in such a way that we can still stop the CallLater if desired. It executes _startBgScan which itself queues the actual probing apc in the background thread anyway. However, if we have a special background thread, why not have the timer run on that thread. I'm not sure how to do this though, probably using a waitable timer, which executes apc functions.

@jcsteh
Copy link
Contributor

jcsteh commented Sep 7, 2017

assert wxThread::IsMain(): timer can only be started from the main thread

Oo. Yes, this is bad and definitely needs to be fixed. While starting timers from background threads does seem to work, the fact that it causes wx assertions suggests it could have nasty side effects. We fixed several instances of this last year and suspect this may have led to unexplained crashes in the past.

This is the wx.CallLater that executes a back ground scan every 10 seconds. Although it should only be started from the main thread,

Not sure if you mean it's only ever started from the main thread or that wx requires that it only be started from the main thread. This part of the stack confirms it's definitely being started in the background thread:

  File "bdDetect.py", line 213, in _bgScan
    self._callLater.Start(POLL_INTERVAL, self._startBgScan, bluetooth=True)

As you suggest, I think having the timer in the background thread makes the most sense, since the thread is already there. I think using a waitable timer makes sense. Note that you'll need to set the timer in the background thread in all cases so that it gets executed there. Also, ensure it is a one shot timer (lPeriod 0) to avoid the timer accidentally running during a waitForRead, which would be... very bad.

@LeonarddeR
Copy link
Collaborator

I consider the code in the @BabbageCom i1271 branch to be feature complete, but before I file a pr, here is an overview of the changes since @jcsteh worked on this. Several things have changed, as @jcsteh's implementation was pre hwIo. Most notably:

  1. hwIO.IoBase._recvEvt is no longer a Python event, since threading.Event.wait blocks the calling thread, including queued APCs.

  2. hwIO.IoBase.waitForRead no longer waits for hwIO.IoBase._recvEvt to be set, but waits using the kernel32.WaitForSingleObjectEx method. This allows braille display initialization on the background thread. If this change wasn't made, waitForRead would be run on the background thread but would also block all background read attempts, effectively causing no reading to occur at all. :)

  3. Bluetooth polling is now every 5 seconds instead of 10. In practice, probing ports that represent devices out of range will cause an increase of this delay, so it will hardly be five seconds in reality.

  4. Braille display auto detection no longer uses its own thread but makes use of braille._BgThread. Display driver initialization, instead of being queued to the main thread, is now done entirely on this background thread. Since the background thread now knows about a succesful or failed display initialization, support for one USB identifier being defined for multiple drivers should theoretically be guaranteed. I've actually seen one case of this, namely Hims SyncBraille and Baum VarioPro. The latter is not yet supported by NVDA.

  5. In the old situation, a braille display could have USB and Bluetooth definitions. In the current situation, USB definitions are split up between USB HID, USB Serial and USB Custom. The latter will be used for native Hims (Native driver for Hims displays #7712) and Freedom Scientific drivers, both work in progress. It will also be fairly easy to add Bluetooth HID support to bluetooth probing.

  6. The bdDetect module introduces a new concept called a device match, a namedtuple with several pieces of relevant information to make a connection to a device. In the old situation, we had two types of device matches:

    • UsbDeviceMatch(id)
    • BluetoothComPortMatch(address", name, port)

    In the current situation, we have only one device match named tuple: DeviceMatch(type","id, port, deviceInfo). Here, type is one of HID, Serial or Custom. Bluetooth device matches currently have type Serial, but you can easily check whether it is a bluetooth port by checking "bluetoothName" in match.deviceInfo.

    Note that it has been discussed with @jcsteh to subclass DeviceMatch, rather than adding a type slot to the named tuple. I have eventually made the decision not to do this, as per the following reasons.

    A. USB definitions are split up into three categories, and these three categories are saved in a dictionary using the same key constants that are used in the named tuples as the device type. So, whatever we do, unless I'm missing something, we need type constants.
    B. Due to the point above, using subclasses would require tieing subclasses to type constants as well as multiple isinstance checks, and thus would require several additional lines of code, which is unnessesary if we just use type constants within the named tuples.

Additional ideas are welcome, though.

  1. Alva and Freedom Scientific displays are excluded from auto detection for now, since we aren't sure whether they are thread safe. In contrast, Handy Tech has been added. The new native drivers for Eurobraille, Hims and Freedom Scientific displays will support bdDetect in the future.

  2. The following methods have been added to the braille.BrailleDisplayDriver class:

    • _newWithSupportedKwargs: Creates a new instance of a BrailleDisplayDriver, but only with the keyword args it supports. This, although officially unsupported, might ease downgrading to drivers without a port setting in the future. See Native driver for Handy Tech braille displays #7590 (comment) and surrounding comments.
    • _getAutoPorts: Returns possible ports to connect to using L{bdDetect} automatic detection data. Either USB, Bluetooth or both.
    • _getTryPorts(port): Generic function that yields device matches based on the provided port. Port is either a deviceMatch or a string. If a string, should be one of "automatic", "usb", "bluetooth" or a specific Com port. This method yields device matches. To give an example of its functionality, see the below code snippet from the Baum driver.
    def __init__(self, port="auto"):
    	super(BrailleDisplayDriver, self).__init__()
    	self.numCells = 0
    	self._deviceID = None
    
    	for portType, portId, port, portInfo in self._getTryPorts(port):
    		# At this point, a port bound to this display has been found.
    		# Try talking to the display.
    		self.isHid = portType == bdDetect.KEY_HID
    		......
    

    Subclasses can of course extend this method, which is currently the case for the brailliantB driver. It is very likely that the brailliantB override can be removed, though.

  3. hwPortUtils.listUsbDevices now yields dictionaries instead of just strings containing the USB ID. The yielded dictionaries contain the USB ID, Hardware ID and device path. The latter is useful for USB Bulk devices.

@LeonarddeR
Copy link
Collaborator

@michaelDCurran: I forgot to mention that your feedback on the above comment would also be appreciated. Also @Qchristensen, @josephsl and others, it would be great if you could share your opinion on how you think braille display detection would fit into the current documentation (e.g. should we just list the supported displays in a separate paragraph, or should we add a note to every specific display paragraph)?

@zstanecic
Copy link
Contributor

zstanecic commented Nov 7, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Nov 7, 2017 via email

LeonarddeR pushed a commit to BabbageCom/nvda that referenced this issue Nov 11, 2017
…sm can be activated by choosing the Automatic option from NVDA's braille display settings. See nvaccess#1271 for an in depth discussion of funcionality.
LeonarddeR pushed a commit to BabbageCom/nvda that referenced this issue May 31, 2018
…sm can be activated by choosing the Automatic option from NVDA's braille display settings. See nvaccess#1271 for an in depth discussion of funcionality.

* brailliantB, use generic implementation for USB serial devices
* Use generic check function for braille display drivers supporting bdDetect
* Make auto detection the default
* Set a timeout for joining the bgThread when auto detection was on
* Poll for bluetooth devices on app switches
* Support bluetooth HID in bdDetect
LeonarddeR pushed a commit to BabbageCom/nvda that referenced this issue May 31, 2018
…sm can be activated by choosing the Automatic option from NVDA's braille display settings. See nvaccess#1271 for an in depth discussion of funcionality.

* brailliantB, use generic implementation for USB serial devices
* Use generic check function for braille display drivers supporting bdDetect
* Make auto detection the default
* Set a timeout for joining the bgThread when auto detection was on
* Poll for bluetooth devices on app switches
* Support bluetooth HID in bdDetect
@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Jul 17, 2018
michaelDCurran pushed a commit that referenced this issue Jul 17, 2018
* Implement braille display auto detection. This new background mechanism can be activated by choosing the Automatic option from NVDA's braille display settings. See #1271 for an in depth discussion of funcionality.

* brailliantB, use generic implementation for USB serial devices
* Use generic check function for braille display drivers supporting bdDetect
* Make auto detection the default
* Set a timeout for joining the bgThread when auto detection was on
* Poll for bluetooth devices on app switches
* Support bluetooth HID in bdDetect

* Use a separate thread for background scanning.

* Make the bdDetect thread a daemon thread

* Disable auto detection within the unit test framework. If we don't do this, detection will occur and unit tests will fail.

* in braille.handler.handleGainFocus, check whether the focused object has an active tree interceptor. If so, focus the tree interceptor instead

* Revert the use a separate thread for background scanning and make sure recursion does not occur when using an APC

This reverts commit 5b97f39.

* Created Detector._scanQueuedSafe which wraps changing the state of _scanQueued within a lock

* Fix malformed VID and PID for Brailliant, add an extra check

* NO longer filter serial ports for Brailliant

* Updated changes.t2t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants