Skip to content

Commit

Permalink
refactor(head,common): commonize tool detect
Browse files Browse the repository at this point in the history
The tool detection will also be needed on the pipette. In addition to
therefore needing the tool detection code to live in common/ somewhere,
the pipettes also need to interact with the code in a different way -
they don't care what kind of tool they are (or maybe do only as a
debug), just what carrier they're on. In the same way, the head doesn't
really care what carrier it reads, since it knows which pins it's
looking at, just which tool it is. So we can provide separate filtering
options for each and let each use of the code become simpler.

Unfortunately ranges and views just don't work in clang at all because
of llvm/llvm-project#44178 so even though it
all compiles correctly we have to do a pretty ugly hack that removes
functionality when linting. It would be nice to not have this.
  • Loading branch information
sfoster1 committed Feb 15, 2022
1 parent 9710206 commit 31de101
Show file tree
Hide file tree
Showing 17 changed files with 458 additions and 196 deletions.
1 change: 1 addition & 0 deletions common/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
add_subdirectory(core)
if (${CMAKE_CROSSCOMPILING})
add_subdirectory(firmware)
else()
Expand Down
19 changes: 19 additions & 0 deletions common/core/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
add_library(common-core STATIC tool_detection.cpp)
target_include_directories(common-core PUBLIC ${CMAKE_SOURCE_DIR}/include)
target_link_libraries(common-core PUBLIC can-core)

set_target_properties(common-core
PROPERTIES CXX_STANDARD 20
CXX_STANDARD_REQUIRED TRUE)

target_compile_options(common-core
PRIVATE
-Wall
-Werror
-Weffc++
-Wreorder
-Wsign-promo
-Wextra-semi
-Wctor-dtor-privacy
-fno-rtti
)
74 changes: 74 additions & 0 deletions common/core/tool_detection.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#include "common/core/tool_detection.hpp"

#include <array>
#include <span>

using namespace tool_detection;

constexpr static auto pipette_384_chan_z_bounds =
ToolCheckBounds{.upper = 1883, .lower = 1851};

constexpr static auto pipette_96_chan_z_bounds =
ToolCheckBounds{.upper = 1434, .lower = 1400};

constexpr static auto pipette_single_chan_z_bounds =
ToolCheckBounds{.upper = 460, .lower = 444};

constexpr static auto pipette_multiple_chan_z_bounds =
ToolCheckBounds{.upper = 945, .lower = 918};

constexpr static auto pipette_single_chan_a_bounds =
ToolCheckBounds{.upper = 2389, .lower = 2362};

constexpr static auto pipette_multiple_chan_a_bounds =
ToolCheckBounds{.upper = 2860, .lower = 2844};

constexpr static auto nothing_connected_z_bounds =
ToolCheckBounds{.upper = 3, .lower = 1};

constexpr static auto nothing_connected_a_bounds =
ToolCheckBounds{.upper = 54, .lower = 16};

// revisit these, not sure if EE has a calculation for gripper carrier bounds
constexpr static auto nothing_connected_gripper_bounds =
ToolCheckBounds{.upper = 54, .lower = 16};

constexpr static auto gripper_bounds =
ToolCheckBounds{.upper = 999, .lower = 536};

constexpr static auto undefined_bounds =
ToolCheckBounds{.upper = 0, .lower = 0};

static std::array tool_list(std::to_array<Tool>(
{Tool{.tool_type = can_ids::ToolType::pipette_384_chan,
.tool_carrier = Z_CARRIER,
.bounds = pipette_384_chan_z_bounds},
Tool{.tool_type = can_ids::ToolType::pipette_96_chan,
.tool_carrier = Z_CARRIER,
.bounds = pipette_96_chan_z_bounds},
Tool{.tool_type = can_ids::ToolType::pipette_single_chan,
.tool_carrier = Z_CARRIER,
.bounds = pipette_single_chan_z_bounds},
Tool{.tool_type = can_ids::ToolType::pipette_multi_chan,
.tool_carrier = Z_CARRIER,
.bounds = pipette_multiple_chan_z_bounds},
Tool{.tool_type = can_ids::ToolType::nothing_attached,
.tool_carrier = Z_CARRIER,
.bounds = nothing_connected_z_bounds},
Tool{.tool_type = can_ids::ToolType::pipette_single_chan,
.tool_carrier = A_CARRIER,
.bounds = pipette_single_chan_a_bounds},
Tool{.tool_type = can_ids::ToolType::pipette_multi_chan,
.tool_carrier = A_CARRIER,
.bounds = pipette_multiple_chan_a_bounds},
Tool{.tool_type = can_ids::ToolType::nothing_attached,
.tool_carrier = A_CARRIER,
.bounds = nothing_connected_a_bounds},
Tool{.tool_type = can_ids::ToolType::gripper,
.tool_carrier = GRIPPER_CARRIER,
.bounds = gripper_bounds},
Tool{.tool_type = can_ids::ToolType::nothing_attached,
.tool_carrier = GRIPPER_CARRIER,
.bounds = nothing_connected_gripper_bounds}}));

std::span<Tool> tool_detection::lookup_table() { return std::span(tool_list); }
3 changes: 2 additions & 1 deletion common/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ add_executable(
test_bit_utils.cpp
test_synchronization.cpp
test_spi.cpp
test_tool_detection.cpp
)

target_include_directories(common PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/../../include)
Expand All @@ -28,7 +29,7 @@ target_compile_options(common
-Wextra-semi
-Wctor-dtor-privacy
-fno-rtti)
target_link_libraries(common Catch2::Catch2 common-simulation)
target_link_libraries(common Catch2::Catch2 common-simulation common-core)

catch_discover_tests(common)
add_build_and_test_target(common)
107 changes: 107 additions & 0 deletions common/tests/test_tool_detection.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
#include <algorithm>
#include <ranges>
#include <vector>

#include "can/core/ids.hpp"
#include "catch2/catch.hpp"
#include "common/core/tool_detection.hpp"

// Filter tests - no BDD here because these macros don't have
// BDD equivalents
TEMPLATE_TEST_CASE_SIG("tool list by pipette type", "",
((can_ids::ToolType TT), TT),
(can_ids::ToolType::pipette_96_chan),
(can_ids::ToolType::pipette_384_chan),
(can_ids::ToolType::pipette_single_chan),
(can_ids::ToolType::pipette_multi_chan),
(can_ids::ToolType::gripper),
(can_ids::ToolType::nothing_attached)) {
SECTION("lookup table filtering") {
auto tool_only = tool_detection::lookup_table_filtered(TT);
bool checked_anything = false;
for (auto tool_elem : tool_only) {
checked_anything = true;
REQUIRE(tool_elem.tool_type == TT);
}
CHECK(checked_anything);
}
}

TEMPLATE_TEST_CASE_SIG("tool list by carrier", "",
((tool_detection::Carrier C), C),
(tool_detection::Carrier::GRIPPER_CARRIER),
(tool_detection::Carrier::A_CARRIER),
(tool_detection::Carrier::Z_CARRIER)) {
auto carrier_only = tool_detection::lookup_table_filtered(C);
bool checked_anything = false;
for (auto tool_elem : carrier_only) {
checked_anything = true;
REQUIRE(tool_elem.tool_carrier == C);
}
CHECK(checked_anything);
}

SCENARIO("ot3 tools list for tool detection") {
GIVEN("ot3 tools lower and upper voltage bounds for tool detection") {
auto tools = tool_detection::lookup_table();
WHEN("when lower bound compared with upper bound") {
THEN("lower bound is always found lower or equal to upper bounds") {
for (auto& element : tools) {
REQUIRE(element.bounds.lower <= element.bounds.upper);
}
}
}
WHEN("you pass in an out-of-bounds read") {
uint16_t reading = 0;
THEN("the tool and carrier are undefined") {
REQUIRE(tool_detection::tooltype_from_reading(reading) ==
can_ids::ToolType::undefined_tool);
REQUIRE(tool_detection::carrier_from_reading(reading) ==
tool_detection::Carrier::UNKNOWN);
}
}
WHEN("checking holes in the bounds set for tool mounts") {
auto which_tool = GENERATE(tool_detection::Carrier::A_CARRIER,
tool_detection::Carrier::Z_CARRIER);
// can't modify the lookup table, gotta copy
auto filtered = tool_detection::lookup_table_filtered(which_tool);
auto copied = std::vector(filtered.begin(), filtered.end());
std::sort(copied.begin(), copied.end(),
[](const tool_detection::Tool& a,
const tool_detection::Tool& b) {
return a.bounds.upper < b.bounds.upper;
});
auto trailer =
std::ranges::subrange(copied.begin(), --copied.end());
auto leader = std::ranges::subrange(++copied.begin(), copied.end());
for (auto trailer_it = trailer.begin(), leader_it = leader.begin();
trailer_it != trailer.end() && leader_it != leader.end();
trailer_it++, leader_it++) {
INFO("Checking tool detection for carrier "
<< static_cast<uint32_t>(which_tool) << ", tool type "
<< static_cast<uint32_t>(trailer_it->tool_type) << " ("
<< trailer_it->bounds.lower << "->"
<< trailer_it->bounds.upper << "mV) -> "
<< static_cast<uint32_t>(leader_it->tool_type) << " ("
<< leader_it->bounds.lower << "->"
<< leader_it->bounds.upper << "mV)");
// Just above the upper range of the first
REQUIRE(tool_detection::tooltype_from_reading(
trailer_it->bounds.upper, filtered) ==
can_ids::ToolType::undefined_tool);
// Just below the lower range of the second
REQUIRE(tool_detection::tooltype_from_reading(
leader_it->bounds.lower - 1, filtered) ==
can_ids::ToolType::undefined_tool);
// Dead-on in the middle
auto mid_value =
(trailer_it->bounds.upper + leader_it->bounds.lower) / 2;
INFO("Bounds gap mid value " << mid_value
<< " matched to a tool");
REQUIRE(tool_detection::tooltype_from_reading(mid_value,
filtered) ==
can_ids::ToolType::undefined_tool);
}
}
}
}
10 changes: 2 additions & 8 deletions head/core/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
function(target_head_core TARGET)
target_sources(${TARGET} PUBLIC
${CMAKE_CURRENT_FUNCTION_LIST_DIR}/tool_list.cpp
${CMAKE_CURRENT_FUNCTION_LIST_DIR}/tasks.cpp
${CMAKE_CURRENT_FUNCTION_LIST_DIR}/can_task.cpp)
target_include_directories(${TARGET} PUBLIC ${CMAKE_CURRENT_FUNCTION_LIST_DIR})
endfunction()

function(target_head_core_tool_list TARGET)
target_sources(${TARGET} PUBLIC
${CMAKE_CURRENT_FUNCTION_LIST_DIR}/tool_list.cpp)
target_include_directories(${TARGET} PUBLIC ${CMAKE_CURRENT_FUNCTION_LIST_DIR})
target_include_directories(${TARGET} PUBLIC ${CMAKE_CURRENT_FUNCTION_LIST_DIR})
target_link_libraries(${TARGET} PUBLIC common-core)
endfunction()
37 changes: 0 additions & 37 deletions head/core/tool_list.cpp

This file was deleted.

2 changes: 1 addition & 1 deletion head/firmware/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ list(TRANSFORM CLANG_EXTRA_ARGS PREPEND --extra-arg=-I)
list(APPEND CLANG_EXTRA_ARGS "--extra-arg=-frelaxed-template-template-args")
add_custom_target(head-lint
ALL
COMMAND ${Clang_CLANGTIDY_EXECUTABLE} ${CLANG_EXTRA_ARGS} -p ${CMAKE_BINARY_DIR} ${HEAD_FW_LINTABLE_SRCS} ${CORE_LINTABLE_SOURCES})
COMMAND ${Clang_CLANGTIDY_EXECUTABLE} "--extra-arg=-DOPENTRONS_CLANG_TIDY_WORKAROUND_44178" ${CLANG_EXTRA_ARGS} -p ${CMAKE_BINARY_DIR} ${HEAD_FW_LINTABLE_SRCS} ${CORE_LINTABLE_SOURCES})
list(APPEND LINT_TARGETS head-lint)
set(LINT_TARGETS ${LINT_TARGETS} PARENT_SCOPE)
# Runs cross-gdb (since CMAKE_CROSSCOMPILING_EMULATOR is set in an
Expand Down
6 changes: 1 addition & 5 deletions head/firmware/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include "common/firmware/spi_comms.hpp"
#include "head/core/presence_sensing_driver.hpp"
#include "head/core/tasks.hpp"
#include "head/core/tool_list.hpp"
#include "head/firmware/adc_comms.hpp"
#include "motor-control/core/linear_motion_system.hpp"
#include "motor-control/core/motor.hpp"
Expand Down Expand Up @@ -193,10 +192,7 @@ adc::ADC_interface ADC_intf1 = {

static auto ADC_comms = adc::ADC(ADC_intf1, ADC_intf2);

static auto attached_tools = ot3_tool_list::AttachedTool{};

static auto psd =
presence_sensing_driver::PresenceSensingDriver{ADC_comms, attached_tools};
static auto psd = presence_sensing_driver::PresenceSensingDriver{ADC_comms};

auto timer_for_notifier = freertos_timer::FreeRTOSTimer<pdMS_TO_TICKS(100)>(
"timer for notifier", ([] {
Expand Down
4 changes: 1 addition & 3 deletions head/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ target_compile_options(head
-Wctor-dtor-privacy
-fno-rtti)

target_head_core_tool_list(head)

target_link_libraries(head Catch2::Catch2 common-simulation)
target_link_libraries(head PUBLIC Catch2::Catch2 common-simulation common-core)

catch_discover_tests(head)
add_build_and_test_target(head)
39 changes: 11 additions & 28 deletions head/tests/test_presence_sensing_driver.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "catch2/catch.hpp"
#include "common/core/tool_detection.hpp"
#include "head/core/presence_sensing_driver.hpp"
#include "head/tests/mock_adc.hpp"

Expand All @@ -8,9 +9,7 @@ SCENARIO("get readings called on presence sensing driver") {
adc::RawADCReadings{.z_motor = 666, .a_motor = 666, .gripper = 666};
static auto adc_comms = adc::MockADC{raw_readings};

auto at = ot3_tool_list::AttachedTool{};

auto ps = presence_sensing_driver::PresenceSensingDriver(adc_comms, at);
auto ps = presence_sensing_driver::PresenceSensingDriver(adc_comms);

WHEN("get_readings func is called") {
auto voltage_readings = ps.get_readings();
Expand Down Expand Up @@ -40,10 +39,9 @@ SCENARIO("get_tool called on presence sensing driver") {
auto raw_readings = adc::RawADCReadings{
.z_motor = 2332, .a_motor = 3548, .gripper = 666};
static auto adc_comms = adc::MockADC{raw_readings};
auto ps = presence_sensing_driver::PresenceSensingDriver(
adc_comms, ot3_tool_list::AttachedTool{});
auto tools = ot3_tool_list::AttachedTool(
ps.get_readings(), ot3_tool_list::get_tool_list());
auto ps = presence_sensing_driver::PresenceSensingDriver(adc_comms);
auto tools = attached_tools::AttachedTools(
ps.get_readings(), tool_detection::lookup_table());

THEN("Tools mapped to voltage reading") {
REQUIRE(tools.gripper == can_ids::ToolType::gripper);
Expand All @@ -55,10 +53,9 @@ SCENARIO("get_tool called on presence sensing driver") {
auto raw_readings = adc::RawADCReadings{
.z_motor = 9999, .a_motor = 9999, .gripper = 9999};
static auto adc_comms = adc::MockADC{raw_readings};
auto ps = presence_sensing_driver::PresenceSensingDriver(
adc_comms, ot3_tool_list::AttachedTool{});
auto tools = ot3_tool_list::AttachedTool(
ps.get_readings(), ot3_tool_list::get_tool_list());
auto ps = presence_sensing_driver::PresenceSensingDriver(adc_comms);
auto tools = attached_tools::AttachedTools(
ps.get_readings(), tool_detection::lookup_table());

THEN("Tools mapped to voltage reading") {
REQUIRE(tools.gripper == can_ids::ToolType::undefined_tool);
Expand All @@ -72,10 +69,9 @@ SCENARIO("get_tool called on presence sensing driver") {
auto raw_readings =
adc::RawADCReadings{.z_motor = 2, .a_motor = 20, .gripper = 20};
static auto adc_comms = adc::MockADC{raw_readings};
auto ps = presence_sensing_driver::PresenceSensingDriver(
adc_comms, ot3_tool_list::AttachedTool{});
auto tools = ot3_tool_list::AttachedTool(
ps.get_readings(), ot3_tool_list::get_tool_list());
auto ps = presence_sensing_driver::PresenceSensingDriver(adc_comms);
auto tools = attached_tools::AttachedTools(
ps.get_readings(), tool_detection::lookup_table());

THEN("Tools mapped to voltage reading") {
REQUIRE(tools.gripper == can_ids::ToolType::nothing_attached);
Expand All @@ -85,16 +81,3 @@ SCENARIO("get_tool called on presence sensing driver") {
}
}
}

SCENARIO("ot3 tools list for tool detection") {
GIVEN("ot3 tools lower and upper voltage bounds for tool detection") {
auto tools = ot3_tool_list::get_tool_list();
WHEN("when lower bound compared with upper bound") {
THEN("lower bound is always found lower or equal to upper bounds") {
for (auto& element : tools) {
REQUIRE(element.bounds.lower <= element.bounds.upper);
}
}
}
}
}
Loading

0 comments on commit 31de101

Please sign in to comment.