fix(engine): include modules w/o labware in motion planning

[mcous]

Overview

This PR fixes a bug in ProtocolEngine's motion planning. Modules loaded into the engine without a labware would not be taken into account when calculating the safe travel height for arc movements. This could potentially lead to collisions during LPC if a protocol loaded modules in such a way.

Videos

In this setup, I placed a blue piece of paper over a magnetic module so you can see the travel height of the pipette as it arcs

release_6.0.0 / edge

IMG_1480.MOV

This branch

IMG_1481.MOV

Changelog

• When getting the tallest item on the deck, check all labware and all modules

Review requests

Reproduction procedure below. Compare this logic to the PAPIv2 motion planning logic in src/opentrons/protocols/geometry/planning.py

Risk assessment

My main question is if I've misunderstood something about thermocyclers. If I have, this could lead to spurious errors during. However, I'm not seeing any differences in max-Z calculations.

Steps to reproduce

I reproduced the buggy behavior using an HTTP protocol, since this mirrors what LPC will be doing.

• P300 single pipette on the right mount
• Tip rack loaded in slot 4
• Well plate loaded in slot 3
• Magnetic module loaded in slot 2 (but not physically placed there, because we're testing for collisions)

Procedure:

1. Create run
2. Home
3. Load P300 single onto the right mount
4. Load well plate into slot 3
5. Load tip rack into slot 4
6. Load magnetic module into slot 2
7. Pick up a tip (with a little offset because that's what I need on my robot)
8. Move to well A1 of well plate
9. Move back to well A1 of the tip rack

POST /runs
{"data": {}}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "home",
        "params": {}
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "loadPipette",
        "params": {
            "pipetteName": "p300_single",
            "mount": "right",
            "pipetteId": "pipette-id"
        }
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "loadLabware",
        "params": {
            "loadName": "corning_96_wellplate_360ul_flat",
            "version": 1,
            "namespace": "opentrons",
            "location": {"slotName": "3"},
            "labwareId": "well-plate-id"
        }
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "loadLabware",
        "params": {
            "loadName": "opentrons_96_tiprack_300ul",
            "version": 1,
            "namespace": "opentrons",
            "location": {"slotName": "4"},
            "labwareId": "tip-rack-id"
        }
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "loadModule",
        "params": {
            "model": "magneticModuleV2",
            "location": {"slotName": "2"},
            "moduleId": "module-id"
        }
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "pickUpTip",
        "params": {
            "labwareId": "tip-rack-id",
            "pipetteId": "pipette-id",
            "wellName": "A1",
            "wellLocation": {
                "offset": { "x": 1, "y": 1, "z": 0 }
            }
        }
    }
}

POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "moveToWell",
        "params": {
            "labwareId": "well-plate-id",
            "pipetteId": "pipette-id",
            "wellName": "A1"
        }
    }
}


POST /runs/:run_id/commands?waitUntilComplete=true
{
    "data": {
        "commandType": "moveToWell",
        "params": {
            "labwareId": "tip-rack-id",
            "pipetteId": "pipette-id",
            "wellName": "A1"
        }
    }
}

[codecov]

Codecov Report

Merging #10902 (03d3f8e) into release_6.0.0 (075179e) will not change coverage.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           release_6.0.0   #10902   +/-   ##
==============================================
  Coverage          73.79%   73.79%           
==============================================
  Files               2076     2076           
  Lines              57331    57331           
  Branches            5722     5722           
==============================================
  Hits               42307    42307           
  Misses             13790    13790           
  Partials            1234     1234           

Flag	Coverage Δ
notify-server	89.17% <ø> (ø)

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

Impacted Files	Coverage Δ
api/src/opentrons/protocol_engine/state/modules.py	91.04% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/geometry.py	100.00% <100.00%> (ø)

[SyntaxColoring]

Code makes sense to me so far.

There's a # TODO on ModuleView.get_overall_height() about it not being tested or used. We should probably update that and ideally also add the tests.

---

The meaning of bareOverallHeight with respect to a Thermocycler is a bit unclear to me. I suppose it could either cover:

1. The distance from the deck to the top of the bottom half of the "clamshell."
2. The distance from the deck to the top of a closed lid.

I think we need to make sure it's (1), not (2). If it's (2), any time the pipette needs to do a GENERAL_ARC at a deck-wide safe height, the computed deck-wide safe-height will be very high. I assume that the reason we need our XY Thermocycler-dodging behavior in the first place is that that height would be too high—beyond the range of pipettes. So if it's (2), when a Thermocycler is loaded, every GENERAL_ARC movement plan would cause a motion planning error.

[mcous]

There's a # TODO on ModuleView.get_overall_height() about it not being tested or used. We should probably update that and ideally also add the tests.

Done and done.

The meaning of bareOverallHeight with respect to a Thermocycler is a bit unclear to me. I suppose it could either cover:

1. The distance from the deck to the top of the bottom half of the "clamshell."
2. The distance from the deck to the top of a closed lid.

I think we need to make sure it's (1), not (2)

I agree. It's value is 98.0 mm, which is less that the height of a magnetic module. Could someone with a TC handy measure it and confirm?

[SyntaxColoring]

I think another way to test this, if you don't feel like issuing HTTP requests manually, is to run through LPC with this protocol:

metadata = {"apiLevel": "2.12"}

def run(protocol):
    tip_rack = protocol.load_labware("opentrons_96_tiprack_300ul", 5)
    plate_1 = protocol.load_labware("biorad_96_wellplate_200ul_pcr", 1)
    plate_2 = protocol.load_labware("biorad_96_wellplate_200ul_pcr", 3)

    # Try commenting out each one, one at a time.
    thermocycler = protocol.load_module("thermocycler")
    # magnetic_module = protocol.load_module("magnetic module gen2", 6)

    pipette = protocol.load_instrument("p300_single_gen2", mount="left", tip_racks=[tip_rack])

    pipette.transfer(300, plate_1.wells_by_name()["A1"], plate_2.wells_by_name()["A12"])

And pay attention to the height of the pipette when it travels between the two plates.

[b-cooper]

There's a # TODO on ModuleView.get_overall_height() about it not being tested or used. We should probably update that and ideally also add the tests.

Done and done.

The meaning of bareOverallHeight with respect to a Thermocycler is a bit unclear to me. I suppose it could either cover:

1. The distance from the deck to the top of the bottom half of the "clamshell."
2. The distance from the deck to the top of a closed lid.

I think we need to make sure it's (1), not (2)

I agree. It's value is 98.0 mm, which is less that the height of a magnetic module. Could someone with a TC handy measure it and confirm?

The answer is 1.

The module definition includes a labwareOffset's z value of 97.8 mm and the bottom of "the clam shell"'s surface is slightly higher than the top of a loaded well plate in the block (98mm). The when the lid itself is closed, the thermocycler's height exceeds the maximum z travel of many pipette+tip combinations. So Max's thought is exactly right, that is why we avoid the closed TC for all move commands regardless of pipette+tip combination out of caution.

[b-cooper]

code and logic make sense to me 🏁

[SyntaxColoring]

Code looks good to me given your on-robot testing (thank you for the videos!) and @b-cooper's explanation above (thank you!).