fix(api): update USB port & hub mapping logic for Flex #12775

pmoegenburg · 2023-05-23T23:01:32Z

Overview

Previously, we represented USB-connected modules with the data structure (hub, port). We will now represent USB-connected modules with the data structure (hub, port_group, port, hub_port). On the OT-2_OG, we have 1 USB port bank with 2 ports for connecting modules. On the OT-2_R, we have 1 port bank with 2 ports for connecting modules. On the Flex, we have 3 port banks (left and right with 4 ports each, front with 1 port) for connecting modules.

If a USB hub/extender is attached to a port, the port's hub data field will be True and the hub_port data field will be populated with the port on the USB hub. If a hub is not attached to a port, the port's hub data field will be False and the hub_port data field will be None.

On the Flex, the left port bank USB ports are numbered 1 to 4 from top to bottom and the right port bank USB ports are numbered 5 to 8 from top to bottom. The data structure ports match these. The front port bank port is always 1.

Test Plan

verify modules on left show up properly
verify modules on right show up properly
verify USB-extender with module on right and module on left show properly
verify USB-extender with module on left and module on right show properly
- >>> api._backend.module_controls._usb.match_virtual_ports(new_mods)
  [ModuleAtPort(port='/dev/ot_module_tempdeck2', name='tempdeck', usb_port=USBPort(name='1-1.3.1', port_number=8, port_group=<PortGroup.RIGHT: 3>, hub=False, hub_port=None, device_path='1.0/tty/ttyACM2/dev')), ModuleAtPort(port='/dev/ot_module_heatershaker1', name='heatershaker', usb_port=USBPort(name='1-1.4.1.4', port_number=4, port_group=<PortGroup.LEFT: 2>, hub=True, hub_port=4, device_path='1.0/tty/ttyACM1/dev'))]
verify module on left and USB-extender with module on front show properly
- >>> api._backend.module_controls._usb.match_virtual_ports(new_mods)
  [ModuleAtPort(port='/dev/ot_module_tempdeck2', name='tempdeck', usb_port=USBPort(name='1-1.7.4', port_number=1, port_group=<PortGroup.FRONT: 4>, hub=True, hub_port=4, device_path='1.0/tty/ttyACM2/dev')), ModuleAtPort(port='/dev/ot_module_heatershaker1', name='heatershaker', usb_port=USBPort(name='1-1.4.4', port_number=1, port_group=<PortGroup.LEFT: 2>, hub=False, hub_port=None, device_path='1.0/tty/ttyACM1/dev'))]

Changelog

Review requests

Risk assessment

codecov · 2023-05-23T23:08:56Z

Codecov Report

Merging #12775 (5f68702) into edge (5bc8eae) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge   #12775      +/-   ##
==========================================
+ Coverage   73.28%   73.32%   +0.03%     
==========================================
  Files        2317     2315       -2     
  Lines       63500    63117     -383     
  Branches     6973     6973              
==========================================
- Hits        46537    46278     -259     
+ Misses      15307    15183     -124     
  Partials     1656     1656

Flag	Coverage Δ
app	`71.56% <ø> (ø)`
components	`67.89% <ø> (ø)`
g-code-testing	`96.44% <ø> (ø)`
labware-library	`49.61% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`46.35% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/src/opentrons/drivers/rpi_drivers/types.py	`87.87% <ø> (-1.28%)`	⬇️
...entrons/hardware_control/backends/ot3controller.py	`66.82% <ø> (-0.16%)`	⬇️
...ardware_control/emulation/module_server/helpers.py	`72.41% <ø> (ø)`
...i/src/opentrons/hardware_control/module_control.py	`72.64% <ø> (ø)`
.../src/opentrons/hardware_control/modules/mod_abc.py	`81.00% <ø> (-0.56%)`	⬇️
api/src/opentrons/hardware_control/types.py	`96.37% <ø> (ø)`
...-server/robot_server/modules/module_data_mapper.py	`95.83% <ø> (ø)`
robot-server/robot_server/modules/module_models.py	`100.00% <ø> (ø)`
...rver/robot_server/service/legacy/models/modules.py	`100.00% <ø> (ø)`
...ver/robot_server/service/legacy/routers/modules.py	`98.38% <ø> (ø)`

... and 22 files with indirect coverage changes

…y tested

sfoster1

I think the structure is good but we need some changes.

We should test the new code.
We should make sure we can still handle someone plugging an external hub into one of the USB ports on the OT3
We should make the port group type an Enum
We should make sure we understand how this new data is represented via HTTP

sfoster1 · 2023-05-25T15:38:03Z

api/src/opentrons/drivers/rpi_drivers/types.py

@@ -22,6 +24,7 @@ class USBPort:
 port_number: int
 device_path: str = ""
 hub: Optional[int] = None
+ port_group: str = ""


I think we really want this to be an enumeration since it has a fixed number of possible values. We'll need to represent it as a string eventually in the HTTP layer, but we can cross that bridge when we come to it. We should make the enum handle the left, the right, and "main" (for OT-2) and probably even "front" (for OT-3 - just in case).

sfoster1 · 2023-05-25T15:38:17Z

api/src/opentrons/drivers/rpi_drivers/types.py

@@ -103,18 +112,27 @@ def get_unique_nodes(full_name: str) -> List[str]:
 @staticmethod
 def map_to_revision(
 board_revision: BoardRevision, port_info: Tuple[Optional[int], int]
- ) -> Tuple[Optional[int], int]:
+ ) -> Tuple[Optional[int], str, int]:


this would be a good place for us to return the enum

sfoster1 · 2023-05-25T15:39:15Z

api/src/opentrons/drivers/rpi_drivers/types.py

+ else:
+ return hub, "MAIN", REV_OG_USB_PORTS.get(str(port), port)
+ elif board_revision == BoardRevision.B2:
+ # what will hub look like??


what does it look like?

sfoster1

This is looking really good! I think we should make the revision name for the OT-3 variants include that it's an ot3 - honestly, this driver code should probably leave the rpi_drivers section but we don't need to do that in this PR - and maybe add some comments noting in more depth what's going on with the usb string splitting in map_to_revision.

Also, let's add some tests that explicitly check that strings captured from a running OT-3 prase properly, for left and right with no hubs and left and right with hubs

sfoster1 · 2023-05-26T17:39:25Z

api/src/opentrons/drivers/rpi_drivers/types.py

 hub: Optional[int] = int(port_info[1])
 port = int(port_info[2])
+ hub_port: Optional[int] = int(port_info[3])


this would want to be port_info[-1] maybe?

sfoster1 · 2023-05-26T17:40:24Z

api/src/opentrons/hardware_control/types.py

@@ -458,6 +458,7 @@ class BoardRevision(enum.Enum):
 A = enum.auto()
 B = enum.auto()
 C = enum.auto()
+ B2 = enum.auto()


can we maybe make this FLEX_B2 or something?

sfoster1

Looks good to me once we fix up the tests!

sfoster1

Looks great to me if tests all pass, but let's make sure @sanni-t or another RSS person gets a glance - possibly an in-person demo?

pmoegenburg · 2023-06-06T20:16:46Z

robot-server/tests/integration/test_modules.tavern.yaml

@@ -103,6 +103,7 @@ stages:
 moduleModel: thermocyclerModuleV1
 usbPort:
 port: !anyint
+ hub: !anyint


@sfoster1 will this become an int? It is a bool in the api

When sent via http, the type is expressed as determined in the response model which would also have to change. that would then be a breaking change, which would be rough.

You can see there that the hub field is optional in the model and setup by this data: https://github.com/Opentrons/opentrons/blob/4445d808e5401c9283c8af780bb7e1ddee65a162/robot-server/robot_server/service/legacy/routers/modules.py#LL36C51-L36C51

sfoster1

Looks good pending seeing what this breaks in app interactions if anything

sanni-t

This looks good mostly. Just some minor comments.
I tested this on a flex and it works great! Didn't test on ot2 but I'm assuming it'll have similar app-side change requirements due to the hub/port structure (& type) change? (and that'll be a slight breaking change/ introduce incompatibility with older app).
As for the app changes, this is how the app shows usb ports with your change-

sanni-t · 2023-06-08T17:23:27Z

robot-server/robot_server/service/legacy/models/modules.py

+ "usbPort": {
+ "hub": None,
+ "port": None,
+ "port_group": None,
+ "hub_port": None,
+ },


We want to make sure the example structure is valid acc to the defined models-

Suggested change

"usbPort": {

"hub": None,

"port": None,

"port_group": None,

"hub_port": None,

},

"usbPort": {

"hub": 1,

"port": 1,

"portGroup": 1,

"hubPort": None,

},

If we change port group to have string values, then will need to update that here too.

Also, I think the "port" field above "usbPort" might have been added here by mistake. We should remove it since a real response can't contain both 'port' (virtual port) and a 'usbPort'.

api/src/opentrons/drivers/rpi_drivers/types.py

robot-server/robot_server/service/legacy/models/modules.py

robot-server/robot_server/modules/module_models.py

debugging update

a492ccf

pmoegenburg self-assigned this May 23, 2023

Added OT3 USB left and right port group determination and successfull…

85ea3b0

…y tested

pmoegenburg marked this pull request as ready for review May 24, 2023 22:37

pmoegenburg requested a review from a team as a code owner May 24, 2023 22:37

pmoegenburg requested review from sfoster1 and sanni-t May 24, 2023 22:38

pmoegenburg added api Affects the `api` project fix PR fixes a bug labels May 24, 2023

sfoster1 requested changes May 25, 2023

View reviewed changes

pmoegenburg added 5 commits May 25, 2023 15:40

handle external USB hub

f7dbd7a

Made PortGroup enum (previously a str)

4bab8f4

removed unneeded comment

57d3da6

test fix

d5ed78e

linted

0b15964

pmoegenburg requested a review from sfoster1 May 26, 2023 16:54

sfoster1 requested changes May 26, 2023

View reviewed changes

pmoegenburg added 7 commits May 31, 2023 17:27

made Flex BoardRevision Flex-specific, fixed and tested OT2-R USB data

afc67d7

Merge branch 'edge' into RET-1362_fix_usb_mapping_for_flex

525e1f7

fixed OT2-OG USB parsing and updated parsing test

d7cf9df

made hub boolean and mapped ports to labeled port numbers

9e0650e

removed unneeded code

bfb8546

fixed test

dd015a8

updated method descriptions

c0920e2

pmoegenburg requested a review from sfoster1 June 1, 2023 21:02

pmoegenburg added 2 commits June 1, 2023 17:18

fixed test

18e9a1d

Merge branch 'edge' into RET-1362_fix_usb_mapping_for_flex

61e42ae

sfoster1 reviewed Jun 2, 2023

View reviewed changes

Merge branch 'edge' into RET-1362_fix_usb_mapping_for_flex

24f6bfc

fixed test

70f622b

pmoegenburg requested a review from a team as a code owner June 2, 2023 22:54

pmoegenburg requested a review from sfoster1 June 4, 2023 04:52

sfoster1 approved these changes Jun 5, 2023

View reviewed changes

accounted for Flex front USB port and inverted side port numbering

e42d94e

pmoegenburg requested a review from sfoster1 June 6, 2023 20:15

pmoegenburg commented Jun 6, 2023

View reviewed changes

pmoegenburg added 3 commits June 6, 2023 16:37

Merge branch 'edge' into RET-1362_fix_usb_mapping_for_flex

3d6a148

fixed robot-server module model

19f8b52

robot-server test fixes

228840c

sfoster1 approved these changes Jun 7, 2023

View reviewed changes

updated PhysicalPort class

089c5c4

sanni-t reviewed Jun 8, 2023

View reviewed changes

pmoegenburg added 3 commits June 9, 2023 00:18

updated module model data field naming

db5056f

updated port group data type to string

07a3d65

Merge branch 'edge' into RET-1362_fix_usb_mapping_for_flex

5f68702

pmoegenburg merged commit cbfa12f into edge Jun 14, 2023
44 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): update USB port & hub mapping logic for Flex #12775

fix(api): update USB port & hub mapping logic for Flex #12775

pmoegenburg commented May 23, 2023 •

edited

Loading

codecov bot commented May 23, 2023 •

edited

Loading

sfoster1 left a comment

sfoster1 May 25, 2023

sfoster1 May 25, 2023

sfoster1 May 25, 2023

sfoster1 left a comment

sfoster1 May 26, 2023

sfoster1 May 26, 2023

sfoster1 left a comment

sfoster1 left a comment

pmoegenburg Jun 6, 2023

sfoster1 Jun 6, 2023

sfoster1 Jun 6, 2023

sfoster1 left a comment

sanni-t left a comment

sanni-t Jun 8, 2023

sanni-t Jun 8, 2023

fix(api): update USB port & hub mapping logic for Flex #12775

fix(api): update USB port & hub mapping logic for Flex #12775

Conversation

pmoegenburg commented May 23, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented May 23, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmoegenburg commented May 23, 2023 •

edited

Loading

codecov bot commented May 23, 2023 •

edited

Loading