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): send most recent sensor data after probe #784

Merged
merged 15 commits into from
Jun 14, 2024

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Jun 12, 2024

Overview

After a liquid probe, we want to send the 500 most recent pressure data readings in a buffer over CAN to determine on the Python side if we've successfully hit liquid, hit the bottom of a piece of labware, or not found liquid at all.

This pr creates a circular buffer of pressure sensor data, meaning that after every i2c transaction, we'll write the pressure reading to the buffer. When we reach the end of the buffer, we'll circle back to the front of the buffer and continue overwriting the oldest data until the probing is finished. Then, after receiving a SendAccumulatedSensorDataRequest, we send the buffer in order starting from the oldest data.

Changelog

  • create a sensor buffer regardless of compile target
  • change p_buff in all sensor firmware to sensor_buffer, since the capacitive sensor now uses this too
  • change the buffer size for all pipettes to 500
  • create a new function in pressure_driver to manage the buffer
  • change the way we send the data buffer to accommodate the new shape of the data

Test Plan

for single, multi, and 96 channel pipettes:

  • run liquid probe and make sure that still works the same
  • afterward, using can_control, send a SendAccumulatedSensorDataRequest
  • make sure 500 read_sensor_response messages are sent over CAN
  • make sure that the readings in those messages align with the sensor responses sent over CAN during the probe

Todo

I plan on adding a test for this in a follow-up pr

@caila-marashaj caila-marashaj marked this pull request as ready for review June 12, 2024 19:19
@@ -323,6 +330,7 @@ class MMR920 {
_registers.pressure_result.reading, sensor_version);

if (max_pressure_sync) {
sensor_buffer_log(pressure);
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 go inside this if block, it should be the if(echo_this_time) block otherwise we are double logging the data
and we can remove the ifdef from the if(echo_this_time) block

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 once tested, though I'm curious about the use of (*buff).at() - why not either buff->at() or (*buff)[]? the latter is probably faster since it doesn't range check; this would normally be bad but we can't handle exceptions anyway.

@caila-marashaj caila-marashaj force-pushed the EXEC-516-update-sensor-buffer branch from 9730d3b to 2ca6b66 Compare June 13, 2024 15:18
@caila-marashaj caila-marashaj force-pushed the EXEC-516-update-sensor-buffer branch from b1e745c to e6378ce Compare June 14, 2024 14:31
@caila-marashaj caila-marashaj merged commit 1788c1f into main Jun 14, 2024
38 of 39 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.

3 participants