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(pipettes): sync moves with pressure sensor reading #749

Merged
merged 27 commits into from
Mar 7, 2024

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Jan 29, 2024

Overview

This branch allows us to start and stop the pressure sensor in sync with the execution of a pipette move group.

It seems pretty involved for what it does currently, but I did it this way because I think it will make it easier in the near future to build code for collecting and packaging large amounts of sensor data while the move is happening.

It also is pretty messy with #ifdef directives at the moment, but it has potential to be used for any sensor and could be nice to have for testing in the future, so any suggestions about making that a bit cleaner are much appreciated.

To compile with this feature built in use cmake --preset cross -DUSE_PRESSURE_MOVE and this will set the compiler defs required to enable this feature

Changelog

  • creates a derived PipetteMotorInterruptHandler class, which should allow us to send BindSensorOutputRequest messages directly to the pressure sensor task
  • adds AddSensorMoveRequest, which is almost the same as an AddLinearMoveRequest, but includes a sensor id (we can also add a parameter for SensorType if we want to generalize this further)
  • have this feature be triggered by the receipt of a move group comprised of AddSensorMoveRequest messages

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.28%. Comparing base (f775c63) to head (6bd9fc7).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #749      +/-   ##
==========================================
- Coverage   83.58%   83.28%   -0.30%     
==========================================
  Files         101      101              
  Lines        4044     4063      +19     
==========================================
+ Hits         3380     3384       +4     
- Misses        664      679      +15     

see 8 files with indirect coverage changes

@ryanthecoder ryanthecoder force-pushed the sync-pressure-sensor-reading-w-move-group branch from ca9b08b to 5a833f1 Compare March 4, 2024 21:51
@ryanthecoder ryanthecoder force-pushed the sync-pressure-sensor-reading-w-move-group branch from 5a833f1 to 0073d39 Compare March 6, 2024 20:04
@ryanthecoder ryanthecoder force-pushed the sync-pressure-sensor-reading-w-move-group branch from 7085b36 to 8bb65f8 Compare March 7, 2024 19:11
@ryanthecoder ryanthecoder marked this pull request as ready for review March 7, 2024 19:16
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.

Let's get a Ci task that builds in the new configuration too to make sure nobody accidentally breaks it, and maybe see if we can add a bit in the device info?

@ryanthecoder ryanthecoder merged commit 750940a into main Mar 7, 2024
19 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.

5 participants