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, robot-server, shared-data): add FlexStacker module to the hardware controller and robot server. #17187

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Jan 6, 2025

Overview

Adds the Flex Stacker module to the hardware controller and some other gcodes to the driver.

Closes: EXEC-981

Test Plan and Hands on Testing

  • Connect a flex stacker to the flex and make sure the module shows up in the GET /modules endpoint
  • Connect multiple flex stackers to a flex and make sure they both show up
  • Make sure you can move each axis a fixed distance with move_axis command.
  • Make sure you can home each axis, except for L, with the home_axis command.
  • Make sure you can update the flex stacker using the POST /modules/update endpoint

Changelog

  • Add FlexStacker module to the hardware controller
  • Expose FlexStacker module via the robot servers /modules endpoint.
  • add enable_motors to the driver and module (M17)
  • add set_run_current (M906) and set_ihold_current (M907) to driver
  • add get_motion_params (M120) to driver
  • add current to MoveParams to simplify the interface
  • add STACKER_MOTION_CONFIG to keep track of default move parameters used for homing or moving an axis.
  • move_in_mm and move_to_limit_switch now return MoveResult
  • added move_axis and home_axis methods to the FlexStacker module
  • added close_latch and open_latch methods to the FlexStacker module

Risk assessment

Low, unreleased

@vegano1 vegano1 self-assigned this Jan 6, 2025
revert debug changes
ZE: bool
ZR: bool
LR: bool
XE: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

The GCode that we use to get the limit switch statuses always return results for all axes. What are the reasoning behind having setting default values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes it easier to build the obj for testing.

Copy link
Member

Choose a reason for hiding this comment

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

the data constraints should match real life, IMO, and not have defaults. if it's annoying to build for tests, we can make a helper function in the test file that does it.

return PlatformState.UNKNOWN


class StackerAxisState(Enum):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - I was actually planning on this exact logic of this in FW. So we can send a GCode querying the axis state, and it will return one of these strings.

- add set_run_current and set_ihold_current to driver
- add get_motion_params to driver
- add current to MoveParams to simplify interface
- add STACKER_MOTION_CONFIG to keep track of default move parameters used for homing or moving an axis.
- move_in_mm and move_to_limit_switch now return MoveResult
- added move_axis and home_axis methods to the FlexStacker module
- added close_latch and open_latch methods to the FlexStacker module
@vegano1 vegano1 requested a review from ahiuchingau January 8, 2025 22:08
@vegano1 vegano1 marked this pull request as ready for review January 8, 2025 22:09
@vegano1 vegano1 requested review from a team as code owners January 8, 2025 22:09
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.

robot-server and api/src/opentrons/protocol_engine changes LGTM. Thank you!

@vegano1 vegano1 requested review from sfoster1 and a team January 8, 2025 22:56
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 not do those defaults for testing - make a helper function in the test file if needed instead.

@@ -87,3 +105,7 @@ async def set_led(
) -> bool:
"""Set LED color of status bar."""
...

async def enter_programming_mode(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we call this prep_for_update() or something that's more like what other modules use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enter_programming_mode is the lower-level driver function called by the module's higher-level prep_for_update function, which adheres to the rest of the modules.

ZE: bool
ZR: bool
LR: bool
XE: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

the data constraints should match real life, IMO, and not have defaults. if it's annoying to build for tests, we can make a helper function in the test file that does it.

- added HopperDoorState and LatchState to FlexStacker module
- replaced hopperDoorClosed with hopperDoorState in live data
- replaced axisStateL with LatchState in live data
- use move_axis instead of home_axis for open_latch and close_latch
- set find_dfu_device expected_device_count to 3
- cleanup
max_speed: float | None = None
acceleration: float | None = None
max_speed_discont: float | None = None
axis: Optional[StackerAxis] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

When wouldn't you specify an Axis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably never, ill change it in a follow-up pr I have.

Copy link
Contributor

@caila-marashaj caila-marashaj left a comment

Choose a reason for hiding this comment

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

Nice! I think this is gonna make the module code a little easier to structure going forward

@vegano1 vegano1 merged commit a3f541d into edge Jan 9, 2025
66 of 70 checks passed
@vegano1 vegano1 deleted the flex-stacker-add-module-skeleton branch January 9, 2025 19:22
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.

5 participants