pw_spi: Use non-virtual interface (NVI) pattern on pw::spi::Initiator

The public interface is no longer virtual, and implementations instead implement Do* versions of those methods. This allows the base class to add common functionality. Bug: 308479791 Change-Id: I235fe9c370973c6e4cdd21d86010d9ae18aed6eb Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/236234 Docs-Not-Needed: Anthony DiGirolamo <[email protected]> Commit-Queue: Jonathon Reinhart <[email protected]> Reviewed-by: Anthony DiGirolamo <[email protected]> Lint: Lint 🤖 <[email protected]>
google · Oct 9, 2024 · 4321a46 · 4321a46
1 parent 1e33c23
commit 4321a46
Show file tree

Hide file tree

Showing 12 changed files with 67 additions and 43 deletions.
diff --git a/pw_spi/initiator_mock.cc b/pw_spi/initiator_mock.cc
@@ -20,7 +20,7 @@
 
 namespace pw::spi {
 
-Status MockInitiator::WriteRead(ConstByteSpan tx_buffer, ByteSpan rx_buffer) {
+Status MockInitiator::DoWriteRead(ConstByteSpan tx_buffer, ByteSpan rx_buffer) {
  PW_CHECK_INT_LT(expected_transaction_index_, expected_transactions_.size());
 
  ConstByteSpan expected_tx_buffer =

diff --git a/pw_spi/public/pw_spi/initiator.h b/pw_spi/public/pw_spi/initiator.h
@@ -89,7 +89,9 @@ class Initiator {
  // bits-per-word.
  // Returns OkStatus() on success, and implementation-specific values on
  // failure.
- virtual Status Configure(const Config& config) = 0;
+ // TODO: https://pwbug.dev/308479791 - Remove virtual when all implementors
+ // have moved to DoConfigure().
+ virtual Status Configure(const Config& config) { return DoConfigure(config); }
 
  // Perform a synchronous read/write operation on the SPI bus. Data from the
  // `write_buffer` object is written to the bus, while the `read_buffer` is
@@ -101,8 +103,27 @@ class Initiator {
  // remainder of the transfer.
  // Returns OkStatus() on success, and implementation-specific values on
  // failure.
- virtual Status WriteRead(ConstByteSpan write_buffer,
- ByteSpan read_buffer) = 0;
+ // TODO: https://pwbug.dev/308479791 - Remove virtual when all implementors
+ // have moved to DoWriteRead().
+ virtual Status WriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer) {
+ return DoWriteRead(write_buffer, read_buffer);
+ }
+
+ private:
+ // Subclass API:
+ virtual Status DoConfigure(const Config& /*config*/) {
+ // TODO: https://pwbug.dev/308479791 - Remove this definition,
+ // making this an abstract interface, when all implementors have
+ // moved to DoConfigure().
+ return pw::Status::Unimplemented();
+ }
+ virtual Status DoWriteRead(ConstByteSpan /*write_buffer*/,
+ ByteSpan /*read_buffer*/) {
+ // TODO: https://pwbug.dev/308479791 - Remove this definition,
+ // making this an abstract interface, when all implementors have
+ // moved to DoWriteRead().
+ return pw::Status::Unimplemented();
+ }
 };
 
 } // namespace pw::spi
diff --git a/pw_spi/public/pw_spi/initiator_mock.h b/pw_spi/public/pw_spi/initiator_mock.h
@@ -89,6 +89,10 @@ class MockInitiator : public pw::spi::Initiator {
  // Runs Finalize() regardless of whether it was already optionally finalized.
  ~MockInitiator() override;
 
+ private:
+ span<MockTransaction> expected_transactions_;
+ size_t expected_transaction_index_;
+
  // Implements a mocked backend for the SPI initiator.
  //
  // Expects (via Gtest):
@@ -102,15 +106,11 @@ class MockInitiator : public pw::spi::Initiator {
  //
  // Returns:
  // Specified transaction return type
- pw::Status WriteRead(pw::ConstByteSpan, pw::ByteSpan) override;
+ pw::Status DoWriteRead(pw::ConstByteSpan, pw::ByteSpan) override;
 
- pw::Status Configure(const pw::spi::Config& /*config */) override {
+ pw::Status DoConfigure(const pw::spi::Config& /*config */) override {
  return pw::OkStatus();
  }
-
- private:
- span<MockTransaction> expected_transactions_;
- size_t expected_transaction_index_;
 };
 
 // Makes a new SPI transactions list.

diff --git a/pw_spi/spi_test.cc b/pw_spi/spi_test.cc
@@ -42,10 +42,12 @@ class SpiTestDevice : public ::testing::Test {
  private:
  // Stub SPI Initiator/ChipSelect objects, used to exercise public API surface.
  class TestInitiator : public Initiator {
- public:
- Status Configure(const Config& /*config */) override { return OkStatus(); }
- Status WriteRead(ConstByteSpan /* write_buffer */,
- ByteSpan /* read_buffer */) override {
+ private:
+ Status DoConfigure(const Config& /*config */) override {
+ return OkStatus();
+ }
+ Status DoWriteRead(ConstByteSpan /* write_buffer */,
+ ByteSpan /* read_buffer */) override {
  return OkStatus();
  }
  };

diff --git a/pw_spi_linux/public/pw_spi_linux/spi.h b/pw_spi_linux/public/pw_spi_linux/spi.h
@@ -33,14 +33,14 @@ class LinuxInitiator : public Initiator {
  : max_speed_hz_(max_speed_hz), fd_(fd) {}
  ~LinuxInitiator() override;
 
- // Implements pw::spi::Initiator
- Status Configure(const Config& config) override;
- Status WriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer) override;
-
  private:
  uint32_t max_speed_hz_;
  int fd_;
  std::optional<Config> current_config_;
+
+ // Implements pw::spi::Initiator
+ Status DoConfigure(const Config& config) override;
+ Status DoWriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer) override;
 };
 
 // Linux userspace implementation of SPI ChipSelector

diff --git a/pw_spi_linux/spi.cc b/pw_spi_linux/spi.cc
@@ -33,7 +33,7 @@ LinuxInitiator::~LinuxInitiator() {
  }
 }
 
-Status LinuxInitiator::Configure(const Config& config) {
+Status LinuxInitiator::DoConfigure(const Config& config) {
  if (current_config_ == config) {
  // Don't waste time issuing ioctls if the config is not actually changing.
  return OkStatus();
@@ -79,8 +79,8 @@ Status LinuxInitiator::Configure(const Config& config) {
  return OkStatus();
 }
 
-Status LinuxInitiator::WriteRead(ConstByteSpan write_buffer,
- ByteSpan read_buffer) {
+Status LinuxInitiator::DoWriteRead(ConstByteSpan write_buffer,
+  ByteSpan read_buffer) {
  // Configure a full-duplex transfer using ioctl()
 
  struct spi_ioc_transfer transaction[2] = {};

diff --git a/pw_spi_mcuxpresso/flexio_spi.cc b/pw_spi_mcuxpresso/flexio_spi.cc
@@ -118,7 +118,7 @@ void McuxpressoFlexIoInitiator::SpiCallback(FLEXIO_SPI_Type*,
  driver->transfer_semaphore_.release();
 }
 
-Status McuxpressoFlexIoInitiator::Configure(const Config& config) {
+Status McuxpressoFlexIoInitiator::DoConfigure(const Config& config) {
  if (current_config_ && config == *current_config_) {
  return OkStatus();
  }
@@ -174,8 +174,8 @@ Status McuxpressoFlexIoInitiator::Configure(const Config& config) {
  return status;
 }
 
-Status McuxpressoFlexIoInitiator::WriteRead(ConstByteSpan write_buffer,
- ByteSpan read_buffer) {
+Status McuxpressoFlexIoInitiator::DoWriteRead(ConstByteSpan write_buffer,
+  ByteSpan read_buffer) {
  flexio_spi_transfer_t transfer = {};
  transfer.txData =
  reinterpret_cast<uint8_t*>(const_cast<std::byte*>(write_buffer.data()));

diff --git a/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/flexio_spi.h b/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/flexio_spi.h
@@ -38,11 +38,6 @@ class McuxpressoFlexIoInitiator : public Initiator {
  blocking_(blocking) {}
  ~McuxpressoFlexIoInitiator();
 
- // Implements pw::spi::Initiator
- pw::Status Configure(const Config& config) PW_LOCKS_EXCLUDED(mutex_) override;
- pw::Status WriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer)
- PW_LOCKS_EXCLUDED(mutex_) override;
-
  private:
  // inclusive-language: disable
  static void SpiCallback(FLEXIO_SPI_Type*,
@@ -55,6 +50,12 @@ class McuxpressoFlexIoInitiator : public Initiator {
 
  bool is_initialized() { return !!current_config_; }
 
+ // Implements pw::spi::Initiator
+ pw::Status DoConfigure(const Config& config)
+ PW_LOCKS_EXCLUDED(mutex_) override;
+ pw::Status DoWriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer)
+ PW_LOCKS_EXCLUDED(mutex_) override;
+
  std::optional<const Config> current_config_;
  FLEXIO_SPI_Type flexio_spi_config_;
  flexio_spi_master_handle_t driver_handle_;

diff --git a/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/spi.h b/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/spi.h
@@ -41,11 +41,6 @@ class McuxpressoInitiator : public Initiator {
  blocking_(blocking) {}
  ~McuxpressoInitiator();
 
- // Implements pw::spi::Initiator
- Status Configure(const Config& config) PW_LOCKS_EXCLUDED(mutex_) override;
- Status WriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer)
- PW_LOCKS_EXCLUDED(mutex_) override;
-
  Status SetChipSelect(uint32_t pin) PW_LOCKS_EXCLUDED(mutex_);
 
  private:
@@ -60,6 +55,11 @@ class McuxpressoInitiator : public Initiator {
 
  bool is_initialized() { return !!current_config_; }
 
+ // Implements pw::spi::Initiator
+ Status DoConfigure(const Config& config) PW_LOCKS_EXCLUDED(mutex_) override;
+ Status DoWriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer)
+ PW_LOCKS_EXCLUDED(mutex_) override;
+
  SPI_Type* register_map_;
  spi_master_handle_t driver_handle_;
  // inclusive-language: enable

diff --git a/pw_spi_mcuxpresso/spi.cc b/pw_spi_mcuxpresso/spi.cc
@@ -72,7 +72,7 @@ void McuxpressoInitiator::SpiCallback(SPI_Type*,
  driver->transfer_semaphore_.release();
 }
 
-Status McuxpressoInitiator::Configure(const Config& config) {
+Status McuxpressoInitiator::DoConfigure(const Config& config) {
  std::lock_guard lock(mutex_);
  if (current_config_ && config == *current_config_) {
  return OkStatus();
@@ -126,8 +126,8 @@ Status McuxpressoInitiator::DoConfigureLocked(
  return status;
 }
 
-Status McuxpressoInitiator::WriteRead(ConstByteSpan write_buffer,
- ByteSpan read_buffer) {
+Status McuxpressoInitiator::DoWriteRead(ConstByteSpan write_buffer,
+  ByteSpan read_buffer) {
  spi_transfer_t transfer = {};
 
  transfer.txData =

diff --git a/pw_spi_rp2040/initiator.cc b/pw_spi_rp2040/initiator.cc
@@ -61,7 +61,7 @@ constexpr spi_cpol_t GetPolarity(ClockPolarity polarity) {
 
 } // namespace
 
-Status Rp2040Initiator::Configure(const Config& config) {
+Status Rp2040Initiator::DoConfigure(const Config& config) {
  PW_ASSERT(config.bits_per_word() == 8);
 
  spi_set_format(spi_,
@@ -73,8 +73,8 @@ Status Rp2040Initiator::Configure(const Config& config) {
  return OkStatus();
 }
 
-Status Rp2040Initiator::WriteRead(ConstByteSpan write_buffer,
- ByteSpan read_buffer) {
+Status Rp2040Initiator::DoWriteRead(ConstByteSpan write_buffer,
+  ByteSpan read_buffer) {
  if (write_buffer.empty() && !read_buffer.empty()) {
  // Read only transaction.
  spi_read_blocking(spi_,

diff --git a/pw_spi_rp2040/public/pw_spi_rp2040/initiator.h b/pw_spi_rp2040/public/pw_spi_rp2040/initiator.h
@@ -26,11 +26,11 @@ class Rp2040Initiator final : public Initiator {
  public:
  Rp2040Initiator(spi_inst_t* spi) : spi_(spi) {}
 
+ private:
  // Implements pw::spi::Initiator:
- Status Configure(const Config& config) override;
- Status WriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer) override;
+ Status DoConfigure(const Config& config) override;
+ Status DoWriteRead(ConstByteSpan write_buffer, ByteSpan read_buffer) override;
 
- private:
  spi_inst_t* spi_;
 };