From c02044cc6353f0a8b92b06c48ad4603a306945fe Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 12 Oct 2025 16:03:41 -0500 Subject: [PATCH 1/4] feat(base_peripheral): Update mutex to be `mutable` and make all `read` methods `const`. --- .../include/base_peripheral.hpp | 168 +++++++++++------- 1 file changed, 105 insertions(+), 63 deletions(-) diff --git a/components/base_peripheral/include/base_peripheral.hpp b/components/base_peripheral/include/base_peripheral.hpp index 3b5210cd0..b1b64aafd 100644 --- a/components/base_peripheral/include/base_peripheral.hpp +++ b/components/base_peripheral/include/base_peripheral.hpp @@ -100,7 +100,7 @@ class BasePeripheral : public BaseComponent { /// \note If the probe function is not set, this function will return false /// and set the error code to operation_not_supported /// \note This function is only available if UseAddress is true - bool probe(std::error_code &ec) requires(UseAddress) { + bool probe(std::error_code &ec) const requires(UseAddress) { std::lock_guard lock(base_mutex_); if (base_config_.probe) { return base_config_.probe(base_config_.address); @@ -213,6 +213,14 @@ class BasePeripheral : public BaseComponent { base_config_ = std::move(config); } + /// Get the configuration for the peripheral + /// \return The configuration for the peripheral + const Config &config() const { return base_config_; } + + /// Get the address of the peripheral + /// \return The address of the peripheral + uint8_t address() const { return base_config_.address; } + protected: /// Constructor /// \param config The configuration for the peripheral @@ -223,14 +231,6 @@ class BasePeripheral : public BaseComponent { : BaseComponent(name, verbosity) , base_config_(config) {} - /// Get the configuration for the peripheral - /// \return The configuration for the peripheral - const Config &config() const { return base_config_; } - - /// Get the address of the peripheral - /// \return The address of the peripheral - uint8_t address() const { return base_config_.address; } - /// Write data to the peripheral /// \param data The data to write /// \param length The length of the data to write @@ -269,7 +269,7 @@ class BasePeripheral : public BaseComponent { /// \param data The buffer to read into /// \param length The length of the buffer /// \param ec The error code to set if there is an error - void read(uint8_t *data, size_t length, std::error_code &ec) requires(UseAddress) { + void read(uint8_t *data, size_t length, std::error_code &ec) const requires(UseAddress) { std::lock_guard lock(base_mutex_); if (base_config_.read) { if (!base_config_.read(base_config_.address, data, length)) { @@ -286,7 +286,7 @@ class BasePeripheral : public BaseComponent { /// \param data The buffer to read into /// \param length The length of the buffer /// \param ec The error code to set if there is an error - void read(uint8_t *data, size_t length, std::error_code &ec) requires(!UseAddress) { + void read(uint8_t *data, size_t length, std::error_code &ec) const requires(!UseAddress) { std::lock_guard lock(base_mutex_); if (base_config_.read) { if (!base_config_.read(data, length)) { @@ -305,7 +305,7 @@ class BasePeripheral : public BaseComponent { /// \param length The length of the buffer /// \param ec The error code to set if there is an error void read_register(RegisterAddressType reg_addr, uint8_t *data, size_t length, - std::error_code &ec) requires(UseAddress) { + std::error_code &ec) const requires(UseAddress) { std::lock_guard lock(base_mutex_); if (base_config_.read_register) { if (!base_config_.read_register(base_config_.address, reg_addr, data, length)) { @@ -314,7 +314,42 @@ class BasePeripheral : public BaseComponent { ec.clear(); } } else { - ec = std::make_error_code(std::errc::operation_not_supported); + // use the size of the register address to determine how many bytes the + // register address is + static constexpr size_t reg_addr_size = sizeof(RegisterAddressType); + uint8_t buffer[reg_addr_size]; + put_register_bytes(reg_addr, buffer); + + // now we need to determine if we can write_then_read, or if we need to + // write then read separately + if (base_config_.write_then_read) { + // we can use write_then_read + if (!base_config_.write_then_read(base_config_.address, buffer, reg_addr_size, data, + length)) { + ec = std::make_error_code(std::errc::io_error); + } else { + ec.clear(); + } + } else if (base_config_.write && base_config_.read) { + if (!base_config_.write(base_config_.address, buffer, reg_addr_size)) { + ec = std::make_error_code(std::errc::io_error); + return; + } + if (ec) { + return; + } + if (base_config_.separate_write_then_read_delay.count() > 0) { + std::this_thread::sleep_for(base_config_.separate_write_then_read_delay); + } + if (!base_config_.read(base_config_.address, data, length)) { + ec = std::make_error_code(std::errc::io_error); + return; + } + } else { + // we can't do anything + ec = std::make_error_code(std::errc::operation_not_supported); + return; + } } } @@ -324,7 +359,7 @@ class BasePeripheral : public BaseComponent { /// \param length The length of the buffer /// \param ec The error code to set if there is an error void read_register(RegisterAddressType reg_addr, uint8_t *data, size_t length, - std::error_code &ec) requires(!UseAddress) { + std::error_code &ec) const requires(!UseAddress) { std::lock_guard lock(base_mutex_); if (base_config_.read_register) { if (!base_config_.read_register(reg_addr, data, length)) { @@ -333,7 +368,38 @@ class BasePeripheral : public BaseComponent { ec.clear(); } } else { - ec = std::make_error_code(std::errc::operation_not_supported); + // use the size of the register address to determine how many bytes the + // register address is + static constexpr size_t reg_addr_size = sizeof(RegisterAddressType); + uint8_t buffer[reg_addr_size]; + put_register_bytes(reg_addr, buffer); + + // now we need to determine if we can write_then_read, or if we need to + // write then read separately + if (base_config_.write_then_read) { + // we can use write_then_read + if (!base_config_.write_then_read(buffer, reg_addr_size, data, length)) { + ec = std::make_error_code(std::errc::io_error); + } else { + ec.clear(); + } + } else if (base_config_.write && base_config_.read) { + if (!base_config_.write(buffer, reg_addr_size)) { + ec = std::make_error_code(std::errc::io_error); + return; + } + if (base_config_.separate_write_then_read_delay.count() > 0) { + std::this_thread::sleep_for(base_config_.separate_write_then_read_delay); + } + if (!base_config_.read(data, length)) { + ec = std::make_error_code(std::errc::io_error); + return; + } + } else { + // we can't do anything + ec = std::make_error_code(std::errc::operation_not_supported); + return; + } } } @@ -356,14 +422,17 @@ class BasePeripheral : public BaseComponent { } } else if (base_config_.write && base_config_.read) { logger_.debug("write {} bytes then read {} bytes", write_length, read_length); - write(write_data, write_length, ec); - if (ec) { + if (!base_config_.write(base_config_.address, write_data, write_length)) { + ec = std::make_error_code(std::errc::io_error); return; } if (base_config_.separate_write_then_read_delay.count() > 0) { std::this_thread::sleep_for(base_config_.separate_write_then_read_delay); } - read(read_data, read_length, ec); + if (!base_config_.read(base_config_.address, read_data, read_length)) { + ec = std::make_error_code(std::errc::io_error); + return; + } } else { ec = std::make_error_code(std::errc::operation_not_supported); } @@ -445,7 +514,7 @@ class BasePeripheral : public BaseComponent { /// \param data The buffer to read into /// \param length The length of the buffer /// \param ec The error code to set if there is an error - void read_many(uint8_t *data, size_t length, std::error_code &ec) { + void read_many(uint8_t *data, size_t length, std::error_code &ec) const { logger_.debug("read {} bytes", length); read(data, length, ec); } @@ -456,7 +525,7 @@ class BasePeripheral : public BaseComponent { /// \note You should ensure that the buffer is the correct size before calling /// this function since the buffer size is what is used to know how many /// bytes to read - void read_many(std::vector &data, std::error_code &ec) { + void read_many(std::vector &data, std::error_code &ec) const { logger_.debug("read {} bytes", data.size()); read(data.data(), data.size(), ec); } @@ -464,7 +533,7 @@ class BasePeripheral : public BaseComponent { /// Read a uint8_t from the peripheral /// \param ec The error code to set if there is an error /// \return The data read from the peripheral - uint8_t read_u8(std::error_code &ec) { + uint8_t read_u8(std::error_code &ec) const { logger_.debug("read u8"); uint8_t data = 0; read(&data, 1, ec); @@ -477,7 +546,7 @@ class BasePeripheral : public BaseComponent { /// Read a uint16_t from the peripheral /// \param ec The error code to set if there is an error /// \return The data read from the peripheral - uint16_t read_u16(std::error_code &ec) { + uint16_t read_u16(std::error_code &ec) const { logger_.debug("read u16"); uint16_t data = 0; uint8_t buffer[2]; @@ -559,22 +628,13 @@ class BasePeripheral : public BaseComponent { /// \param register_address The address of the register to read from /// \param ec The error code to set if there is an error /// \return The data read from the peripheral - uint8_t read_u8_from_register(RegisterAddressType register_address, std::error_code &ec) { + uint8_t read_u8_from_register(RegisterAddressType register_address, std::error_code &ec) const { logger_.debug("read u8 from register 0x{:x}", register_address); uint8_t data = 0; std::lock_guard lock(base_mutex_); - if (base_config_.read_register) { - read_register(register_address, &data, 1, ec); - } else { - // use the size of the register address to determine how many bytes the - // register address is - static constexpr size_t reg_addr_size = sizeof(RegisterAddressType); - uint8_t buffer[reg_addr_size]; - put_register_bytes(register_address, buffer); - write_then_read(buffer, reg_addr_size, &data, 1, ec); - if (ec) { - return 0; - } + read_register(register_address, &data, 1, ec); + if (ec) { + return 0; } return data; } @@ -583,22 +643,13 @@ class BasePeripheral : public BaseComponent { /// \param register_address The address of the register to read from /// \param ec The error code to set if there is an error /// \return The data read from the peripheral - uint16_t read_u16_from_register(RegisterAddressType register_address, std::error_code &ec) { + uint16_t read_u16_from_register(RegisterAddressType register_address, std::error_code &ec) const { logger_.debug("read u16 from register 0x{:x}", register_address); uint8_t data[2]; std::lock_guard lock(base_mutex_); - if (base_config_.read_register) { - read_register(register_address, data, 2, ec); - } else { - // use the size of the register address to determine how many bytes the - // register address is - static constexpr size_t reg_addr_size = sizeof(RegisterAddressType); - uint8_t buffer[reg_addr_size]; - put_register_bytes(register_address, buffer); - write_then_read(buffer, reg_addr_size, (uint8_t *)&data, 2, ec); - if (ec) { - return 0; - } + read_register(register_address, data, 2, ec); + if (ec) { + return 0; } if constexpr (BigEndianData) { return (data[0] << 8) | data[1]; @@ -613,20 +664,11 @@ class BasePeripheral : public BaseComponent { /// \param length The length of the buffer /// \param ec The error code to set if there is an error void read_many_from_register(RegisterAddressType register_address, uint8_t *data, size_t length, - std::error_code &ec) { + std::error_code &ec) const { logger_.debug("read_many_from_register {} bytes from register 0x{:x}", length, register_address); std::lock_guard lock(base_mutex_); - if (base_config_.read_register) { - read_register(register_address, data, length, ec); - } else { - // use the size of the register address to determine how many bytes the - // register address is - static constexpr size_t reg_addr_size = sizeof(RegisterAddressType); - uint8_t buffer[reg_addr_size]; - put_register_bytes(register_address, buffer); - write_then_read(buffer, reg_addr_size, data, length, ec); - } + read_register(register_address, data, length, ec); } /// Read many bytes from a register on the peripheral @@ -634,7 +676,7 @@ class BasePeripheral : public BaseComponent { /// \param data The buffer to read into /// \param ec The error code to set if there is an error void read_many_from_register(RegisterAddressType register_address, std::vector &data, - std::error_code &ec) { + std::error_code &ec) const { logger_.debug("read_many_from_register {} bytes from register 0x{:x}", data.size(), register_address); read_many_from_register(register_address, data.data(), data.size(), ec); @@ -771,7 +813,7 @@ class BasePeripheral : public BaseComponent { } // Set the bytes of the register address in the buffer - void put_register_bytes(RegisterAddressType register_address, uint8_t *data) { + void put_register_bytes(RegisterAddressType register_address, uint8_t *data) const { if constexpr (std::is_same_v) { data[0] = register_address; } else { @@ -794,7 +836,7 @@ class BasePeripheral : public BaseComponent { } } - Config base_config_; ///< The configuration for the peripheral - std::recursive_mutex base_mutex_; ///< The mutex to protect access to the peripheral + Config base_config_; ///< The configuration for the peripheral + mutable std::recursive_mutex base_mutex_; ///< The mutex to protect access to the peripheral }; } // namespace espp From c909fa4ead1eab43822a7c36e30a5c3e378151a7 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 12 Oct 2025 16:08:34 -0500 Subject: [PATCH 2/4] remove extraneous ec check --- components/base_peripheral/include/base_peripheral.hpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/components/base_peripheral/include/base_peripheral.hpp b/components/base_peripheral/include/base_peripheral.hpp index b1b64aafd..d27273ba8 100644 --- a/components/base_peripheral/include/base_peripheral.hpp +++ b/components/base_peripheral/include/base_peripheral.hpp @@ -335,9 +335,6 @@ class BasePeripheral : public BaseComponent { ec = std::make_error_code(std::errc::io_error); return; } - if (ec) { - return; - } if (base_config_.separate_write_then_read_delay.count() > 0) { std::this_thread::sleep_for(base_config_.separate_write_then_read_delay); } From d2e9df15b300b1f1f33645911e69372f1e003fd1 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 12 Oct 2025 16:13:33 -0500 Subject: [PATCH 3/4] fix accidental change --- components/base_peripheral/include/base_peripheral.hpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/components/base_peripheral/include/base_peripheral.hpp b/components/base_peripheral/include/base_peripheral.hpp index d27273ba8..04e22917b 100644 --- a/components/base_peripheral/include/base_peripheral.hpp +++ b/components/base_peripheral/include/base_peripheral.hpp @@ -419,17 +419,14 @@ class BasePeripheral : public BaseComponent { } } else if (base_config_.write && base_config_.read) { logger_.debug("write {} bytes then read {} bytes", write_length, read_length); - if (!base_config_.write(base_config_.address, write_data, write_length)) { - ec = std::make_error_code(std::errc::io_error); + write(write_data, write_length, ec); + if (ec) { return; } if (base_config_.separate_write_then_read_delay.count() > 0) { std::this_thread::sleep_for(base_config_.separate_write_then_read_delay); } - if (!base_config_.read(base_config_.address, read_data, read_length)) { - ec = std::make_error_code(std::errc::io_error); - return; - } + read(read_data, read_length, ec); } else { ec = std::make_error_code(std::errc::operation_not_supported); } From 257f7959765bce509b0cbc13afbc2952148fdea5 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 12 Oct 2025 16:15:39 -0500 Subject: [PATCH 4/4] remove unnecessary locking --- components/base_peripheral/include/base_peripheral.hpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/components/base_peripheral/include/base_peripheral.hpp b/components/base_peripheral/include/base_peripheral.hpp index 04e22917b..e2b9188e4 100644 --- a/components/base_peripheral/include/base_peripheral.hpp +++ b/components/base_peripheral/include/base_peripheral.hpp @@ -625,7 +625,6 @@ class BasePeripheral : public BaseComponent { uint8_t read_u8_from_register(RegisterAddressType register_address, std::error_code &ec) const { logger_.debug("read u8 from register 0x{:x}", register_address); uint8_t data = 0; - std::lock_guard lock(base_mutex_); read_register(register_address, &data, 1, ec); if (ec) { return 0; @@ -640,7 +639,6 @@ class BasePeripheral : public BaseComponent { uint16_t read_u16_from_register(RegisterAddressType register_address, std::error_code &ec) const { logger_.debug("read u16 from register 0x{:x}", register_address); uint8_t data[2]; - std::lock_guard lock(base_mutex_); read_register(register_address, data, 2, ec); if (ec) { return 0; @@ -661,7 +659,6 @@ class BasePeripheral : public BaseComponent { std::error_code &ec) const { logger_.debug("read_many_from_register {} bytes from register 0x{:x}", length, register_address); - std::lock_guard lock(base_mutex_); read_register(register_address, data, length, ec); }