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): Add Gcodes to get histogram measurements from the TOF sensors on the Flex Stacker. #17578

Open
wants to merge 1 commit into
base: edge
Choose a base branch
from

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Feb 24, 2025

Overview

We can finally take histogram readings once we load the measurement application, configure the custom SPAD map, load the factory calibration data, and load short-range calibration data. Each histogram reading is 3840 bytes in total; we need to be able to send that data over serial whenever asked, right now, we won't be processing this data internally in the Flex Stacker firmware since we want to collect data to create a baseline. So, we will need a mechanism that allows the Python host to request a histogram reading and receive packets until the entire reading has been transferred. This pull request goes with this Opentrons/opentrons-modules#505 pr.

Closes: EXEC-1181

Test Plan and Hands on Testing

  • Make sure we can start/stop TOF measurements with GCODE 225
  • Make sure we can receive TOF histogram frames with GCODE 226
  • Make sure the get_tof_histogram function requests/parses 30 histogram frames and processes them to produce channel measurements from 0-9.

Changelog

  • Added M225 - Start/Cancel TOF Sensor Measurements Gcode
  • Added M226 - Get TOF Histogram measurement Gcode
  • Added get_tof_function that starts and requests TOF histogram measurements from the Flex Stacker.

Review requests

Any feedback would be great, especially the get_tof_histogram function.

Risk assessment

low, unreleased

@vegano1 vegano1 requested a review from a team as a code owner February 24, 2025 16:11
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.

From an outsider's perspective, this all makes sense and is pretty easy to follow. Thanks!

Copy link
Contributor

@ahiuchingau ahiuchingau left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me!

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good but let's call the message something else since it does more than start, and we should move the parse code into a place where the system can cancel the measurement if it's ongoing.

What happens if a client starts a measurement then disconnects and reconnects? Can we tolerate an ongoing measurement or detect and cancel it? Will sending two starts in a row just restart the measurement?

Also, please add tests.

"""Start or Cancel Measurements from the TOF sensor."""
...

async def get_tof_measurement(
Copy link
Member

Choose a reason for hiding this comment

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

is a caller above the level of the module ever going to want to get just one frame of the measurement instead of the full histogram? if not, can we lose this from the abstract base class?

Copy link
Contributor Author

@vegano1 vegano1 Feb 24, 2025

Choose a reason for hiding this comment

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

Yeah, the sensor can support other types of measurements which I didn't implement here.
get_tof_measurement would be the interface for getting other types of measurements as well.

Copy link
Member

Choose a reason for hiding this comment

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

oh, then can we call it something else?

@vegano1
Copy link
Contributor Author

vegano1 commented Feb 24, 2025

Looks good but let's call the message something else since it does more than start, and we should move the parse code into a place where the system can cancel the measurement if it's ongoing.

What happens if a client starts a measurement then disconnects and reconnects? Can we tolerate an ongoing measurement or detect and cancel it? Will sending two starts in a row just restart the measurement?

Also, please add tests.

We can detect and cancel measurements, when there's a measurement already in progress we will receive an invalid response, we can capture that and send cancel.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

enable_tof_measurement() kind of has the same problem, right? enable_tof_measurement(..., enable=False) doesn't really feel good. Sorry to keep harping on this but it's a real pet peeve. Let's call it manage_tof_measurement or start_stop_tof_measurment or something.

@vegano1 vegano1 force-pushed the flex-stacker-add-tof-sensor-measurements branch from 110be9f to 25c849e Compare February 24, 2025 23:54
@vegano1 vegano1 requested a review from sfoster1 February 24, 2025 23:55
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.

4 participants