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(shared-data): add deck schema v6 #17250

Draft
wants to merge 8 commits into
base: edge
Choose a base branch
from

Conversation

sfoster1
Copy link
Member

This schema contains a concept of "locating features" for addressable areas. These locating features are things that constrain the position of labware loaded on top of the addressable area. The presence of a locating feature indicates that the addressable area can contain deck items. Which locating feature is available constrains which labware can be loaded; different labware define different locatable features (for instance, their back left or front left corners, or the outer surface of one of their wells).

Locating feature names are not constrained to enums because we expect a fairly wide number of them.

To come out of draft

  • positions for modules that are not currently in

risk

low, because nothing should be using this (purposeful, will be followup prs)

Closes EXEC-80

This schema contains a concept of "locating features" for addressable
areas. These locating features are things that constrain the position of
labware loaded on top of the addressable area. The presence of a
locating feature indicates that the addressable area can contain deck
items. Which locating feature is available constrains which labware can
be loaded; different labware define different locatable features (for
instance, their back left or front left corners, or the outer surface of
one of their wells).

Locating feature names are not constrained to enums because we expect a
fairly wide number of them.

Closes EXEC-80
@sfoster1 sfoster1 requested review from a team as code owners January 10, 2025 20:20
@sfoster1 sfoster1 requested review from ncdiehl11 and removed request for a team January 10, 2025 20:20
@sfoster1 sfoster1 marked this pull request as draft January 10, 2025 20:20
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Looks great so far. Very excited for this.

Comment on lines 41 to 54
"locatingFeature": {
"type": "object",
"required": ["locatingFeatureId", "offsetVector"],
"properties": {
"locatingFeatureId": {
"type": "string",
"description": "The name of a kind of locating feature that will be looked up in labware. Common ones are backLeft, frontLeft, wellA1, center, but others may be used."
},
"offsetVector": {
"$ref": "#/definitions/xyzArray",
"description": "A vector from the origin of the thing that has the locating feature (i.e. an addressable area) to the key point of the locating feature."
}
}
}
Copy link
Contributor

@SyntaxColoring SyntaxColoring Jan 13, 2025

Choose a reason for hiding this comment

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

Locating feature names are not constrained to enums because we expect a fairly wide number of them.

I agree with not constraining them to enums, for versioning churn reasons, but I do think we'll need some central documented source that lists them all out and describes their semantics.

Like, what if we did something roughly like this:

"anyOf": [
  { "const": "backLeft": "description": "..."},
  { "const": "center": "description": "..."},
  { "type": "string", "description": "reserved for future expansion" }
]

Alternatively, I guess this documentation could live in the labware definition schema. I don't have a preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh definitely! I was trying to remember how to do this and couldn't haha. I'll add that.

Comment on lines +15 to +16
const defV5Glob = path.join(__dirname, '../../deck/definitions/5/*.json*')
const defV6Glob = path.join(__dirname, '../../deck/definitions/6/*.json*')
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the trailing *s for?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's definitely a typo that got copy pasted

Comment on lines 203 to 210
{
"locatingFeatureId": "frontLeft",
"offsetVector": [0.0, 86.0, 0.0]
},
{
"locatingFeatureId": "backLeft",
"offsetVector": [0.0, 0.0, 0.0]
}
Copy link
Contributor

@SyntaxColoring SyntaxColoring Jan 13, 2025

Choose a reason for hiding this comment

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

Either:

  • The slot origin is the front left, which means frontLeft should have an offset of [0, 0, 0] and backLeft should have an offset of [0, 86, 0]
  • Or the slot origin is the back left, which means frontLeft should have an offset of [0, -86, 0] and backLeft should have an offset of [0, 0, 0]

Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

...yes. I will fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing I'm going to do is keep the slot origin as the front left. This is fine because actually what we're saying is that the AA origin is the point indicated by the offsetFromCutoutFixture vector, and all coordinates are relative to that. Conveniently, keeping that vector the same means that we can avoid changing some app code.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.33%. Comparing base (9a28d32) to head (340e366).
Report is 46 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17250      +/-   ##
==========================================
+ Coverage   73.84%   75.33%   +1.49%     
==========================================
  Files          43       30      -13     
  Lines        3303     2818     -485     
==========================================
- Hits         2439     2123     -316     
+ Misses        864      695     -169     
Flag Coverage Δ
shared-data 75.33% <100.00%> (+1.49%) ⬆️

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

Files with missing lines Coverage Δ
...ed-data/python/opentrons_shared_data/deck/types.py 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

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