Skip to content

Windows detection update #281

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 21, 2018
Merged

Windows detection update #281

merged 8 commits into from
Feb 21, 2018

Conversation

bridadan
Copy link
Contributor

This was made in response to #279.

The Windows registry is responsible for keeping track of all USB
devices. Previously, we were using a portion of the registry that is
always cached, even if a device is removed. This caused certain network
drives to appear as mbeds if they attached to a previously attached
mbed's drive letter (network drives don't appear to update the registry).

Now we are checking the enumeration part of the registry to ensure the
device is actually plugged in before return it as a result.

This also has the benefit that we no longer need to call the subprocess dir for each potentially found device. This translate to an execution speed up! My preliminary tests show the speed up is on the order of 50-100ms.

@bridadan
Windows API can now tell if mbeds are actually plugged in
cf2c1cd 
The Windows registry is responsible for keeping track of all USB
devices. Previously, we were using a portion of the registry that is
always cached, even if a device is removed. This caused certain network
drives to appear as mbeds if they attached to a previously attached
mbed's drive letter (network drives don't appear to update the registry).

Now we are checking the enumeration part of the registry to ensure the
device is actually plugged in before return it as a result.
@bridadan
Fixing typo in docstring
a1561e0
@bridadan
Clean up of prints and code in windows platform
31081cd
@bridadan
Adding tests for new Windows detection
b0d3386
@coveralls
Copy link

coveralls commented Dec 14, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.8%) to 75.061% when pulling b0d3386 on bridadan:windows_detection_update into 29e9179 on ARMmbed:master.

@bridadan
Copy link
Contributor Author

This could use a little more testing, specifically on Windows 7 since I've been testing on Windows 10

@bridadan
Copy link
Contributor Author

Ok, I've tested this on a Windows 7 VM and I'm reasonably confident that this isn't breaking any of the detection!

@bridadan bridadan changed the title Windows detection update [DON'T MERGE] Windows detection update Dec 18, 2017
@bridadan
Copy link
Contributor Author

bridadan commented Dec 18, 2017 •
edited
Loading

There's apparently an issue with the detection with this update, just got an incorrect serial port name using this branch, so let's hold off on this for now.

@coveralls
Copy link

coveralls commented Dec 18, 2017 •
edited
Loading

Coverage Status

Coverage increased (+1.9%) to 76.23% when pulling 2dae79a on bridadan:windows_detection_update into 29e9179 on ARMmbed:master.

@bridadan
Copy link
Contributor Author

I believe I've patched the issue, however I still need to test with a bunch of hardware again to make sure I didn't break anything. I probably should document the full detection strategy at the top of the file too.

@bridadan bridadan changed the title [DON'T MERGE] Windows detection update Windows detection update Jan 5, 2018
@bridadan
Copy link
Contributor Author

bridadan commented Jan 5, 2018

Ok, I've ran this a bunch of hardware on my desk and things didn't break, so I'm reasonably happy with this at the moment. Open to review!

theotherjimmy
theotherjimmy reviewed Feb 9, 2018
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

I had a few comments waiting!

mbed_lstools/windows.py Outdated
for name, vid in zip(self.iter_keys_as_str(usb_devs),
self.iter_keys(usb_devs)):

def composite_device_key(device):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this method-private funciton into a private static method?

mbed_lstools/windows.py
# These keys in the registry enumerate all composite DosDevices
# as a value with an integer. This check ensures we ignore a few
# helper values in the registry key.
_ = int(point)
Copy link
Contributor

@theotherjimmy theotherjimmy Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You might need point.decode("utf-16le", "ignore") here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that and it broke the whole thing on my machine lol. For debugging this I added this code:

logger.warning(point)
logger.warning(point.decode("utf-16le", "ignore"))
logger.warning('------')
_ = int(point)
$ mbedls
WARNING:mbedls.lstools_win7:0
WARNING:mbedls.lstools_win7:
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:Count
WARNING:mbedls.lstools_win7:潃湵
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:NextInstance
WARNING:mbedls.lstools_win7:敎瑸湉瑳湡散
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:1
WARNING:mbedls.lstools_win7:
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:Count
WARNING:mbedls.lstools_win7:潃湵
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:NextInstance
WARNING:mbedls.lstools_win7:敎瑸湉瑳湡散
WARNING:mbedls.lstools_win7:------
WARNING:mbedls.lstools_win7:0
WARNING:mbedls.lstools_win7:
WARNING:mbedls.lstools_win7:------

If you think I need to try different decode options, just let me know!

mbed_lstools/windows.py
except ValueError:
continue

label_parts = label.split('\\')
Copy link
Contributor

@theotherjimmy theotherjimmy Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You might need label.decode("utf-16le", "ignore").split('\\') here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto here as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually strike that, doesn't seem to break anything.

mbed_lstools/windows.py
try:
composite_parent_id_prefix, _ = winreg.QueryValueEx(composite_key,
'ParentIdPrefix')
parent_id_prefixes.append(composite_parent_id_prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

decode?

@bridadan
Copy link
Contributor Author

bridadan commented Feb 9, 2018

@theotherjimmy and I went through this offline, looks like it doesn't need decode. decode in fact breaks it if ran with Python 3!

@bridadan
Copy link
Contributor Author

bridadan commented Feb 9, 2018

@theotherjimmy I went over this again and I think everything is ok. I fixed the static function you mentioned above. I also added back the mount_point_ready implementation in Windows to use the dir command to prevent the Python pop box that occurs on accesses to mbed that are in the "ejected" state. This works fine when not using -u, but still occurs when that flag is used.

I think I need to get a test in for this implementation of mount_point_ready otherwise coverage will go down.

@bridadan
Copy link
Contributor Author

Ok, test coverage added!

theotherjimmy
theotherjimmy approved these changes Feb 21, 2018
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the test updates!

@theotherjimmy theotherjimmy merged commit f866fbe into ARMmbed:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherjimmy theotherjimmy theotherjimmy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bridadan @coveralls @theotherjimmy