-
Notifications
You must be signed in to change notification settings - Fork 12
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(flex-stacker): implement home motor sequence #475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good functionally but some feedback on small improvements to the circular buffer
public: | ||
explicit CircularBuffer(std::size_t buffer_size, bool allow_overwrite = false) | ||
// NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays) | ||
: _buffer(std::make_unique<T[]>(buffer_size)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will allocate with malloc
which is generally something we try to avoid. I know we're under a time crunch, and this is only a single big allocation per buffer with almost always a program lifetime, so it's probably fine, but we should probably make this an allocation from a static buffer somewhere.
|
||
private: | ||
// NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays) | ||
std::unique_ptr<T[]> _buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c-style unspecified-VLAs are things that work but are fundamentally for solving a C problem of "wanting a tagged-length array" via e.g. struct { int size; char contents[]; }
such that contents can be any size and contained in the struct. We should not use this in C++ code.
Instead, since the max size is a ctor param and a const internally, let's promote it to a template argument and forward it to a std::array
:
template <typename T, std::size_t MaxSize>
struct CircularBuffer {
using BackingStore = std::array<T, MaxSize>;
CircularBuffer(bool allow_overwrite=false):
_buffer(std::make_unique<BackingStore>(), ... {...}
...
std::unique_ptr<BackingStore> _buffer;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'd then access max size via the template parameter or via _buffer->size()
@@ -51,8 +54,8 @@ struct MotorState { | |||
[[nodiscard]] auto get_speed_discont() const -> float { | |||
return speed_mm_per_sec_discont * get_usteps_per_mm(); | |||
} | |||
[[nodiscard]] auto get_distance(float mm) const -> float { | |||
return mm * get_usteps_per_mm(); | |||
[[nodiscard]] auto get_distance(float mm) const -> long { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's use sized vars e.g. uint64_t
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #475 +/- ##
==========================================
- Coverage 81.51% 81.25% -0.26%
==========================================
Files 77 77
Lines 6870 6951 +81
==========================================
+ Hits 5600 5648 +48
- Misses 1270 1303 +33
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Surprised you also deleted the move ctors but I trust the need to do so.
This PR implements the homing routine for each motor on the stacker.
Unlike the OT-3, the operation of the Flex stacker requires its axes to move from one end to the other most of the time. Only the Z needs to perform fixed distance movements to give clearance to the latch motor. So we've decided to put most of the movement logic on the firmware (motor task to be exact), instead of letting the host control the motors individually.
If we know the distance to the limit switch, we can make a motor perform a fast move until there's a few mms left, then slowly move towards the switch. To do that, we need to be able to plan multiple moves ahead and and only execute the next move in the buffer when the current move is done. Using a buffer inside the task ensures only one motor is moving at a time. The buffer size is set to 10 moves for now and might change when we implement more complicated commands such as retrieve and store labware.
There are not a single encoder on the stacker so we only do this fast homing sequence if the limit switch at the opposite end is triggered.
A followup PR will be needed to add the defaults for EVT.