Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ void StSpinMotorController::sendAndReceiveFrame(const MotorIndex motor,

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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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


motor_status_[motor].frame_count++;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ uint8_t TmcMotorController::readWriteByte(const uint8_t motor, const uint8_t dat
// The first byte should contain the address on a read operation.
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a constant, not a magic number


currently_reading_ = true;
currently_writing_ = false;
Expand All @@ -608,7 +608,7 @@ uint8_t TmcMotorController::readWriteByte(const uint8_t motor, const uint8_t dat
{
// we have all the bytes for this transfer, lets trigger the transfer and
// reset state
spiTransfer(file_descriptors_[motor], tx_, rx_, 5, spi_speed);
spiTransfer<5>(file_descriptors_[motor], tx_, rx_, spi_speed);
transfer_started_ = false;
}

Expand Down
10 changes: 5 additions & 5 deletions src/software/embedded/motor_controller/tmc_motor_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,13 @@ class TmcMotorController : public MotorController
std::unique_ptr<Gpio> reset_gpio_;

// Transfer Buffers for spiTransfer
uint8_t tx_[5] = {};
uint8_t rx_[5] = {};
std::array<uint8_t, 5> tx_ = {};
std::array<uint8_t, 5> rx_ = {};

// Transfer Buffers for readThenWriteSpiTransfer
uint8_t write_tx_[5] = {};
uint8_t read_tx_[5] = {};
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_ = {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

more magic numbers


// Transfer State
bool transfer_started_ = false;
Expand Down
25 changes: 14 additions & 11 deletions src/software/embedded/spi_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@

#include "software/logger/logger.h"

void spiTransfer(int fd, uint8_t const* tx, uint8_t const* rx, unsigned len,
template <unsigned len>
void spiTransfer(int fd, const std::array<uint8_t, len>& tx, std::array<uint8_t, len>& rx,
uint32_t spi_speed)
{
int ret;

struct spi_ioc_transfer tr[1];
memset(tr, 0, sizeof(tr));

tr[0].tx_buf = (unsigned long)tx;
tr[0].rx_buf = (unsigned long)rx;
tr[0].tx_buf = reinterpret_cast<uintptr_t>(tx.data());
tr[0].rx_buf = reinterpret_cast<uintptr_t>(rx.data());
tr[0].len = len;
tr[0].delay_usecs = 0;
tr[0].speed_hz = spi_speed;
Expand All @@ -27,24 +28,26 @@ void spiTransfer(int fd, uint8_t const* tx, uint8_t const* rx, unsigned len,
<< strerror(errno);
}

void readThenWriteSpiTransfer(int fd, const uint8_t* read_tx, const uint8_t* write_tx,
const uint8_t* read_rx, const uint32_t read_len,
const uint32_t write_len, uint32_t spi_speed)
template <uint32_t read_len, uint32_t write_len>
void readThenWriteSpiTransfer(int fd, const std::array<uint8_t, read_len>& read_tx,
std::array<uint8_t, write_len>& write_tx,
const std::array<uint8_t, read_len>& read_rx,
int32_t spi_speed)
{
uint8_t write_rx[5] = {0};
uint8_t write_rx[5] = {};

struct spi_ioc_transfer tr[2];
memset(tr, 0, sizeof(tr));

tr[0].tx_buf = (unsigned long)read_tx;
tr[0].rx_buf = (unsigned long)read_rx;
tr[0].tx_buf = reinterpret_cast<uintptr_t>(read_tx);
tr[0].rx_buf = reinterpret_cast<uintptr_t>(read_rx);
tr[0].len = read_len;
tr[0].delay_usecs = 0;
tr[0].speed_hz = spi_speed;
tr[0].bits_per_word = 8;
tr[0].cs_change = 0;
tr[1].tx_buf = (unsigned long)write_tx;
tr[1].rx_buf = (unsigned long)write_rx;
tr[1].tx_buf = reinterpret_cast<uintptr_t>(write_tx);
tr[1].rx_buf = reinterpret_cast<uintptr_t>(write_rx);
tr[1].len = write_len;
tr[1].delay_usecs = 0;
tr[1].speed_hz = spi_speed;
Expand Down
17 changes: 7 additions & 10 deletions src/software/embedded/spi_utils.h

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

template header files should be names .hpp

Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
#pragma once

#include <string.h>

#include <array>
#include <cstdint>

// TODO: #3747 Wrap in spi_utils namespace

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well do this TODO

// TODO: #3751 Use std::array instead of raw pointers for tx and rx buffers.
/**
* Trigger an SPI transfer over an open SPI connection
*
Expand All @@ -14,11 +12,10 @@
* @param fd the SPI file descriptor to transfer data over
* @param tx the tx buffer, data to send out
* @param rx the rx buffer, will be updated with data from the full-duplex transfer
* @param len the length of the tx and rx buffer
* @param spi_speed the speed to run spi at in Hz
*
*/
void spiTransfer(int fd, uint8_t const* tx, uint8_t const* rx, unsigned len,
template <unsigned len>
void spiTransfer(int fd, const std::array<uint8_t, len>& tx, std::array<uint8_t, len>& rx,
uint32_t spi_speed);

/**
Expand All @@ -32,7 +29,7 @@ void spiTransfer(int fd, uint8_t const* tx, uint8_t const* rx, unsigned len,
* @param read_rx the buffer our read response will be placed in
* @param spi_speed the speed to run spi at in Hz
*/
void readThenWriteSpiTransfer(
int fd, const uint8_t* read_tx, const uint8_t* write_tx, const uint8_t* read_rx,
const uint32_t read_len, const uint32_t write_len,
uint32_t spi_speed); // refactor to take std::array, not raw pointers
template <uint32_t read_len, uint32_t write_len>
void readThenWriteSpiTransfer(int fd, const std::array<uint8_t, read_len>& read_tx,
const std::array<uint8_t, write_len>& write_tx,
std::array<uint8_t, read_len>& read_rx, uint32_t spi_speed);
Loading