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

feat(thermocycler): check closed lid sensor when a move finishes #449

Merged
merged 14 commits into from
Mar 19, 2024
Merged

feat(thermocycler): check closed lid sensor when a move finishes #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8cafab1
throw an error if a move finishes without triggering a sensor
caila-marashaj Feb 27, 2024
f004401
format
caila-marashaj Feb 28, 2024
5b838f3
use error code instead of new error msg
caila-marashaj Mar 4, 2024
320daba
send error message if unexpected lid state
caila-marashaj Mar 8, 2024
a1086d8
edit tests
caila-marashaj Mar 8, 2024
8728918
typo
caila-marashaj Mar 8, 2024
73ca051
update gcc12 compiliation errors
ryanthecoder Mar 18, 2024
a479c63
move test to close lid
ryanthecoder Mar 18, 2024
a8634b6
typo
ryanthecoder Mar 18, 2024
b8406d8
fix unknown lid state tests
ryanthecoder Mar 18, 2024
935ff85
fix remaining tests
ryanthecoder Mar 18, 2024
bda60fa
tests passing + rm outside calls to test
caila-marashaj Mar 18, 2024
a4d1de1
rm tmc2130 h change and include utility in pc
caila-marashaj Mar 18, 2024
e160e8c
format
caila-marashaj Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion stm32-modules/include/common/core/xt1511.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ concept XT1511Policy = requires(Policy& p, BufferT& b) {
// Function to get the max PWM configured for the timer. Should
// be in the same byte format as the PWM buffer type passed in.
{ p.get_max_pwm() } -> std::same_as<typename BufferT::value_type>;
std::unsigned_integral<typename BufferT::value_type>;
requires std::unsigned_integral<typename BufferT::value_type>;
};

// This class represents a single XT1511
Expand Down
1 change: 1 addition & 0 deletions stm32-modules/include/thermocycler-gen2/thermocycler-gen2/errors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ enum class ErrorCode {
SEAL_MOTOR_STALL = 506,
LID_CLOSED = 507,
SEAL_MOTOR_SWITCH = 508,
UNEXPECTED_LID_STATE = 509,
};

auto errorstring(ErrorCode code) -> const char*;
Expand Down
15 changes: 9 additions & 6 deletions stm32-modules/include/thermocycler-gen2/thermocycler-gen2/motor_task.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace motor_task {
/*
* The MotorExecutionPolicy is how the portable task interacts
* with the hardware. It is defined as a concept so it can be
* passed as a reference paramter to run_once(), which means the
* passed as a reference parameter to run_once(), which means the
* type of policy in actual use does not have to be part of the class's
* type signature (which is used all over the place), just run_once's
* type signature, which is used just by the rtos task and the test
Expand Down Expand Up @@ -370,9 +370,10 @@ class MotorTask {
static_cast<void>(msg); // No contents in message
LidStepperState::Status old_state = _lid_stepper_state.status.load();
auto error = handle_hinge_state_end(policy);
if (_lid_stepper_state.status == LidStepperState::Status::IDLE &&
old_state != _lid_stepper_state.status &&
_lid_stepper_state.response_id != INVALID_ID) {
if ((_lid_stepper_state.status == LidStepperState::Status::IDLE &&
old_state != _lid_stepper_state.status &&
_lid_stepper_state.response_id != INVALID_ID) ||
error != errors::ErrorCode::NO_ERROR) {
// Send an ACK if a movement just finished
auto response = messages::AcknowledgePrevious{
.responding_to_id = _lid_stepper_state.response_id,
Expand Down Expand Up @@ -1289,8 +1290,10 @@ class MotorTask {
motor_util::LidStepper::Position::CLOSED;
// The overall lid state machine can advance now
error = handle_lid_state_end(policy);
// TODO(Frank, Mar-7-2022) check if the lid didn't make it in
// all the way
// if the lid isn't actually closed, overwrite error status
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do this before the handle_lid_state_end call above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it would be easier to overwrite the error value after that function is done, since inside of its scope it doesn't know the desired status of the lid, which in this case is OVERDRIVE

if (!policy.lid_read_closed_switch()) {
error = errors::ErrorCode::UNEXPECTED_LID_STATE;
}
break;
case LidStepperState::Status::LIFT_NUDGE:
policy.lid_stepper_start(
Expand Down
3 changes: 3 additions & 0 deletions stm32-modules/include/thermocycler-gen2/thermocycler-gen2/plate_control.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
*/
#pragma once

#include <array>
#include <optional>

#include "core/pid.hpp"
#include "thermocycler-gen2/thermal_general.hpp"

Expand Down
13 changes: 7 additions & 6 deletions stm32-modules/include/thermocycler-gen2/thermocycler-gen2/tmc2130_registers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,14 @@ enum class Registers : uint8_t {

/** Template concept to constrain what structures encapsulate registers.*/
template <typename Reg>
concept TMC2130Register = requires(Reg& r, uint64_t value) {
// Struct has a valid register address
std::same_as<decltype(Reg::address), Registers&>;
// Struct has an integer with the total number of bits in a register.
// This is used to mask the 64-bit value before writing to the IC.
concept TMC2130Register =
std::same_as<std::remove_cvref_t<decltype(Reg::address)>,
std::remove_cvref_t<Registers&>> &&
std::integral<decltype(Reg::value_mask)>;
};

// Struct has a valid register address
// Struct has an integer with the total number of bits in a register.
// This is used to mask the 64-bit value before writing to the IC.

template <typename Reg>
concept WritableRegister = requires() {
Expand Down
3 changes: 3 additions & 0 deletions stm32-modules/thermocycler-gen2/src/errors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ const char* const SEAL_MOTOR_SWITCH =
"ERR508:seal:Seal switch should not be engaged OK\n";

const char* const UNKNOWN_ERROR = "ERR-1:unknown error code OK\n";
const char* const UNEXPECTED_LID_STATE =
"ERR509:lid:Lid status does not match expected open/closed status";

// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
#define HANDLE_CASE(errname) \
Expand Down Expand Up @@ -144,6 +146,7 @@ auto errors::errorstring(ErrorCode code) -> const char* {
HANDLE_CASE(SEAL_MOTOR_STALL);
HANDLE_CASE(LID_CLOSED);
HANDLE_CASE(SEAL_MOTOR_SWITCH);
HANDLE_CASE(UNEXPECTED_LID_STATE);
}
return UNKNOWN_ERROR;
}
2 changes: 2 additions & 0 deletions stm32-modules/thermocycler-gen2/src/plate_control.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

#include "thermocycler-gen2/plate_control.hpp"

#include <utility>

#include "thermocycler-gen2/thermal_general.hpp"

using namespace plate_control;
Expand Down
6 changes: 3 additions & 3 deletions stm32-modules/thermocycler-gen2/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ add_executable(${TARGET_MODULE_NAME}
test_m116.cpp
test_m117.cpp
test_m119.cpp
test_m126.cpp
test_m126.cpp
test_m127.cpp
test_m128.cpp
test_m140.cpp
Expand All @@ -52,8 +52,8 @@ add_executable(${TARGET_MODULE_NAME}
test_m241d.cpp
test_m242d.cpp
test_m243d.cpp
test_m900d.cpp
test_m901d.cpp
test_m900d.cpp
test_m901d.cpp
test_m902d.cpp
test_m903d.cpp
test_m904d.cpp
Expand Down
218 changes: 159 additions & 59 deletions stm32-modules/thermocycler-gen2/tests/test_motor_task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,18 +904,37 @@ SCENARIO("motor task lid state machine") {
.seal_on = true,
.seal_direction = true,
.seal_switch_armed = false},
// Should send ACK now
{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack =
messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}},
};
test_motor_state_machine(tasks, steps);
AND_WHEN("the closed switch is triggered") {
motor_policy.set_lid_closed_switch(true);
steps.push_back(
// an ack with error code NO_ERROR should follow
MotorStep{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}});
test_motor_state_machine(tasks, steps);
}
AND_WHEN("the closed switch is not triggered") {
motor_policy.set_lid_closed_switch(false);
steps.push_back(
// an ack with error code UNEXPECTED_LID_STATE should follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 0,
.with_error =
errors::ErrorCode::UNEXPECTED_LID_STATE}});
test_motor_state_machine(tasks, steps);
}
}
WHEN("sending plate lift command") {
std::vector<MotorStep> steps;
Expand Down Expand Up @@ -973,6 +992,7 @@ SCENARIO("motor task lid state machine") {
.lid_rpm =
motor_task::LidStepperState::LID_DEFAULT_VELOCITY_RPM});
steps.push_back(
// an ack with error code NO_ERROR should follow
MotorStep{.msg = messages::LidStepperComplete(),
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
Expand Down Expand Up @@ -1009,18 +1029,39 @@ SCENARIO("motor task lid state machine") {
.seal_on = true,
.seal_direction = true,
.seal_switch_armed = false},
// Should send ACK now
{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack =
messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}},
};
test_motor_state_machine(tasks, steps);
AND_WHEN("the closed switch is triggered") {
motor_policy.set_lid_closed_switch(true);
steps.push_back(
// an ack with error code NO_ERROR should follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}});
test_motor_state_machine(tasks, steps);
}
AND_WHEN("the closed switch is not triggered") {
motor_policy.set_lid_closed_switch(false);
steps.push_back(
// an ack with error code UNEXPECTED_LID_STATE should
// follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 0,
.with_error =
errors::ErrorCode::UNEXPECTED_LID_STATE}});
test_motor_state_machine(tasks, steps);
}
}
}
GIVEN("seal switches aren't shared") {
Expand Down Expand Up @@ -1052,18 +1093,39 @@ SCENARIO("motor task lid state machine") {
.seal_on = true,
.seal_direction = false,
.seal_switch_armed = true},
// Should send ACK now
{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack =
messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}},
};
test_motor_state_machine(tasks, steps);
AND_WHEN("the closed switch is triggered") {
motor_policy.set_lid_closed_switch(true);
steps.push_back(
// an ack with error code NO_ERROR should follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}});
test_motor_state_machine(tasks, steps);
}
AND_WHEN("the closed switch is not triggered") {
motor_policy.set_lid_closed_switch(false);
steps.push_back(
// an ack with error code UNEXPECTED_LID_STATE should
// follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.motor_state = MotorStep::MotorState::IDLE,
.ack = messages::AcknowledgePrevious{
.responding_to_id = 0,
.with_error =
errors::ErrorCode::UNEXPECTED_LID_STATE}});
test_motor_state_machine(tasks, steps);
}
}
}
}
Expand Down Expand Up @@ -1130,35 +1192,73 @@ SCENARIO("motor task lid state machine") {
.lid_angle_decreased = true,
.lid_overdrive = false,
.lid_rpm =
motor_task::LidStepperState::LID_DEFAULT_VELOCITY_RPM},
motor_task::LidStepperState::LID_DEFAULT_VELOCITY_RPM}};
test_motor_state_machine(tasks, steps);
steps.clear();
AND_WHEN("the closed switch is triggered") {
motor_policy.set_lid_closed_switch(true);
// Fourth step overdrives hinge
{.msg = messages::LidStepperComplete(),
.lid_angle_decreased = true,
.lid_overdrive = true},
steps.push_back(MotorStep{.msg = messages::LidStepperComplete(),
.lid_angle_decreased = true,
.lid_overdrive = true});
// Now extend seal to switch
{.msg = messages::LidStepperComplete(),
.seal_on = true,
.seal_direction = false,
.seal_switch_armed = true},
steps.push_back(MotorStep{.msg = messages::LidStepperComplete(),
.seal_on = true,
.seal_direction = false,
.seal_switch_armed = true});
// Retract seal from switch
{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::LIMIT},
.seal_on = true,
.seal_direction = true,
.seal_switch_armed = false},
// Should send ACK now
{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.ack =
messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}},
};
test_motor_state_machine(tasks, steps);
steps.push_back(
MotorStep{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::LIMIT},
.seal_on = true,
.seal_direction = true,
.seal_switch_armed = false});
steps.push_back(
// an ack with error code NO_ERROR should follow
MotorStep{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.ack = messages::AcknowledgePrevious{
.responding_to_id = 123,
.with_error = errors::ErrorCode::NO_ERROR}});
test_motor_state_machine(tasks, steps);
}
AND_WHEN("the closed switch is not triggered") {
motor_policy.set_lid_closed_switch(false);
// Fourth step overdrives hinge
steps.push_back(MotorStep{.msg = messages::LidStepperComplete(),
.lid_angle_decreased = true,
.lid_overdrive = true});
// Now extend seal to switch
steps.push_back(MotorStep{.msg = messages::LidStepperComplete(),
.seal_on = true,
.seal_direction = false,
.seal_switch_armed = true});
// Retract seal from switch
steps.push_back(
MotorStep{.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::LIMIT},
.seal_on = true,
.seal_direction = true,
.seal_switch_armed = false});
steps.push_back(
// an ack with error code UNEXPECTED_LID_STATE should follow
MotorStep{
.msg =
messages::SealStepperComplete{
.reason = messages::SealStepperComplete::
CompletionReason::DONE},
.ack = messages::AcknowledgePrevious{
.responding_to_id = 0,
.with_error =
errors::ErrorCode::UNEXPECTED_LID_STATE}});
test_motor_state_machine(tasks, steps);
}
}
WHEN("sending plate lift command") {
std::vector<MotorStep> steps = {
Expand Down
Loading