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

fix(app): tighten filter logic that identifies Realtek U2E adapters #5707

Merged
merged 1 commit into from
May 20, 2020

Conversation

mcous
Copy link
Contributor

@mcous mcous commented May 20, 2020

overview

As was pointed out in the PR that added USB device collection to the app, our filter for determining if a given device was a Realtek U2E adapter consisted of asking "Does this device report that its manufacturer is the string 'Realtek'?" This proved to be insufficient pretty quickly, because Realtek makes devices that are not U2E adapters.

This PR tightens the filter to check:

  • Does the Vendor ID of the USB device match Realteks's registered VID?
  • Is the Product ID close to the registered PID of an RTL8150
    • Note: in real world testing + collected analytics, our devices report a PID that is slightly off from what is expected (expected: 0x8150, actual: 0x8050)
    • Hence, "close to" rather than exact match

changelog

  • fix(app): tighten filter logic that identifies Realtek U2E adapters

review requests

Run the smoke test from #5656, especially if you, like @nusrat813, have Realtek devices on your machine that are not U2E adapters.

risk assessment

Low! Change is well contained and well (unit) tested

I've also analyzed the limited analytics data we have collected since the 3.17.1 release, and I've found that:

  • The vendor ID of all U2E devices reported is correct, so I'm comfortable with the exact match as implemented in this PR
  • Several different product IDs have been reported, and they would be caught by the regexp implementation in this PR
  • Our existing filter is definitely too loose! We've got several other Realtek devices erroneously reported by the app U2E adapters (e.g. card readers, bluetooth adapters, etc.)

@mcous mcous added app Affects the `app` project ready for review fix PR fixes a bug robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). qa QA input / review required labels May 20, 2020
@mcous mcous requested a review from a team as a code owner May 20, 2020 14:52
@mcous mcous requested review from shlokamin, a team, amitlissack and sanni-t and removed request for a team May 20, 2020 14:52
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

❗ No coverage uploaded for pull request base (release_3.18.0@3e6f054). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##             release_3.18.0    #5707   +/-   ##
=================================================
  Coverage                  ?   78.61%           
=================================================
  Files                     ?      182           
  Lines                     ?    17974           
  Branches                  ?        0           
=================================================
  Hits                      ?    14131           
  Misses                    ?     3843           
  Partials                  ?        0           
Impacted Files Coverage Δ
opentrons/protocol_api/module_geometry.py 88.42% <0.00%> (ø)
opentrons/protocol_api/util.py 90.90% <0.00%> (ø)
opentrons/protocols/types.py 94.59% <0.00%> (ø)
opentrons/protocol_api/instrument_context.py 87.99% <0.00%> (ø)
...bot-server/robot_server/service/routers/session.py 100.00% <0.00%> (ø)
...t_server/service/models/json_api/resource_links.py 100.00% <0.00%> (ø)
opentrons/legacy_api/containers/placeable.py 93.27% <0.00%> (ø)
opentrons/protocols/parse.py 93.24% <0.00%> (ø)
...bot-server/robot_server/service/routers/modules.py 98.50% <0.00%> (ø)
...obot-server/robot_server/service/models/modules.py 100.00% <0.00%> (ø)
... and 172 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e6f054...cf38271. Read the comment docs.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

It looks good to me and i'll test. As a sidenote, I think the only really long-term-viable way we can do this without continual little weird updates is to use [a library with more usb stack introspection capability which may be this one but may not be since I just found it recently and you probably saw it already, so that we can filter on

  • manufacturer is realtek, by vid
  • device supports a the correct usb device class which I think is CDC/ECM per usb spec

Copy link

@nusrat813 nusrat813 left a comment

Choose a reason for hiding this comment

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

Tested - working as intended

@mcous mcous merged commit ea9a6c4 into release_3.18.0 May 20, 2020
@mcous mcous deleted the app_tighten-realtek-filter branch May 20, 2020 16:52
@mcous mcous mentioned this pull request May 20, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project fix PR fixes a bug qa QA input / review required robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants