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

API: adds support for pipette versions v1.3 #1474

Merged
merged 43 commits into from
Jun 4, 2018
Merged

API: adds support for pipette versions v1.3 #1474

merged 43 commits into from
Jun 4, 2018

Conversation

andySigler
Copy link
Contributor

@andySigler andySigler commented May 17, 2018

overview

This PR adds API support for pipettes marked with a version number "v1.3". This new version number is needed for pipettes that are assembled with an updated homing PCB which shifts all the plunger positions by 1.5mm.

Changes include:

  1. Additions to the pipette-config fallback and shared-data for the new models
  2. Each time a pipette is created from InstrumentWrapper, the pipette model version is derived from what pipette version is currently attached to that mount on the bot.
    • When simulating, the version defaults to v1
    • If the attached pipette is an entirely different model, then it falls back to the protocol-specified model
    • here's the line where that happens
  3. Addition to the tools.write_pipette_memory script to write new version strings

@andySigler andySigler added api Affects the `api` project ready for review labels May 17, 2018
@andySigler andySigler changed the title API: adds support for pipette versions V13 API: adds support for pipette versions v1.3 May 31, 2018
@andySigler
Copy link
Contributor Author

changed model string to "p50_single_v1.3" and so on for other models

@andySigler andySigler requested a review from sanni-t May 31, 2018 14:37
@@ -14,6 +14,7 @@
class PipetteTest(unittest.TestCase):
def setUp(self):
self.robot = Robot()
self.robot.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this does anything, but doesn't specifically need to be removed either

@@ -74,17 +76,18 @@ def test_deprecated_axis_call(self):
warnings.filterwarnings('error')
# Check that user warning occurs when axis is called
self.assertRaises(
UserWarning, Pipette, self.robot, axis='a')
RuntimeError, Pipette, self.robot, mount='left')
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't keep the intended effect of this test--the original purpose was to make sure that a warning would be raised if a protocol uses a deprecated parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -133,8 +136,6 @@ def test_volume_percentage(self):
self.assertRaises(RuntimeError, self.p200._volume_percentage, 300)
self.assertEquals(self.p200._volume_percentage(100), 0.5)
self.assertEquals(len(self.robot.get_warnings()), 0)
self.p200._volume_percentage(self.p200.min_volume / 2)
self.assertEquals(len(self.robot.get_warnings()), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we purposefully dropping this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@btmorr
Copy link
Contributor

btmorr commented Jun 4, 2018

🦅

@andySigler andySigler merged commit 9b5166d into edge Jun 4, 2018
@andySigler andySigler deleted the api-pipette-v2 branch June 4, 2018 17:25
@mcous
Copy link
Contributor

mcous commented Jun 5, 2018

Merging this PR broke pipette swap in the 3.1.1 app release and should not have been merged until app support was added. Key takeaways:

  1. As you can see in my above comment, I definitely didn't make enough noise nor was I at all explicit enough that this PR should've been blocked by app work
    1. Adding a "DO NOT MERGE" label to the repo to be used in these sorts of situations
    2. In the future, a change request should be made rather than posting a single comment to alert the author of these issues
  2. shared-data was changed but no front-end reviewers were requested
    1. Adding a shared-data label to the repo for tagging these PRs
    2. PRs that touch shared-data must include PR review and testing
    3. PRs that touch shared-data must include App review and testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants