From 98f2a0b4a440b160e99c5526b353610062c88232 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Sun, 26 Jul 2026 00:57:35 -0700 Subject: [PATCH] [modbus] Typed send helpers; migrate in-tree callers off send() (#17859) --- .../growatt_solar/growatt_solar.cpp | 3 +- .../havells_solar/havells_solar.cpp | 3 +- esphome/components/kuntze/kuntze.cpp | 3 +- esphome/components/modbus/modbus.cpp | 21 +++++ esphome/components/modbus/modbus.h | 39 +++++++++- .../modbus_controller/modbus_controller.cpp | 5 +- esphome/components/pzemac/pzemac.cpp | 3 +- esphome/components/pzemdc/pzemdc.cpp | 3 +- esphome/components/sdm_meter/sdm_meter.cpp | 3 +- .../components/selec_meter/selec_meter.cpp | 3 +- .../modbus/modbus_client_hub_test.cpp | 77 +++++++++++++++++++ 11 files changed, 144 insertions(+), 19 deletions(-) diff --git a/esphome/components/growatt_solar/growatt_solar.cpp b/esphome/components/growatt_solar/growatt_solar.cpp index 6485e90c25..d2102496a2 100644 --- a/esphome/components/growatt_solar/growatt_solar.cpp +++ b/esphome/components/growatt_solar/growatt_solar.cpp @@ -7,7 +7,6 @@ namespace esphome::growatt_solar { static const char *const TAG = "growatt_solar"; -static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t MODBUS_REGISTER_COUNT[] = {33, 95}; // indexed with enum GrowattProtocolVersion void GrowattSolar::loop() { @@ -31,7 +30,7 @@ void GrowattSolar::update() { } this->waiting_to_update_ = false; - this->send(MODBUS_CMD_READ_IN_REGISTERS, 0, MODBUS_REGISTER_COUNT[this->protocol_version_]); + this->read_input_registers(0, MODBUS_REGISTER_COUNT[this->protocol_version_]); this->last_send_ = millis(); } diff --git a/esphome/components/havells_solar/havells_solar.cpp b/esphome/components/havells_solar/havells_solar.cpp index 45e57544db..6af72c352b 100644 --- a/esphome/components/havells_solar/havells_solar.cpp +++ b/esphome/components/havells_solar/havells_solar.cpp @@ -7,7 +7,6 @@ namespace esphome::havells_solar { static const char *const TAG = "havells_solar"; -static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x03; static const uint8_t MODBUS_REGISTER_COUNT = 48; // 48 x 16-bit registers void HavellsSolar::on_response(std::span request_pdu, std::span response_pdu) { @@ -122,7 +121,7 @@ void HavellsSolar::on_response(std::span request_pdu, std::spandci_of_t_sensor_->publish_state(dci_of_t); } -void HavellsSolar::update() { this->send(MODBUS_CMD_READ_IN_REGISTERS, 0, MODBUS_REGISTER_COUNT); } +void HavellsSolar::update() { this->read_holding_registers(0, MODBUS_REGISTER_COUNT); } void HavellsSolar::dump_config() { ESP_LOGCONFIG(TAG, "HAVELLS Solar:\n" diff --git a/esphome/components/kuntze/kuntze.cpp b/esphome/components/kuntze/kuntze.cpp index 1475ca61ae..c47a80777c 100644 --- a/esphome/components/kuntze/kuntze.cpp +++ b/esphome/components/kuntze/kuntze.cpp @@ -7,7 +7,6 @@ namespace esphome::kuntze { static const char *const TAG = "kuntze"; -static const uint8_t CMD_READ_REG = 0x03; static const uint16_t REGISTER[] = {4136, 4160, 4680, 6000, 4688, 4728, 5832}; // Maximum bytes to log for Modbus responses (2 registers = 4, plus count = 5) @@ -77,7 +76,7 @@ void Kuntze::loop() { if (this->waiting_ || (this->state_ == 0)) return; this->last_send_ = now; - send(CMD_READ_REG, REGISTER[this->state_ - 1], 2); + this->read_holding_registers(REGISTER[this->state_ - 1], 2); this->waiting_ = true; } diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index 3f8aef433f..a507e7e513 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -718,4 +718,25 @@ void Modbus::clear_rx_buffer_(const LogString *reason, bool warn, size_t bytes_t } } +void ModbusClientDevice::read_entities(EntityType entity_type, uint16_t start_address, uint16_t number_of_entities) { + switch (entity_type) { + case EntityType::HOLDING: + this->read_holding_registers(start_address, number_of_entities); + return; + case EntityType::INPUT_REGISTER: + this->read_input_registers(start_address, number_of_entities); + return; + case EntityType::COIL: + this->read_coils(start_address, number_of_entities); + return; + case EntityType::DISCRETE_INPUT: + this->read_discrete_inputs(start_address, number_of_entities); + return; + default: + ESP_LOGW(TAG, "Invalid entity type for read_entities: %d", (int) entity_type); + this->on_not_sent(); // every rejected send is signalled, like send_pdu()'s own refusals + return; + } +} + } // namespace esphome::modbus diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index 2e2c027dd0..6afb8584aa 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -206,9 +206,8 @@ class ModbusClientDevice { this->on_modbus_not_sent(); #pragma GCC diagnostic pop } - /// Called when no (valid) response arrived; return true to have the hub re-queue the frame for a retry. - /// The hub does not bound retries: the device is responsible for limiting them (e.g. track a counter and - /// return false when exhausted), or an unresponsive peer will starve other traffic on the bus. + /// Called when no matching, uninterrupted response arrived; return true to have the hub re-queue the frame for a + /// retry. The hub does not bound retries: the device is responsible for limiting them. virtual bool on_no_response() { #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -221,6 +220,7 @@ class ModbusClientDevice { // Remove before 2027.2.0 ESPDEPRECATED("Override on_no_response() instead. Removed in 2027.2.0", "2026.8.0") virtual bool on_modbus_no_response() { return false; } + ESPDEPRECATED("Use the typed read_*/write_* helpers or send_pdu() instead. Removed in 2027.2.0", "2026.8.0") void send(uint8_t function, uint16_t start_address, uint16_t number_of_entities, uint8_t payload_len = 0, const uint8_t *payload = nullptr) { this->parent_->send_pdu( @@ -237,6 +237,39 @@ class ModbusClientDevice { } this->parent_->send_pdu(payload[0], std::span(payload).subspan(1), this); } + // Dispatches to the matching read_* method; defined in modbus.cpp because it logs on an invalid type. + void read_entities(EntityType entity_type, uint16_t start_address, uint16_t number_of_entities); + void read_input_registers(uint16_t start_address, uint16_t number_of_registers) { + this->send_pdu(helpers::create_read_pdu(FunctionCode::READ_INPUT_REGISTERS, start_address, number_of_registers)); + } + void read_holding_registers(uint16_t start_address, uint16_t number_of_registers) { + this->send_pdu(helpers::create_read_pdu(FunctionCode::READ_HOLDING_REGISTERS, start_address, number_of_registers)); + } + void read_coils(uint16_t start_address, uint16_t number_of_coils) { + this->send_pdu(helpers::create_read_pdu(FunctionCode::READ_COILS, start_address, number_of_coils)); + } + void read_discrete_inputs(uint16_t start_address, uint16_t number_of_inputs) { + this->send_pdu(helpers::create_read_pdu(FunctionCode::READ_DISCRETE_INPUTS, start_address, number_of_inputs)); + } + void write_single_register(uint16_t start_address, uint16_t value) { + this->send_pdu(helpers::create_write_single_register_pdu(start_address, value)); + } + void write_single_coil(uint16_t address, bool value) { + this->send_pdu(helpers::create_write_single_coil_pdu(address, value)); + } + void write_multiple_registers(uint16_t start_address, std::span values) { + this->send_pdu(helpers::create_write_registers_pdu(start_address, values)); + } + /// Note: std::vector cannot bind to std::span; use a contiguous bool container or the packed + /// overload. + void write_multiple_coils(uint16_t start_address, std::span values) { + this->send_pdu(helpers::create_write_coils_pdu(start_address, values)); + } + /// Packed variant: a PackedBits view (the same layout on_read_coils() delivers), so + /// read-modify-write needs no unpack/repack. + void write_multiple_coils(uint16_t start_address, PackedBits bits) { + this->send_pdu(helpers::create_write_coils_pdu(start_address, bits)); + } inline void clear_tx_queue_for_address(bool clear_sent = true) { this->parent_->clear_tx_queue_for_address(this->address_, clear_sent); } diff --git a/esphome/components/modbus_controller/modbus_controller.cpp b/esphome/components/modbus_controller/modbus_controller.cpp index 8a81acab3a..8822b7b40a 100644 --- a/esphome/components/modbus_controller/modbus_controller.cpp +++ b/esphome/components/modbus_controller/modbus_controller.cpp @@ -523,8 +523,9 @@ ModbusCommandItem ModbusCommandItem::create_custom_command( bool ModbusCommandItem::send() { if (this->function_code != FunctionCode::CUSTOM) { - modbusdevice->send(uint8_t(this->function_code), this->register_address, this->register_count, this->payload.size(), - this->payload.empty() ? nullptr : &this->payload[0]); + modbusdevice->send_pdu( + modbus::helpers::create_client_pdu(this->function_code, this->register_address, this->register_count, + this->payload.empty() ? nullptr : &this->payload[0], this->payload.size())); } else { modbusdevice->send_raw(this->payload); } diff --git a/esphome/components/pzemac/pzemac.cpp b/esphome/components/pzemac/pzemac.cpp index 233ad6fc53..5651e07af0 100644 --- a/esphome/components/pzemac/pzemac.cpp +++ b/esphome/components/pzemac/pzemac.cpp @@ -5,7 +5,6 @@ namespace esphome::pzemac { static const char *const TAG = "pzemac"; -static const uint8_t PZEM_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t PZEM_CMD_RESET_ENERGY = 0x42; static const uint8_t PZEM_REGISTER_COUNT = 10; // 10x 16-bit registers @@ -62,7 +61,7 @@ void PZEMAC::on_response(std::span request_pdu, std::spanpower_factor_sensor_->publish_state(power_factor); } -void PZEMAC::update() { this->send(PZEM_CMD_READ_IN_REGISTERS, 0, PZEM_REGISTER_COUNT); } +void PZEMAC::update() { this->read_input_registers(0, PZEM_REGISTER_COUNT); } void PZEMAC::dump_config() { ESP_LOGCONFIG(TAG, "PZEMAC:\n" diff --git a/esphome/components/pzemdc/pzemdc.cpp b/esphome/components/pzemdc/pzemdc.cpp index 9de72d51f9..5e505cde0c 100644 --- a/esphome/components/pzemdc/pzemdc.cpp +++ b/esphome/components/pzemdc/pzemdc.cpp @@ -5,7 +5,6 @@ namespace esphome::pzemdc { static const char *const TAG = "pzemdc"; -static const uint8_t PZEM_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t PZEM_CMD_RESET_ENERGY = 0x42; static const uint8_t PZEM_REGISTER_COUNT = 10; // 10x 16-bit registers @@ -52,7 +51,7 @@ void PZEMDC::on_response(std::span request_pdu, std::spanenergy_sensor_->publish_state(energy); } -void PZEMDC::update() { this->send(PZEM_CMD_READ_IN_REGISTERS, 0, 8); } +void PZEMDC::update() { this->read_input_registers(0, 8); } void PZEMDC::dump_config() { ESP_LOGCONFIG(TAG, "PZEMDC:\n" diff --git a/esphome/components/sdm_meter/sdm_meter.cpp b/esphome/components/sdm_meter/sdm_meter.cpp index 989f22dd2a..1ebc7fa3d8 100644 --- a/esphome/components/sdm_meter/sdm_meter.cpp +++ b/esphome/components/sdm_meter/sdm_meter.cpp @@ -7,7 +7,6 @@ namespace esphome::sdm_meter { static const char *const TAG = "sdm_meter"; -static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t MODBUS_REGISTER_COUNT = 80; // 74 x 16-bit registers void SDMMeter::on_response(std::span request_pdu, std::span response_pdu) { @@ -83,7 +82,7 @@ void SDMMeter::on_response(std::span request_pdu, std::spanexport_reactive_energy_sensor_->publish_state(export_reactive_energy); } -void SDMMeter::update() { this->send(MODBUS_CMD_READ_IN_REGISTERS, 0, MODBUS_REGISTER_COUNT); } +void SDMMeter::update() { this->read_input_registers(0, MODBUS_REGISTER_COUNT); } void SDMMeter::dump_config() { ESP_LOGCONFIG(TAG, "SDM Meter:\n" diff --git a/esphome/components/selec_meter/selec_meter.cpp b/esphome/components/selec_meter/selec_meter.cpp index f5f0fdf40d..688923d8e6 100644 --- a/esphome/components/selec_meter/selec_meter.cpp +++ b/esphome/components/selec_meter/selec_meter.cpp @@ -7,7 +7,6 @@ namespace esphome::selec_meter { static const char *const TAG = "selec_meter"; -static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t MODBUS_REGISTER_COUNT = 34; // 34 x 16-bit registers void SelecMeter::on_response(std::span request_pdu, std::span response_pdu) { @@ -82,7 +81,7 @@ void SelecMeter::on_response(std::span request_pdu, std::spanmaximum_demand_apparent_power_sensor_->publish_state(maximum_demand_apparent_power); } -void SelecMeter::update() { this->send(MODBUS_CMD_READ_IN_REGISTERS, 0, MODBUS_REGISTER_COUNT); } +void SelecMeter::update() { this->read_input_registers(0, MODBUS_REGISTER_COUNT); } void SelecMeter::dump_config() { ESP_LOGCONFIG(TAG, "SELEC Meter:\n" diff --git a/tests/components/modbus/modbus_client_hub_test.cpp b/tests/components/modbus/modbus_client_hub_test.cpp index 7bdc4ac35b..6335a75775 100644 --- a/tests/components/modbus/modbus_client_hub_test.cpp +++ b/tests/components/modbus/modbus_client_hub_test.cpp @@ -272,4 +272,81 @@ TEST(ModbusDeviceShim, LegacyCallbacksReceiveTheOldShapes) { EXPECT_EQ(device.last_error_code_, 0x02); } +// --- typed send helpers -------------------------------------------------------------------------- +// Each helper is a one-line forward onto a merged builder; these pin the function code and wire +// bytes each one queues, so a swapped code or transposed field cannot survive review silently. +TEST(ModbusTypedSendHelpers, HelpersQueueExpectedPdus) { + NoResponseProbeHub hub; + ModbusClientDevice device(&hub, 0x02); + auto check = [&](const std::vector &expected) { + ASSERT_EQ(hub.queued_frames(), 1u); + auto pdu = hub.front().frame.pdu(); + EXPECT_EQ(std::vector(pdu.begin(), pdu.end()), expected); + hub.force_send_front(); + hub.timeout_waiting(); // default on_no_response() declines the retry, dropping the frame + }; + + device.read_holding_registers(0x0102, 3); + check({0x03, 0x01, 0x02, 0x00, 0x03}); + device.read_input_registers(0x0010, 2); + check({0x04, 0x00, 0x10, 0x00, 0x02}); + device.read_coils(0x0020, 10); + check({0x01, 0x00, 0x20, 0x00, 0x0A}); + device.read_discrete_inputs(0x0030, 1); + check({0x02, 0x00, 0x30, 0x00, 0x01}); + device.write_single_register(0x0040, 0xABCD); + check({0x06, 0x00, 0x40, 0xAB, 0xCD}); + device.write_single_coil(0x0041, true); + check({0x05, 0x00, 0x41, 0xFF, 0x00}); + device.write_single_coil(0x0041, false); + check({0x05, 0x00, 0x41, 0x00, 0x00}); + const uint16_t regs[] = {0x000B, 0x0016}; + device.write_multiple_registers(0x0050, regs); + check({0x10, 0x00, 0x50, 0x00, 0x02, 0x04, 0x00, 0x0B, 0x00, 0x16}); + const bool coils[] = {true, false, true}; + device.write_multiple_coils(0x0060, coils); + check({0x0F, 0x00, 0x60, 0x00, 0x03, 0x01, 0x05}); + const uint8_t packed[] = {0x05}; + device.write_multiple_coils(0x0060, PackedBits(packed, 3)); // packed overload, same wire bytes + check({0x0F, 0x00, 0x60, 0x00, 0x03, 0x01, 0x05}); +} + +TEST(ModbusTypedSendHelpers, ReadEntitiesDispatchesByTypeAndRejectsInvalid) { + NoResponseProbeHub hub; + ModbusClientDevice device(&hub, 0x02); + + device.read_entities(EntityType::HOLDING, 0x0001, 1); + ASSERT_EQ(hub.queued_frames(), 1u); + EXPECT_EQ(hub.front().frame.pdu()[0], 0x03); + hub.force_send_front(); + hub.timeout_waiting(); + + device.read_entities(EntityType::DISCRETE_INPUT, 0x0001, 1); + ASSERT_EQ(hub.queued_frames(), 1u); + EXPECT_EQ(hub.front().frame.pdu()[0], 0x02); + hub.force_send_front(); + hub.timeout_waiting(); + + device.read_entities(EntityType::CUSTOM, 0x0001, 1); // no read function: logged and not queued + EXPECT_EQ(hub.queued_frames(), 0u); +} + +// A rejected read_entities() signals on_not_sent() like every other refused send. +namespace { +class NotSentCountingDevice : public ModbusClientDevice { + public: + NotSentCountingDevice(ModbusClientHub *hub, uint8_t address) : ModbusClientDevice(hub, address) {} + void on_not_sent() override { this->not_sent_++; } + int not_sent_{0}; +}; +} // namespace + +TEST(ModbusTypedSendHelpers, InvalidReadEntitiesSignalsNotSent) { + NoResponseProbeHub hub; + NotSentCountingDevice device(&hub, 0x02); + device.read_entities(EntityType::CUSTOM, 0x0001, 1); + EXPECT_EQ(device.not_sent_, 1); + EXPECT_EQ(hub.queued_frames(), 0u); +} + } // namespace esphome::modbus::testing