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

feat(api): wire up mmToEdge to liquid class transfer #17293

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

jbleon95
Copy link
Contributor

@jbleon95 jbleon95 commented Jan 16, 2025

Overview

Closes AUTH-1292.

This PR wires up the liquid class touch tip property mmToEdge to the touch tip execution logic. This follows up #17107 which added the argument to the touchTip protocol engine. In order to access this through the API, the argument was added as an optional argument with a default to the core methods. For legacy cores it will raise an APIVersionError if supplied with anything other than None (the default). For the engine core, it will pass the arguments to the engine.

Since this is only added to the core, the user facing InstrumentContext touch tip still uses radius as the only way to change where the touch tip moves go to in the well. The only place that uses this new argument is in the logic for transfer_liquid, which uses the core methods.

Test Plan and Hands on Testing

Unit test coverage and testing in previous PR confirmed that the execution logic worked.

  • Test simple liquid transfer on robot to confirm that a touch tip moves to the correct spot (the value will most likely need to be changed to be dramatic in order to see it)

Changelog

  • added mm_from_edge argument to API core touch_tip methods
  • wired up liquid class property mmToEdge to touch tip calls.

Review requests

There is now a divergence between what the API and engine code calls this argument (mm_from_edge) and what the transfer property still has it as mmToEdge. I prefer using from since it's more natural english in describing the property. I have not changed anything about the liquid class names yet, but if this should be done in this PR I can do that.

There's also the fact that supplying a radius of a value other than 1.0 and a mmFromEdge argument of not None or 0 will cause the engine touch tip implementation to raise. No additional check has been put in the core, though the docstring contains information that this is mutually exclusive. Should we have an additional check or is the engine one enough?

Risk assessment

Low.

@jbleon95 jbleon95 requested a review from sanni-t January 16, 2025 19:40
@jbleon95 jbleon95 requested a review from a team as a code owner January 16, 2025 19:40
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

This LGTM. Just that one soft recommendation about touch_tip args.
Tested on a Flex and changing the liquid class property correctly changed the touch tip distance.

Comment on lines 391 to +394
radius: float,
z_offset: float,
speed: float,
mm_from_edge: Optional[float] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. I think I prefer making both radius and mm_from_edge optional and asserting that only one of them is used.
If not that, then I think we should at least assert that radius is 1 when mm_from_edge is specified. This will make it easier for unit tests to catch wrong usage of these arguments.

If this was a public API then we would definitely want to have both optional and raising an appropriate error if they are used incorrectly.

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

@jbleon95 jbleon95 merged commit c006e7c into edge Jan 17, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants