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(hepa-uv): Add CAN messages to control the Hepa/UV filter. #753

Merged
merged 7 commits into from
Feb 8, 2024

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Feb 8, 2024

Overview

We need to control the Hepa/UV filter from the Flex for testing and eventual system integration, this PR implements the CAN messages required to make this happen. It also adds a new method to query the time remaining before a software timer expires. This PR coincides with Opentrons/opentrons#14452 PR.

Closes: RET-1422 RET-1423

Test Plan

  • Make sure the Hepa fan still turns on/off with the push button
  • Make sure the UV Ballast still turns on/off with the push button when the Flex door is closed and reed switch is set.
  • Make sure that we can set/get the Hepa fan state and duty cycle with the SetHepaFanStateRequest and GetHepaFanStateRequest messages.
  • Make sure that we can set/get the UV light state and timeout with the SetHepaUVStateRequest and GetHepaUVStateRequest messages.
  • Make sure that the UV light ONLY turns on via CAN message when the door is closed and reed switch is set.
  • Make sure that the UV light turns OFF when the door is opened
  • After turning ON the UV light via a CAN message, make sure we can turn off the light when the push button is pressed.

Change Log

  • Added implementation for CAN messages to set/get Hepa fan state/duty cycle
  • Added implementation for CAN messages to set/get UV Light state/timeout in seconds
  • Added a get_remaining_time method to the FreeRTOSTimer class to know the time in ms before the timer expires.
  • Integrated the new CAN messages into their corresponding Hepa Task and UV Task
  • Refactored the Hepa Task logic so it takes into account the CAN messages so we aren't duplicating code.
  • Refactored the UV Task logic so it takes into account the CAN messages so we aren't duplicating code.

Review Requests

  • Does anything not make sense with the can messages?

Risk Assessment

Low, unreleased

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 change the name of that macro. Also, I think this functionality is getting to the point where it should really have tests.

cpp-utils/include/ot_utils/freertos/freertos_timer.hpp Outdated Show resolved Hide resolved
@vegano1
Copy link
Contributor Author

vegano1 commented Feb 8, 2024

Looks good, but let's change the name of that macro. Also, I think this functionality is getting to the point where it should really have tests.

agreed, I will add a test suite in up coming PR.

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.

Awesome!

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1faf654) 83.58% compared to head (2d53da1) 83.58%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #753   +/-   ##
=======================================
  Coverage   83.58%   83.58%           
=======================================
  Files         101      101           
  Lines        4044     4044           
=======================================
  Hits         3380     3380           
  Misses        664      664           

@vegano1 vegano1 merged commit 65d6a31 into main Feb 8, 2024
31 of 32 checks passed
@vegano1 vegano1 deleted the RET-1422_add_hepa_control_can_msg branch February 8, 2024 22:18
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