diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index cff086aeeac..5305f6313f1 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -815,6 +815,16 @@ void ModbusClientHub::send_next_frame_() { } cmd->sent(); + if (cmd->frame.address() == BROADCAST_ADDRESS) { + // A broadcast (address 0) is never answered (Modbus 4.1), so it is fire-and-forget: on_sent above + // reports the transmission, and the entry then retires with no terminal callback instead of + // occupying the waiting slot until the send-wait timeout expires. The turnaround delay already + // spaces the next frame; the following sweep erases the entry. + ESP_LOGV(TAG, "Broadcast to address 0 sent; no reply expected (fire-and-forget)"); + cmd->complete_broadcast(); + this->sweep_needed_ = true; + return; + } this->waiting_for_response_ = true; } @@ -1033,9 +1043,24 @@ bool ModbusClientHub::queue_pdu(uint8_t address, std::span pdu, M ESP_LOGE(TAG, "Frame too large, refused: %" PRIu8 ":%zu bytes", address, pdu.size()); return false; } + // classify() drives both the broadcast guard and the continuous check below; compute it once. + const CommandPriority priority = ModbusDeviceCommand::classify(pdu[0]); + + // A broadcast (address 0) is never answered (Modbus 4.1), so it is only meaningful for a command that + // changes state. Refuse a broadcast that expects a reply - anything but a write or a custom/vendor code - + // as it could never deliver a result, so the caller learns via the false return (and on_not_sent). + // 0x17 (read/write multiple) is a knowing inclusion: classify() treats it as a write, so its write half + // lands on every server and its unanswerable read half is simply discarded. An exception-flagged custom + // code (0x80 bit set) is refused: is_function_code_custom() masks that bit away, so exclude it explicitly + // here to match classify()'s exception-first handling of the write side. + if (address == BROADCAST_ADDRESS && priority != CommandPriority::WRITE && + (!helpers::is_function_code_custom(pdu[0]) || helpers::is_function_code_exception(pdu[0]))) { + ESP_LOGW(TAG, "Broadcast refused for function 0x%X: a broadcast (address 0) is never answered", pdu[0]); + return false; + } // continuous is ignored for every mutating code (re-writing a value forever is never intended). - const bool mutates = ModbusDeviceCommand::classify(pdu[0]) == CommandPriority::WRITE; + const bool mutates = priority == CommandPriority::WRITE; bool continuous = false; if (options.continuous) { if (mutates) { diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index 6bd407a687e..dfe4a4872de 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -171,6 +171,15 @@ struct ModbusDeviceCommand { this->pending = 0; this->device = nullptr; } + // Fire-and-forget completion for a broadcast (address 0): the frame was transmitted (on_sent already + // fired), but a broadcast is never answered (Modbus 4.1), so the entry retires with NO terminal + // callback and the sweep erases it. Unlike response()/error()/timed_out(), it delivers nothing. + // A broadcast only carries a write or a custom code (reads are refused at queue_pdu()), and every such + // code caps pending at 1, so pending is always 1 here - clear it. + void complete_broadcast() { + this->state = FrameState::RETIRED; + this->pending = 0; + } // Re-ready for another transmission, restamped to the tail of its class (hub passes next_seq_++). void requeue(uint16_t seq) { this->state = FrameState::READY; @@ -270,7 +279,8 @@ class ModbusClientHub : public Modbus { }; /// Queue a request. The name says queue, not send: the frame is appended to the transmit queue and /// goes out later from loop(), so a true return means accepted into the machine (it will resolve in - /// exactly one terminal callback), NOT that anything reached the wire - that is on_sent(). False means + /// exactly one terminal callback - except a broadcast (address 0), which is never answered and so gets + /// only on_sent()), NOT that anything reached the wire - that is on_sent(). False means /// it never entered the machine at all (empty or oversize PDU, full queue, anonymous or over-cap /// duplicate) and no callback of any kind will follow; the false return is the whole story. bool queue_pdu(uint8_t address, std::span pdu, ModbusClientDevice *device = nullptr, @@ -411,13 +421,14 @@ class ModbusServerHub : public Modbus { /// Callback contract. Each accepted request ends in exactly ONE terminal: on_response() (data), /// on_error() (exception), on_no_response() (timeout/interruption), or on_not_sent() (dropped by /// clear_tx_queue_for_address before transmission). A request refused at queue_pdu() (false return) -/// gets none. on_sent() is additional, once per transmission, never for an on_not_sent() request. -/// on_response()/on_error() fire at parse time and on_no_response() at the send-wait watchdog, all -/// from a quiescent hub; only on_not_sent() is delivered by the sweep. Sending or clearing from +/// gets none, and a broadcast (address 0) gets on_sent() with NO terminal, since a broadcast is never +/// answered (Modbus 4.1). on_sent() is additional, once per transmission, never for an on_not_sent() +/// request. on_response()/on_error() fire at parse time and on_no_response() at the send-wait watchdog, +/// all from a quiescent hub; only on_not_sent() is delivered by the sweep. Sending or clearing from /// inside a callback is safe (picked up by the next sweep). Exceptions to "exactly one terminal": -/// clear_tx_queue_for_device() drops the caller's own frames silently; a continuous poll's cycles are -/// its own accounting (a one-shot duplicate downgrades the poll to a one-shot; a continuous duplicate -/// merges into it). +/// a broadcast is fire-and-forget (on_sent, no terminal); clear_tx_queue_for_device() drops the caller's +/// own frames silently; a continuous poll's cycles are its own accounting (a one-shot duplicate +/// downgrades the poll to a one-shot; a continuous duplicate merges into it). /// /// Invariants: /// - Public entry points (queue_pdu/clear_tx_queue_*) only append to the queue or mutate an existing @@ -531,8 +542,9 @@ class ModbusClientDevice { this); } /// See ModbusClientHub::queue_pdu(): true = accepted into the queue and a terminal callback will - /// follow, false = refused at the door and nothing further happens. Neither means the frame is on - /// the wire; on_sent() reports that. + /// follow (except a broadcast (address 0), which is never answered and so gets only on_sent()), + /// false = refused at the door and nothing further happens. Neither means the frame is on the wire; + /// on_sent() reports that. bool queue_pdu(std::span pdu, CommandOptions options = {}) { return this->parent_->queue_pdu(this->address_, pdu, this, options); } @@ -548,8 +560,9 @@ class ModbusClientDevice { this->parent_->queue_pdu(payload[0], std::span(payload).subspan(1), this); } // The typed request builders below all queue through queue_pdu(), so they share its contract: true - // means the request is queued and will resolve in exactly one terminal callback, false means it was - // refused outright with no callback. Neither says the frame has been transmitted - on_sent() does. + // means the request is queued and will resolve in exactly one terminal callback (except a broadcast + // (address 0), which is never answered and so gets only on_sent()), false means it was refused outright + // with no callback. Neither says the frame has been transmitted - on_sent() does. // Reads via the table-appropriate function code; an unreadable entity type maps to INVALID, which // create_read_pdu() rejects into an empty PDU and queue_pdu() refuses with a false return. bool read_entities(EntityType entity_type, uint16_t start_address, uint16_t number_of_entities, diff --git a/esphome/components/modbus_client/modbus_client.h b/esphome/components/modbus_client/modbus_client.h index 7e6d9d069f3..20dc1a4745f 100644 --- a/esphome/components/modbus_client/modbus_client.h +++ b/esphome/components/modbus_client/modbus_client.h @@ -64,7 +64,8 @@ template class ClientActionBase : public Action, public m protected: /// The hub refuses some sends at the door with no callback (a duplicate write already pending, a full /// queue, or an empty PDU - which is how the create_*_pdu() builders reject out-of-spec input). Every - /// send still gets exactly one outcome, so resolve refusals here via on_not_sent. + /// send still gets exactly one outcome (a broadcast (address 0) is the exception - never answered, it + /// resolves through on_sent() alone), so resolve refusals here via on_not_sent. /// Takes a span, not a PduBuffer: the builders return right-sized buffers (a read PDU is 5 bytes), and /// a PduBuffer parameter would widen each one to the 253-byte maximum just to cross the call. void send_or_resolve_(std::span pdu) { diff --git a/esphome/components/modbus_controller/modbus_controller.cpp b/esphome/components/modbus_controller/modbus_controller.cpp index 35f21fd0af0..2c568938e4a 100644 --- a/esphome/components/modbus_controller/modbus_controller.cpp +++ b/esphome/components/modbus_controller/modbus_controller.cpp @@ -109,8 +109,22 @@ void ModbusCommandItem::on_not_sent(std::span request_pdu) { // Fired once per wire transmission (including hub re-queues from a retry), so the on_command_sent // trigger reflects when the frame actually went out, not when it was queued. void ModbusCommandItem::on_sent(std::span request_pdu) { - if (this->controller_ != nullptr) - this->controller_->command_sent(static_cast(this->function_code_), this->start_address_); + if (this->controller_ == nullptr) + return; + this->controller_->command_sent(static_cast(this->function_code_), this->start_address_); + // A broadcast (address 0) is never answered (Modbus 4.1), so the hub delivers no terminal callback. + // on_sent is this command's only callback, so drop the one-shot from the queue here, or it would leak. + // Test the address the frame went to, not address_: a custom command's frame carries its own address + // (frame[0]), which may differ from this controller's. (unqueue_command() is a no-op for a poll.) + uint8_t wire_address = this->address_; + if (this->function_code_ == FunctionCode::CUSTOM) { + std::span frame = + this->custom_data_ != nullptr ? std::span(*this->custom_data_) : this->payload; + if (!frame.empty()) + wire_address = frame[0]; + } + if (wire_address == modbus::BROADCAST_ADDRESS) + this->controller_->unqueue_command(this); } bool ModbusCommandItem::on_no_response(std::span request_pdu) { diff --git a/tests/components/modbus/modbus_client_hub_test.cpp b/tests/components/modbus/modbus_client_hub_test.cpp index c2a36c0da7b..43c81bbf344 100644 --- a/tests/components/modbus/modbus_client_hub_test.cpp +++ b/tests/components/modbus/modbus_client_hub_test.cpp @@ -684,6 +684,155 @@ TEST(ModbusClientHubSent, FiresOnWireNotOnQueue) { EXPECT_TRUE(hub.waiting()); } +namespace { +// Records on_sent / on_response / on_no_response so a broadcast's fire-and-forget completion +// (on_sent, and no terminal) can be asserted. +class BroadcastProbeDevice : public ModbusClientDevice { + public: + BroadcastProbeDevice(ModbusClientHub *hub, uint8_t address) : ModbusClientDevice(hub, address) {} + void on_sent(std::span request_pdu) override { this->sent_count_++; } + void on_response(std::span request_pdu, std::span response_pdu) override { + this->response_count_++; + this->last_response_size_ = response_pdu.size(); + } + bool on_no_response(std::span request_pdu) override { + this->no_response_count_++; + return false; + } + int sent_count_{0}; + int response_count_{0}; + int no_response_count_{0}; + size_t last_response_size_{0}; +}; +} // namespace + +// A broadcast (address 0) is never answered (Modbus 4.1), so the client treats it as fire-and-forget: +// on_sent fires as the frame goes out, NO terminal (on_response/on_error/on_no_response) is delivered, +// the hub is left NOT waiting - no timeout is burned - and the sweep erases the entry. +TEST(ModbusClientHubBroadcast, CompletesAtTransmissionWithoutWaiting) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t write[] = {0x06, 0x00, 0x10, 0x00, 0x01}; // write single register 0x0010 = 0x0001 + ASSERT_TRUE(device.queue_pdu(write)); + EXPECT_EQ(hub.queued_frames(), 1u); + + hub.send_next_for_test(); // transmit + sweep + + EXPECT_EQ(device.sent_count_, 1); // the frame went on the wire + EXPECT_EQ(device.response_count_, 0); // fire-and-forget: no terminal callback + EXPECT_EQ(device.no_response_count_, 0); // and it never waited for a reply + EXPECT_FALSE(hub.waiting()); // no waiting slot occupied + EXPECT_EQ(hub.queued_frames(), 0u); // and the entry is gone + EXPECT_EQ(hub.entries(), 0u); +} + +namespace { +// Keeps the DEFAULT on_response() (so the base typed dispatcher runs) and records the typed write +// callback and the catch-all, to prove a broadcast reaches neither - only on_sent. +class BroadcastTypedProbeDevice : public ModbusClientDevice { + public: + BroadcastTypedProbeDevice(ModbusClientHub *hub, uint8_t address) : ModbusClientDevice(hub, address) {} + void on_sent(std::span request_pdu) override { this->sent_count_++; } + void on_write_single_register(uint16_t address, uint16_t value, ResponseStatus status) override { + this->write_single_count_++; + } + void on_custom_response(std::span request_pdu, std::span response_pdu, + ResponseStatus status) override { + this->custom_count_++; + } + int sent_count_{0}; + int write_single_count_{0}; + int custom_count_{0}; +}; +} // namespace + +// Completing a broadcast with an empty response({}) used to fall, for a device on the default +// on_response(), through the typed dispatcher to on_custom_response() - firing the wrong callback and +// logging a spurious "non-standard" warning. Fire-and-forget delivers no terminal at all, so a broadcast +// write reaches neither the typed write callback nor the catch-all: only on_sent. +TEST(ModbusClientHubBroadcast, DeliversNoTerminalToTypedDevice) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastTypedProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t write[] = {0x06, 0x00, 0x10, 0x00, 0x01}; // write single register 0x0010 = 0x0001 + ASSERT_TRUE(device.queue_pdu(write)); + + hub.send_next_for_test(); // transmit + sweep + + EXPECT_EQ(device.sent_count_, 1); // on_sent still reports the transmission + EXPECT_EQ(device.write_single_count_, 0); // no terminal: the typed write callback never fires + EXPECT_EQ(device.custom_count_, 0); // and it is NOT diverted to the catch-all (no false warning) + EXPECT_FALSE(hub.waiting()); + EXPECT_EQ(hub.entries(), 0u); +} + +// A broadcast is only meaningful for a command that changes state; a broadcast READ could never be +// answered, so the hub refuses it at the door (false return, no entry queued) rather than silently +// retiring it. Writes, 0x17, and custom codes still go through (covered above). +TEST(ModbusClientHubBroadcast, RefusesReadBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t read[] = {0x03, 0x00, 0x10, 0x00, 0x02}; // read holding registers 0x0010, count 2 + EXPECT_FALSE(device.queue_pdu(read)); // refused: a broadcast read is never answered + EXPECT_EQ(hub.entries(), 0u); // nothing entered the machine + EXPECT_FALSE(hub.waiting()); + + hub.send_next_for_test(); // nothing to send + EXPECT_EQ(device.sent_count_, 0); // never transmitted +} + +// The counterpart to RefusesReadBroadcast: a custom (user-defined) function code carries no reply the +// hub knows how to expect, so a broadcast of one is accepted and completes fire-and-forget like a write. +TEST(ModbusClientHubBroadcast, AcceptsCustomBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t custom[] = {0x41, 0x01, 0x02}; // FC 0x41: first user-defined function code space + ASSERT_TRUE(device.queue_pdu(custom)); // accepted: a custom code is not a read + EXPECT_EQ(hub.queued_frames(), 1u); + + hub.send_next_for_test(); // transmit + sweep + + EXPECT_EQ(device.sent_count_, 1); // the frame went on the wire + EXPECT_EQ(device.response_count_, 0); // fire-and-forget: no terminal callback + EXPECT_EQ(device.no_response_count_, 0); // and it never waited for a reply + EXPECT_FALSE(hub.waiting()); + EXPECT_EQ(hub.entries(), 0u); // the entry is gone +} + +// An exception-flagged custom code (0x80 bit set) is not a real request: is_function_code_custom() masks +// the bit away and would accept it, but the broadcast guard excludes it, matching classify()'s handling +// of an exception-flagged write. +TEST(ModbusClientHubBroadcast, RefusesExceptionFlaggedCustomBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t exception_custom[] = {0xC1, 0x01, 0x02}; // 0x41 | 0x80: custom code with the exception bit + EXPECT_FALSE(device.queue_pdu(exception_custom)); // refused: exception-flagged, never a real broadcast + EXPECT_EQ(hub.entries(), 0u); // nothing entered the machine + EXPECT_FALSE(hub.waiting()); + + hub.send_next_for_test(); // nothing to send + EXPECT_EQ(device.sent_count_, 0); // never transmitted +} + namespace { // tx_blocked() clear for send_next_frame_'s gate, then blocked for send_frame_'s post-delay re-check. class RejectPostDelayHub : public NoResponseProbeHub { diff --git a/tests/integration/fixtures/uart_mock_modbus_broadcast_write.yaml b/tests/integration/fixtures/uart_mock_modbus_broadcast_write.yaml new file mode 100644 index 00000000000..8857bf8c96d --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_broadcast_write.yaml @@ -0,0 +1,150 @@ +esphome: + name: uart-mock-modbus-broadcast + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +# Dummy uart entry to satisfy modbus's DEPENDENCIES = ["uart"] +# The actual UART bus used is the uart_mock component below +uart: + baud_rate: 115200 + port: /dev/null + +uart_mock: + - id: virtual_uart_server + baud_rate: 9600 + auto_start: true # controller polls at boot; forwarding must already be active + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_controller + data: !lambda return data; + - uart_mock.inject_rx: + id: virtual_uart_server_2 + data: !lambda return data; + - id: virtual_uart_server_2 + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_server + data: !lambda return data; + - uart_mock.inject_rx: + id: virtual_uart_controller + data: !lambda return data; + - id: virtual_uart_controller + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_server + data: !lambda return data; + - uart_mock.inject_rx: + id: virtual_uart_server_2 + data: !lambda return data; + +modbus: + - uart_id: virtual_uart_server + id: virtual_modbus_server + role: server + - uart_id: virtual_uart_server_2 + id: virtual_modbus_server_2 + role: server + - uart_id: virtual_uart_controller + id: virtual_modbus_client + role: client + turnaround_time: 10ms + +globals: + - id: srv1_reg + type: int + initial_value: "0" + - id: srv2_reg + type: int + initial_value: "0" + +modbus_controller: + - address: 1 + modbus_id: virtual_modbus_client + # Polling is off until the test has subscribed; the Start Scenario button starts it, so the + # first poll is never lost to a boot-time race ahead of the API subscription. + update_interval: never + id: modbus_controller_1 + +modbus_server: + - address: 1 + modbus_id: virtual_modbus_server + registers: + - address: 0x01 + value_type: U_WORD + read_lambda: return 919; + - address: 0x10 + value_type: U_WORD + read_lambda: return id(srv1_reg); + write_lambda: |- + id(srv1_reg) = x; + return true; + - address: 2 + modbus_id: virtual_modbus_server_2 + registers: + - address: 0x10 + value_type: U_WORD + read_lambda: return id(srv2_reg); + write_lambda: |- + id(srv2_reg) = x; + return true; + +sensor: + # Normal polling continues before and after the broadcast: the old behavior burned a + # timeout per broadcast, which surfaces as modbus warnings and failed expectations here. + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "reg_u_word" + address: 0x01 + register_type: holding + value_type: U_WORD + # Republish every poll (the value is constant 919): the test observes successive publishes to + # prove polling continues before and after the broadcast, which dedup would otherwise hide. + force_update: true + # The servers' written values, published locally. + - platform: template + name: "srv1_written" + lambda: return id(srv1_reg); + update_interval: 0.2s + - platform: template + name: "srv2_written" + lambda: return id(srv2_reg); + update_interval: 0.2s + # Whether the hub accepted the broadcast into the transmit queue (the bool queue_pdu() returns). + - platform: template + name: "broadcast_accepted" + id: broadcast_accepted + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + on_press: + - lambda: |- + // Start polling now that the test has subscribed. + id(modbus_controller_1).set_update_interval(1000); + id(modbus_controller_1).start_poller(); + // Broadcast (address 0) write single register: reg 0x10 = 777 on every server. + // PDU is function code + data (no address/CRC); the hub prepends address 0 and appends CRC. + const uint8_t pdu[] = {0x06, 0x00, 0x10, 0x03, 0x09}; + // queue_pdu() returns whether the broadcast was accepted into the machine (the answer this PR + // makes meaningful); publish it so the test asserts the accept, not just the servers' writes. + bool accepted = id(virtual_modbus_client)->queue_pdu(0x00, pdu); + id(broadcast_accepted).publish_state(accepted ? 1.0f : 0.0f); diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 057d419fd85..d0b375dd252 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -836,6 +836,44 @@ async def test_uart_mock_modbus_fairness( ) +@pytest.mark.asyncio +async def test_uart_mock_modbus_broadcast_write( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """A client broadcast write (address 0) reaches every server and costs no timeout. + + The scenario button sends a broadcast single-register write of 777 to register + 0x10; both servers must apply it. The client's normal polling sensor must keep + updating, and no modbus warnings may appear - the pre-broadcast-support behavior + parked the frame in the waiting slot until the send-wait timeout, which surfaced + here as 'Stop waiting for response' warnings and a stalled poll. + """ + line_callback, error_log_lines, warning_log_lines = _make_modbus_line_callback() + + tracker = SensorTracker( + ["reg_u_word", "srv1_written", "srv2_written", "broadcast_accepted"] + ) + poll_before = tracker.expect("reg_u_word", 919) + written = tracker.expect_all({"srv1_written": 777, "srv2_written": 777}) + # queue_pdu() must accept the broadcast into the machine (return true), the answer this PR adds. + accepted = tracker.expect("broadcast_accepted", 1) + + async with ( + run_compiled(yaml_config, line_callback=line_callback), + api_client_connected() as client, + ): + await tracker.setup_and_start_scenario(client) + await tracker.await_change(accepted, "broadcast_accepted") + await tracker.await_change(poll_before, "reg_u_word") + await tracker.await_all(written) + # Polling must continue after the broadcast (a burned timeout stalls it). + poll_after = tracker.expect("reg_u_word", 919) + await tracker.await_change(poll_after, "reg_u_word", timeout=3.0) + _assert_no_modbus_errors(error_log_lines, warning_log_lines) + + @pytest.mark.asyncio async def test_uart_mock_modbus_client_read_write( yaml_config: str,