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.

Sign up for GitHub

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

Jump to bottom

modify tip presence sensor task #713

Merged
merged 12 commits into from
Aug 14, 2023
Merged

modify tip presence sensor task #713

merged 12 commits into from
Aug 14, 2023

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Aug 9, 2023 •
edited
Loading

Overview

This code enables both tip presence sensors to be read on the 8 and 96 channel pipettes, making each sensor individually addressable via CAN message.

Changelog

  • replace single tip notification task with two tasks: front and rear
  • add sensor id parameter to tip notification task and related CAN messages
  • switch pin configurations for front and rear sensors to match MCU schematic

@caila-marashaj caila-marashaj marked this pull request as ready for review August 10, 2023 16:04
sfoster1
sfoster1 requested changes Aug 10, 2023
View reviewed changes
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.

Couple small change requests but looks good!

include/sensors/core/message_handlers/sensors.hpp Outdated Show resolved Hide resolved
include/sensors/core/tasks/tip_presence_notification_task.hpp Outdated Show resolved Hide resolved
@caila-marashaj caila-marashaj mentioned this pull request Aug 10, 2023
Merged
Laura-Danielle
Laura-Danielle approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

Looks really good, nice work Caila! Just make sure to have @sfoster1 do a quick glance afterwards.

sfoster1
sfoster1 requested changes Aug 11, 2023
View reviewed changes
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.

Love the structure, needs the null checks though.

include/can/core/messages.hpp Outdated Show resolved Hide resolved
include/sensors/core/tasks/tip_presence_notification_task.hpp Outdated Show resolved Hide resolved
pipettes/core/sensor_tasks.cpp Outdated Show resolved Hide resolved
pipettes/core/sensor_tasks.cpp Outdated Show resolved Hide resolved
pipettes/firmware/main.cpp Outdated Show resolved Hide resolved
pipettes/firmware/main.cpp Outdated Show resolved Hide resolved
sfoster1
sfoster1 approved these changes Aug 14, 2023
View reviewed changes
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 to me!

@caila-marashaj caila-marashaj merged commit 1b280e9 into main Aug 14, 2023
22 of 24 checks passed
@caila-marashaj caila-marashaj deleted the separate-tip-presence-tasks-front-and-rear branch August 14, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfoster1 sfoster1 sfoster1 approved these changes

@Laura-Danielle Laura-Danielle Laura-Danielle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@caila-marashaj @sfoster1 @Laura-Danielle