spi_transfer Refactor Pointers into std::array#3755
Conversation
adrianchan787
requested review from
Andrewyx,
sauravbanna and
williamckha
as code owners
someone2060
left a comment
someone2060
left a comment
There was a problem hiding this comment.
I've noticed that spi_utils.cpp241:: static const inline std::unordered_map<MotorIndex, const char*> SPI_PATHS still contains a pointer.
Is it possible to remove this pointer, or is it unnecessary to remove it?
I wanna say it's not necessary? way I interpreted the issue, it was just changing the spiTransfer functions in spi_utils. Maybe Grayson could clarify though |
| // Trigger a transfer (1 byte) and buffer the response (4 bytes) | ||
| tx_[position_] = data; | ||
| spiTransfer(file_descriptors_[motor], tx_, rx_, 5, spi_speed); | ||
| spiTransfer<5>(file_descriptors_[motor], tx_, rx_, spi_speed); |
There was a problem hiding this comment.
This should be a constant, not a magic number
| uint8_t read_rx_[5] = {}; | ||
| std::array<uint8_t, 5> write_tx_ = {}; | ||
| std::array<uint8_t, 5> read_tx_ = {}; | ||
| std::array<uint8_t, 5> read_rx_ = {}; |
There was a problem hiding this comment.
template header files should be names .hpp
|
the const char* is a c-style string and I think it should stay that way to be compatible with spidev open() |
williamckha
left a comment
williamckha
left a comment
There was a problem hiding this comment.
Regarding SPI_PATHS, we should use std::string_view. But that is out of scope for this task, so it's up to you if you want to make that change.
| const std::array<uint8_t, write_len>& write_tx, | ||
| std::array<uint8_t, read_len>& read_rx, uint32_t spi_speed) | ||
| { | ||
| uint8_t write_rx[5] = {}; |
There was a problem hiding this comment.
Should be
| uint8_t write_rx[5] = {}; | |
| uint8_t write_rx[write_len] = {}; |
| // ensure tx_ and rx_ are cleared | ||
| memset(read_tx_, 0, 5); | ||
| memset(write_tx_, 0, 5); | ||
| memset(read_rx_, 0, 5); |
There was a problem hiding this comment.
Any reason why we're not clearing the arrays any more? We can call .fill(0) on the std::arrays instead of using memset
There was a problem hiding this comment.
Was originally thinking they're initialized to 0 so no need, but just realized they're variables that exist for the lifetime of the controller so yeah my bad, I think you're right
| populateTx(outgoing_frame, tx); | ||
|
|
||
| spiTransfer(spi_fds_[motor], tx.data(), rx.data(), FRAME_LEN, SPI_SPEED_HZ); | ||
| spiTransfer<FRAME_LEN>(spi_fds_[motor], tx, rx, SPI_SPEED_HZ); |
There was a problem hiding this comment.
The template param can be deduced from the length of tx and rx, so you do not need to explicitly specify it with <FRAME_LEN>. There are similar cases in tmc_motor_controller.cpp
| #include "software/embedded/spi_utils.h" | ||
| #include "software/logger/logger.h" | ||
|
|
||
| // TODO: #3747 Wrap in spi_utils namespace |
4 participants
Description
Refactored pointers into arrays. Also changed the C-style casts for the relevant code, used uintptr instead of unsigned long for the casts, and made the receive buffers not constant.
Testing Done
No more pointers.
Resolved Issues
resolves #3751
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue