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

hwPortUtils.listComPorts: Don't barf when SPDRP_FRIENDLYNAME is invalid #6462

Merged
merged 2 commits into from Nov 3, 2016

Conversation

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Oct 14, 2016

Fixed another rare issue when scanning for serial ports on some systems which made some braille display drivers unusable.

hwPortUtils.listComPorts: In some rare cases, the SPDRP_FRIENDLYNAME registry property doesn't exist/isn't valid. In these cases, just use the port name as the friendly name.
Originally reported by @joshknnd1982 in #6007 (comment)

While we're at it:

  • Also use PortName for FriendlyName on ERROR_INSUFFICIENT_BUFFER. We haven't seen it, but callers depend on FriendlyName, so we want to fall back if it does happen.
  • For extra safety, skip the port altogether if PortName is empty.
…ms which made some braille display drivers unusable.

hwPortUtils.listComPorts: In some rare cases, the SPDRP_FRIENDLYNAME registry property doesn't exist/isn't valid. In these cases, just use the port name as the friendly name.
Re #6007.
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Oct 14, 2016

@feerrenrut, mind taking a peek?

Copy link
Contributor

@feerrenrut feerrenrut left a comment

I added a couple of questions, I'll leave it up to you.

@@ -231,7 +231,9 @@ def __str__(self):
):
# Ignore ERROR_INSUFFICIENT_BUFFER
if ctypes.GetLastError() != ERROR_INSUFFICIENT_BUFFER:

This comment has been minimized.

@feerrenrut

feerrenrut Oct 14, 2016
Contributor

Unrelated to your change. While it might be unlikely I think it would be a good idea to log an insufficient buffer error.

raise ctypes.WinError()
# #6007: SPDRP_FRIENDLYNAME sometimes doesn't exist/isn't valid.
log.debugWarning("Couldn't get SPDRP_FRIENDLYNAME for %s: %s" % (port, ctypes.WinError()))
entry["friendlyName"] = port

This comment has been minimized.

@feerrenrut

feerrenrut Oct 14, 2016
Contributor

could port be an empty string? Would that matter?

@joshknnd1982
Copy link

@joshknnd1982 joshknnd1982 commented Oct 14, 2016

I'm so glad I was able to help you developer guys out with fixing NVDA
by providing error logs! I hope I can contribute more, in this way, in
the future! its fun! thanks for NVDA!

On 10/14/2016 12:16 AM, James Teh wrote:

Fixed another rare issue when scanning for serial ports on some
systems which made some braille display drivers unusable.

hwPortUtils.listComPorts: In some rare cases, the SPDRP_FRIENDLYNAME
registry property doesn't exist/isn't valid. In these cases, just use
the port name as the friendly name.
Originally reported by @joshknnd1982 https://github.com/joshknnd1982
in #6007 (comment)
#6007 (comment)


    You can view, comment on, or merge this pull request online at:

#6462

    Commit Summary


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6462, or mute the thread
https://github.com/notifications/unsubscribe-auth/APPbfZlOSuXVKSHJemMXTP1ton0HkBMDks5qzwItgaJpZM4KWn19.

mozilla thunderbird email client

…or FriendlyName, log a warning and fall back to PortName for ERROR_INSUFFICIENT_BUFFER too.
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Oct 18, 2016

@feerrenrut, I realised that we depend on friendlyName and it wasn't being set at all on ERROR_INSUFFICIENT_BUFFER. So, we now just treat ERROR_INSUFFICIENT_BUFFER the same as any other error for friendlyName. Also, just in case, I handle port being empty. Can you please take another look? Ta!

jcsteh added a commit that referenced this pull request Oct 20, 2016
@nvaccessAuto nvaccessAuto assigned jcsteh and unassigned feerrenrut Oct 20, 2016
@jcsteh jcsteh merged commit dbdc303 into master Nov 3, 2016
@nvaccessAuto nvaccessAuto removed the incubating label Nov 3, 2016
@nvaccessAuto nvaccessAuto added this to the 2016.4 milestone Nov 3, 2016
jcsteh added a commit that referenced this pull request Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants