diff --git a/esphome/components/modbus_controller/__init__.py b/esphome/components/modbus_controller/__init__.py index 35a5479ecb..ea01331be3 100644 --- a/esphome/components/modbus_controller/__init__.py +++ b/esphome/components/modbus_controller/__init__.py @@ -170,10 +170,7 @@ async def add_modbus_base_properties( [ (sensor_type.operator("ptr"), "item"), (lambda_param_type, "x"), - ( - cg.std_vector.template(cg.uint8).operator("const").operator("ref"), - "data", - ), + (cg.std_span.template(cg.uint8.operator("const")), "data"), ], return_type=cg.optional.template(lambda_return_type), ) diff --git a/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.cpp b/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.cpp index d3caaaa3d9..b0c927cf84 100644 --- a/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.cpp +++ b/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.cpp @@ -7,17 +7,18 @@ static const char *const TAG = "modbus_controller.binary_sensor"; void ModbusBinarySensor::dump_config() { LOG_BINARY_SENSOR("", "Modbus Controller Binary Sensor", this); } -void ModbusBinarySensor::parse_and_publish(const std::vector &data) { +void ModbusBinarySensor::parse_and_publish(std::span data) { bool value; + // For coils/discrete inputs this is the bit index; for registers it is the byte offset. + const size_t offset = this->offset; switch (this->register_type) { case modbus::EntityType::DISCRETE_INPUT: case modbus::EntityType::COIL: - // offset for coil is the actual number of the coil not the byte offset - value = modbus::helpers::bit_from_packed(this->offset, data); + value = modbus::helpers::bit_from_packed(offset, data); break; default: - value = modbus::helpers::get_data(data, this->offset) & this->bitmask; + value = modbus::helpers::get_data(data.data(), offset) & this->bitmask; break; } // Is there a lambda registered diff --git a/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.h b/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.h index f56a32a5ec..902a3ba8dd 100644 --- a/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.h +++ b/esphome/components/modbus_controller/binary_sensor/modbus_binarysensor.h @@ -13,8 +13,8 @@ class ModbusBinarySensor final : public Component, public binary_sensor::BinaryS ModbusBinarySensor(modbus::EntityType register_type, uint16_t start_address, uint8_t offset, uint32_t bitmask, uint16_t skip_updates, bool force_new_range) { this->register_type = register_type; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->bitmask = bitmask; this->sensor_value_type = SensorValueType::BIT; this->skip_updates = skip_updates; @@ -27,12 +27,12 @@ class ModbusBinarySensor final : public Component, public binary_sensor::BinaryS } } - void parse_and_publish(const std::vector &data) override; + void parse_and_publish(std::span data) override; void set_state(bool state) { this->state = state; } void dump_config() override; - using transform_func_t = optional (*)(ModbusBinarySensor *, bool, const std::vector &); + using transform_func_t = optional (*)(ModbusBinarySensor *, bool, std::span); void set_template(transform_func_t f) { this->transform_func_ = f; } protected: diff --git a/esphome/components/modbus_controller/modbus_controller.cpp b/esphome/components/modbus_controller/modbus_controller.cpp index 8822b7b40a..15f36ce89b 100644 --- a/esphome/components/modbus_controller/modbus_controller.cpp +++ b/esphome/components/modbus_controller/modbus_controller.cpp @@ -134,7 +134,7 @@ void ModbusController::on_register_data(modbus::EntityType register_type, uint16 const std::vector &data) { ESP_LOGV(TAG, "data for register address : 0x%X : ", start_address); - // loop through all sensors with the same start address + // loop through all sensors in this range; each reads its own bytes from the position resolved for it. auto sensors = find_sensors_(register_type, start_address); for (auto *sensor : sensors) { sensor->parse_and_publish(data); @@ -211,100 +211,122 @@ size_t ModbusController::create_register_ranges_() { return 0; } - // iterator is sorted see SensorItemsComparator for details - auto ix = this->sensorset_.begin(); + // Sensors are walked in the sensor set's order (see SensorItemsComparator): register type, then + // force_new_range ahead of the rest, then address - so the walk is not purely address-ordered. + // Each keeps the address it was configured with; what is resolved here is its `offset`, the position + // of its data within the response of whichever range it ends up in. RegisterRange r = {}; - uint8_t buffer_offset = 0; + bool have_range = false; + // Set while the open range belongs to a force_new_range sensor: a range the user asked to keep + // separate must not quietly absorb other sensors. + bool range_forced = false; + // Set once a sensor has joined by sharing the range's start address, which widens the read. Only a + // widened range can absorb a later sensor by coverage: ranges that were kept apart before stay apart, + // so their frames and polling rates are untouched. + bool range_shared = false; + // Bytes the range's registers have consumed so far. An extending sensor starts after them, so a + // register that returns more bytes than its count implies pushes the sensors after it along. + // range_custom_size records whether any of them returns something other than two bytes per register, + // which is what makes a position inside the range impossible to work out from addresses alone. Coils + // count as such: they carry one bit per address, so bit ranges never take the coverage join. + size_t range_bytes = 0; + bool range_custom_size = false; SensorItem *prev = nullptr; - while (ix != this->sensorset_.end()) { - SensorItem *curr = *ix; + for (SensorItem *curr : this->sensorset_) { + ESP_LOGV(TAG, "Register: 0x%X count=%d size=%zu offset=%u skip=%u addr=%p", curr->start_address, + curr->register_count, curr->get_register_size(), curr->offset, curr->skip_updates, curr); - ESP_LOGV(TAG, "Register: 0x%X %d %d %zu offset=%u skip=%u addr=%p", curr->start_address, curr->register_count, - curr->offset, curr->get_register_size(), curr->offset, curr->skip_updates, curr); + const bool custom_size = curr->get_register_size() != static_cast(curr->register_count) * 2; - if (r.register_count == 0) { - // this is the first register in range + bool join = false; + if (have_range && !curr->force_new_range && r.register_type == curr->register_type && + curr->register_type != modbus::EntityType::CUSTOM) { + if (curr->start_address == (r.start_address + r.register_count - prev->register_count) && + prev->start_address + prev->register_count == r.start_address + r.register_count && + curr->register_count == prev->register_count && curr->get_register_size() == prev->get_register_size()) { + // A second sensor on the register(s) the previous one covers: it reads those same bytes, + // starting where that sensor's offset pointed, so a chain configured 0/2/4 resolves to 0/2/6. + // Both address tests matter. The first identifies the previous sensor's register by working back + // from the range's end, which only describes it while it actually sits there - hence the second. + // A sensor that joined mid-range must never anchor this, or the next one inherits its offset. + curr->offset = static_cast(prev->offset + curr->offset_from_start_address); + join = true; + ESP_LOGV(TAG, "Re-use previous register 0x%X", curr->start_address); + } else if (curr->start_address == (r.start_address + r.register_count)) { + // The next contiguous register(s): the data begins after what the range has consumed so far - + // the byte cursor for registers, the distance in bits for coils. + curr->offset = + static_cast((curr->addresses_bits() ? curr->start_address - r.start_address : range_bytes) + + curr->offset_from_start_address); + range_bytes += curr->get_register_size(); + range_custom_size = range_custom_size || custom_size; + r.register_count += curr->register_count; + join = true; + ESP_LOGV(TAG, "Extend range to include 0x%X", curr->start_address); + } else if (range_shared && !range_forced && curr->start_address >= r.start_address && + curr->start_address + curr->register_count <= r.start_address + r.register_count && + !range_custom_size && !custom_size && curr->skip_updates == r.skip_updates) { + // The registers already fall inside a range that a shared-address join widened, so this sensor + // reads its slice of that response instead of adding an overlapping second poll. The guards keep + // it narrow: only a widened range, never a force-isolated one; only where every register in the + // range returns two bytes, so interior positions follow from the addresses; only sensors genuinely + // inside it, which is why the lower bound is needed given the walk is not address-ordered; and + // only where the polling rates already match, since joining runs this sensor through the rate + // merge below and would otherwise change one of them. + const uint16_t addr_delta = curr->start_address - r.start_address; + curr->offset = static_cast((curr->addresses_bits() ? addr_delta : addr_delta * 2) + + curr->offset_from_start_address); + join = true; + ESP_LOGV(TAG, "Register 0x%X already covered by range 0x%X", curr->start_address, r.start_address); + } + } + + // Sensors on the same start address have to share one range: a response is dispatched to a single + // range per (start_address, register_type), so a second range with that key would never receive + // data. This holds for force_new_range and custom entities too. The read widens to cover whichever + // sensor needs the most registers, which also fixes a short read for coils that use offset. + if (!join && have_range && r.register_type == curr->register_type && r.start_address == curr->start_address) { + curr->offset = curr->offset_from_start_address; // shares the range start + r.register_count = std::max(r.register_count, curr->register_count); + range_bytes = std::max(range_bytes, curr->get_register_size()); + range_custom_size = range_custom_size || custom_size; + range_shared = true; + range_forced = range_forced || curr->force_new_range; + join = true; + ESP_LOGV(TAG, "Share range start 0x%X", curr->start_address); + } + + if (!join) { + if (have_range) { + ESP_LOGV(TAG, "Add range 0x%X %d skip:%d", r.start_address, r.register_count, r.skip_updates); + this->register_ranges_.push_back(std::move(r)); + } + r = {}; + range_bytes = curr->get_register_size(); + range_custom_size = custom_size; + range_forced = curr->force_new_range; + range_shared = false; + curr->offset = curr->offset_from_start_address; r.start_address = curr->start_address; r.register_count = curr->register_count; r.register_type = curr->register_type; - r.sensors.insert(curr); r.skip_updates = curr->skip_updates; r.skip_updates_counter = 0; - buffer_offset = curr->get_register_size(); - - ESP_LOGV(TAG, "Started new range"); - } else { - // this is not the first register in range so it might be possible - // to reuse the last register or extend the current range - if (!curr->force_new_range && r.register_type == curr->register_type && - curr->register_type != modbus::EntityType::CUSTOM) { - if (curr->start_address == (r.start_address + r.register_count - prev->register_count) && - curr->register_count == prev->register_count && curr->get_register_size() == prev->get_register_size()) { - // this register can re-use the data from the previous register - - // remove this sensore because start_address is changed (sort-order) - ix = this->sensorset_.erase(ix); - - curr->start_address = r.start_address; - curr->offset += prev->offset; - - this->sensorset_.insert(curr); - // move iterator backwards because it will be incremented later - ix--; - - ESP_LOGV(TAG, "Re-use previous register - change to register: 0x%X %d offset=%u", curr->start_address, - curr->register_count, curr->offset); - } else if (curr->start_address == (r.start_address + r.register_count)) { - // this register can extend the current range - - // remove this sensore because start_address is changed (sort-order) - ix = this->sensorset_.erase(ix); - - curr->start_address = r.start_address; - curr->offset += buffer_offset; - buffer_offset += curr->get_register_size(); - r.register_count += curr->register_count; - - this->sensorset_.insert(curr); - // move iterator backwards because it will be incremented later - ix--; - - ESP_LOGV(TAG, "Extend range - change to register: 0x%X %d offset=%u", curr->start_address, - curr->register_count, curr->offset); - } - } - } - - if (curr->start_address == r.start_address && curr->register_type == r.register_type) { - // use the lowest non zero value for the whole range - // Because zero is the default value for skip_updates it is excluded from getting the min value. - if (curr->skip_updates != 0) { - if (r.skip_updates != 0) { - r.skip_updates = std::min(r.skip_updates, curr->skip_updates); - } else { - r.skip_updates = curr->skip_updates; - } - } - - // add sensor to this range - r.sensors.insert(curr); - - ix++; - } else { - ESP_LOGV(TAG, "Add range 0x%X %d skip:%d", r.start_address, r.register_count, r.skip_updates); - this->register_ranges_.push_back(r); - r = {}; - buffer_offset = 0; - // do not increment the iterator here because the current sensor has to be re-evaluated + have_range = true; + } else if (curr->skip_updates != 0) { + // use the lowest non-zero skip_updates for the whole range (0 is the default and is excluded) + r.skip_updates = (r.skip_updates != 0) ? std::min(r.skip_updates, curr->skip_updates) : curr->skip_updates; } + // Every member records its range's first register. The resolved offset is relative to it, so the + // two together give the sensor's real position, and the address a write entity targets. + curr->range_start_address = r.start_address; + r.sensors.insert(curr); prev = curr; } - - if (r.register_count > 0) { - // Add the last range + if (have_range) { ESP_LOGV(TAG, "Add last range 0x%X %d skip:%d", r.start_address, r.register_count, r.skip_updates); - this->register_ranges_.push_back(r); + this->register_ranges_.push_back(std::move(r)); } return this->register_ranges_.size(); diff --git a/esphome/components/modbus_controller/modbus_controller.h b/esphome/components/modbus_controller/modbus_controller.h index 3c789936af..b5ef707a74 100644 --- a/esphome/components/modbus_controller/modbus_controller.h +++ b/esphome/components/modbus_controller/modbus_controller.h @@ -72,12 +72,32 @@ T get_data(const std::vector &data, size_t buffer_offset) { return modbus::helpers::get_data(data, buffer_offset); } +// Span overloads of the deprecated helpers below: read lambdas receive their payload as a +// std::span (previously a const std::vector &), and a span does not convert to +// a vector, so existing lambdas calling these by name need an overload that accepts one. These carry +// this release's deprecation window, since the span forms only exist from it. +// payload_to_number() deliberately has no such overload: one of its arguments is a modbus::helpers +// type, so a span call already reaches the helper by argument-dependent lookup, and a forwarder here +// would only make that call ambiguous. +// Remove before 2027.2.0. +template +ESPDEPRECATED("Use modbus::helpers::get_data() instead. Removed in 2027.2.0", "2026.8.0") +T get_data(std::span data, size_t buffer_offset) { + return modbus::helpers::get_data(data.data(), buffer_offset); +} + // Remove before 2027.2.0 (window restarted when the migration target changed to bit_from_packed()) ESPDEPRECATED("Use modbus::helpers::bit_from_packed() instead. Removed in 2027.2.0", "2026.4.0") inline bool coil_from_vector(int coil, const std::vector &data) { return modbus::helpers::bit_from_packed(coil, data); } +// Remove before 2027.2.0 +ESPDEPRECATED("Use modbus::helpers::bit_from_packed() instead. Removed in 2027.2.0", "2026.8.0") +inline bool coil_from_vector(int coil, std::span data) { + return modbus::helpers::bit_from_packed(coil, data); +} + template ESPDEPRECATED("Use modbus::helpers::mask_and_shift_by_rightbit() instead. Removed in 2026.10.0", "2026.4.0") N mask_and_shift_by_rightbit(N data, uint32_t mask) { @@ -107,11 +127,41 @@ class ModbusController; class SensorItem { public: - virtual void parse_and_publish(const std::vector &data) = 0; + /// Parse this sensor's slice out of its range's response and publish it. The span points into the + /// response buffer and is only valid for the duration of the call. Read the sensor's data from + /// `offset` within it. + virtual void parse_and_publish(std::span data) = 0; + + /// Coils and discrete inputs address individual bits; every other type addresses 16-bit registers. + bool addresses_bits() const { + return this->register_type == modbus::EntityType::COIL || this->register_type == modbus::EntityType::DISCRETE_INPUT; + } + + /// Address a write entity (switch/number/select) targets, derived from its resolved position within + /// the range so that a write lands on the register the sensor reads from. + uint16_t write_address() const { + return this->range_start_address + (this->addresses_bits() ? this->offset : this->offset / 2); + } + + /// Records the offset as configured, and seeds the resolved position with it. Building the ranges + /// overwrites `offset` with the position within the range; an item that is never polled keeps this + /// value, which is what its own address arithmetic expects. + void set_offset_from_start_address(uint8_t offset) { + this->offset_from_start_address = offset; + this->offset = offset; + } + + /// Sets the configured address, and points the range base at it. Building the ranges moves the base + /// to the range's first register; an item that is never polled (an output, or a switch with + /// assumed_state) keeps its own address, so write_address() stays correct for it. + void set_address(uint16_t address) { + this->start_address = address; + this->range_start_address = address; + } void set_custom_data(const std::vector &data) { custom_data = data; } size_t virtual get_register_size() const { - if (register_type == modbus::EntityType::COIL || register_type == modbus::EntityType::DISCRETE_INPUT) { + if (this->addresses_bits()) { return 1; } else { // if CONF_RESPONSE_BYTES is used override the default return response_bytes > 0 ? response_bytes : register_count * 2; @@ -123,9 +173,21 @@ class SensorItem { SensorValueType sensor_value_type{SensorValueType::RAW}; uint16_t start_address{0}; uint32_t bitmask{0}; + /// Position of this sensor's data within its range's response - a byte offset for registers, a bit + /// index for coils and discrete inputs. Resolved while the ranges are built, so it already accounts + /// for the registers ahead of it (including wide response_size ones) and for any offset inherited + /// from an earlier sensor sharing the same register. uint8_t offset{0}; uint8_t register_count{0}; uint8_t response_bytes{0}; + /// The offset exactly as configured: measured from this sensor's own start_address, where `offset` + /// is measured from the first register of the range it ends up polled in. Same units as `offset` - + /// bytes for registers, bits for coils and discrete inputs. Kept so the resolution can be recomputed, + /// and so the sort order of the sensor set never depends on the resolved value. + /// Declared before range_start_address so it lands in the padding after response_bytes. + uint8_t offset_from_start_address{0}; + /// First register of the range this sensor is polled in; equals start_address for an unpolled item. + uint16_t range_start_address{0}; uint16_t skip_updates{0}; std::vector custom_data{}; bool force_new_range{false}; @@ -151,9 +213,11 @@ class SensorItemsComparator { return lhs->start_address < rhs->start_address; } - // sort by offset (ensures update of sensors in ascending order) - if (lhs->offset != rhs->offset) { - return lhs->offset < rhs->offset; + // sort by the offset as configured (ensures update of sensors in ascending order). The resolved + // `offset` is deliberately not used: ranges are built while iterating this set and assign it, and + // a sort key that changed under the iteration would corrupt the set's ordering. + if (lhs->offset_from_start_address != rhs->offset_from_start_address) { + return lhs->offset_from_start_address < rhs->offset_from_start_address; } // The pointer to the sensor is used last to ensure that @@ -398,9 +462,8 @@ class ModbusController final : public PollingComponent, public modbus::ModbusCli * @param item SensorItem object * @return float value of data */ -inline float payload_to_float(std::span data, const SensorItem &item) { - int64_t number = - modbus::helpers::payload_to_number(data, item.sensor_value_type, item.offset, item.bitmask).value_or(0); +inline float payload_to_float(std::span data, const SensorItem &item, size_t offset) { + int64_t number = modbus::helpers::payload_to_number(data, item.sensor_value_type, offset, item.bitmask).value_or(0); float float_value; if (modbus::helpers::value_type_is_float(item.sensor_value_type)) { @@ -412,4 +475,12 @@ inline float payload_to_float(std::span data, const SensorItem &i return float_value; } +// Remove before 2027.2.0 (window opened when this helper gained an explicit offset). item.offset is +// the item's resolved position within its range's response, so this decodes the same bytes as passing +// that offset explicitly. +ESPDEPRECATED("Pass the offset explicitly: payload_to_float(data, item, item.offset). Removed in 2027.2.0", "2026.8.0") +inline float payload_to_float(std::span data, const SensorItem &item) { + return payload_to_float(data, item, item.offset); +} + } // namespace esphome::modbus_controller diff --git a/esphome/components/modbus_controller/number/modbus_number.cpp b/esphome/components/modbus_controller/number/modbus_number.cpp index 7b18b9e9fc..a2a49dcaf0 100644 --- a/esphome/components/modbus_controller/number/modbus_number.cpp +++ b/esphome/components/modbus_controller/number/modbus_number.cpp @@ -10,8 +10,8 @@ static const char *const TAG = "modbus.number"; // Maximum uint16_t registers to log in verbose hex output static constexpr size_t MODBUS_NUMBER_MAX_LOG_REGISTERS = 32; -void ModbusNumber::parse_and_publish(const std::vector &data) { - float result = payload_to_float(data, *this) / this->multiply_by_; +void ModbusNumber::parse_and_publish(std::span data) { + float result = payload_to_float(data, *this, this->offset) / this->multiply_by_; // Is there a lambda registered // call it with the pre converted value and the raw data array @@ -70,12 +70,10 @@ void ModbusNumber::control(float value) { // Create and send the write command if (this->register_count == 1 && !this->use_write_multiple_) { - // since offset is in bytes and a register is 16 bits we get the start by adding offset/2 - write_cmd = ModbusCommandItem::create_write_single_command(this->parent_, this->start_address + this->offset / 2, - payload[0]); + write_cmd = ModbusCommandItem::create_write_single_command(this->parent_, this->write_address(), payload[0]); } else { - write_cmd = ModbusCommandItem::create_write_multiple_command( - this->parent_, this->start_address + this->offset / 2, this->register_count, payload); + write_cmd = ModbusCommandItem::create_write_multiple_command(this->parent_, this->write_address(), + this->register_count, payload); } // publish new value write_cmd.on_data_func = [this, write_cmd, value](modbus::EntityType register_type, uint16_t start_address, diff --git a/esphome/components/modbus_controller/number/modbus_number.h b/esphome/components/modbus_controller/number/modbus_number.h index 582b042caf..1f0d0581eb 100644 --- a/esphome/components/modbus_controller/number/modbus_number.h +++ b/esphome/components/modbus_controller/number/modbus_number.h @@ -15,8 +15,8 @@ class ModbusNumber final : public number::Number, public Component, public Senso ModbusNumber(modbus::EntityType register_type, uint16_t start_address, uint8_t offset, uint32_t bitmask, SensorValueType value_type, int register_count, uint16_t skip_updates, bool force_new_range) { this->register_type = register_type; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->bitmask = bitmask; this->sensor_value_type = value_type; this->register_count = register_count; @@ -25,12 +25,12 @@ class ModbusNumber final : public number::Number, public Component, public Senso }; void dump_config() override; - void parse_and_publish(const std::vector &data) override; + void parse_and_publish(std::span data) override; float get_setup_priority() const override { return setup_priority::HARDWARE; } void set_parent(ModbusController *parent) { this->parent_ = parent; } void set_write_multiply(float factor) { this->multiply_by_ = factor; } - using transform_func_t = optional (*)(ModbusNumber *, float, const std::vector &); + using transform_func_t = optional (*)(ModbusNumber *, float, std::span); using write_transform_func_t = optional (*)(ModbusNumber *, float, std::vector &); void set_template(transform_func_t f) { this->transform_func_ = f; } void set_write_template(write_transform_func_t f) { this->write_transform_func_ = f; } diff --git a/esphome/components/modbus_controller/output/modbus_output.h b/esphome/components/modbus_controller/output/modbus_output.h index c9efd42224..17eb8e3a8f 100644 --- a/esphome/components/modbus_controller/output/modbus_output.h +++ b/esphome/components/modbus_controller/output/modbus_output.h @@ -12,21 +12,21 @@ class ModbusFloatOutput final : public output::FloatOutput, public Component, pu public: ModbusFloatOutput(uint16_t start_address, uint8_t offset, SensorValueType value_type, int register_count) { this->register_type = modbus::EntityType::HOLDING; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->bitmask = 0xFFFFFFFF; this->register_count = register_count; this->sensor_value_type = value_type; this->skip_updates = 0; - this->start_address += offset; - this->offset = 0; + this->set_address(this->start_address + offset); + this->set_offset_from_start_address(0); } void dump_config() override; void set_parent(ModbusController *parent) { this->parent_ = parent; } void set_write_multiply(float factor) { this->multiply_by_ = factor; } // Do nothing - void parse_and_publish(const std::vector &data) override{}; + void parse_and_publish(std::span data) override{}; using write_transform_func_t = optional (*)(ModbusFloatOutput *, float, std::vector &); void set_write_template(write_transform_func_t f) { this->write_transform_func_ = f; } @@ -45,19 +45,19 @@ class ModbusBinaryOutput final : public output::BinaryOutput, public Component, public: ModbusBinaryOutput(uint16_t start_address, uint8_t offset) { this->register_type = modbus::EntityType::COIL; - this->start_address = start_address; + this->set_address(start_address); this->bitmask = 0xFFFFFFFF; this->sensor_value_type = SensorValueType::BIT; this->skip_updates = 0; this->register_count = 1; - this->start_address += offset; - this->offset = 0; + this->set_address(this->start_address + offset); + this->set_offset_from_start_address(0); } void dump_config() override; void set_parent(ModbusController *parent) { this->parent_ = parent; } // Do nothing - void parse_and_publish(const std::vector &data) override{}; + void parse_and_publish(std::span data) override{}; using write_transform_func_t = optional (*)(ModbusBinaryOutput *, bool, std::vector &); void set_write_template(write_transform_func_t f) { this->write_transform_func_ = f; } diff --git a/esphome/components/modbus_controller/select/__init__.py b/esphome/components/modbus_controller/select/__init__.py index 334a4dfd76..5127360770 100644 --- a/esphome/components/modbus_controller/select/__init__.py +++ b/esphome/components/modbus_controller/select/__init__.py @@ -115,10 +115,7 @@ async def to_code(config): [ (ModbusSelect.operator("const_ptr"), "item"), (cg.int64, "x"), - ( - cg.std_vector.template(cg.uint8).operator("const").operator("ref"), - "data", - ), + (cg.std_span.template(cg.uint8.operator("const")), "data"), ], return_type=cg.optional.template(cg.std_string), ) diff --git a/esphome/components/modbus_controller/select/modbus_select.cpp b/esphome/components/modbus_controller/select/modbus_select.cpp index daa6b10da4..c2d87619a1 100644 --- a/esphome/components/modbus_controller/select/modbus_select.cpp +++ b/esphome/components/modbus_controller/select/modbus_select.cpp @@ -7,10 +7,9 @@ static const char *const TAG = "modbus_controller.select"; void ModbusSelect::dump_config() { LOG_SELECT(TAG, "Modbus Controller Select", this); } -void ModbusSelect::parse_and_publish(const std::vector &data) { - int64_t value = modbus::helpers::payload_to_number(std::span(data), this->sensor_value_type, - this->offset, this->bitmask) - .value_or(0); +void ModbusSelect::parse_and_publish(std::span data) { + int64_t value = + modbus::helpers::payload_to_number(data, this->sensor_value_type, this->offset, this->bitmask).value_or(0); ESP_LOGD(TAG, "New select value %lld from payload", value); @@ -86,7 +85,7 @@ void ModbusSelect::control(size_t index) { return; } - const uint16_t write_address = this->start_address + this->offset / 2; + const uint16_t write_address = this->write_address(); ModbusCommandItem write_cmd; if ((this->register_count == 1) && (!this->use_write_multiple_)) { write_cmd = ModbusCommandItem::create_write_single_command(this->parent_, write_address, data[0]); diff --git a/esphome/components/modbus_controller/select/modbus_select.h b/esphome/components/modbus_controller/select/modbus_select.h index b4834ba4c6..e1ae578ddf 100644 --- a/esphome/components/modbus_controller/select/modbus_select.h +++ b/esphome/components/modbus_controller/select/modbus_select.h @@ -15,9 +15,9 @@ class ModbusSelect final : public Component, public select::Select, public Senso bool force_new_range, std::vector mapping) { this->register_type = modbus::EntityType::HOLDING; // not configurable this->sensor_value_type = sensor_value_type; - this->start_address = start_address; - this->offset = 0; // not configurable - this->bitmask = 0xFFFFFFFF; // not configurable + this->set_address(start_address); + this->set_offset_from_start_address(0); // not configurable + this->bitmask = 0xFFFFFFFF; // not configurable this->register_count = register_count; this->response_bytes = 0; // not configurable this->skip_updates = skip_updates; @@ -25,7 +25,7 @@ class ModbusSelect final : public Component, public select::Select, public Senso this->mapping_ = std::move(mapping); } - using transform_func_t = optional (*)(ModbusSelect *const, int64_t, const std::vector &); + using transform_func_t = optional (*)(ModbusSelect *const, int64_t, std::span); using write_transform_func_t = optional (*)(ModbusSelect *const, const std::string &, int64_t, std::vector &); @@ -36,7 +36,7 @@ class ModbusSelect final : public Component, public select::Select, public Senso void set_write_template(write_transform_func_t f) { this->write_transform_func_ = f; } void dump_config() override; - void parse_and_publish(const std::vector &data) override; + void parse_and_publish(std::span data) override; void control(size_t index) override; protected: diff --git a/esphome/components/modbus_controller/sensor/modbus_sensor.cpp b/esphome/components/modbus_controller/sensor/modbus_sensor.cpp index 559724057a..b2bc2b5fd0 100644 --- a/esphome/components/modbus_controller/sensor/modbus_sensor.cpp +++ b/esphome/components/modbus_controller/sensor/modbus_sensor.cpp @@ -8,8 +8,8 @@ static const char *const TAG = "modbus_controller.sensor"; void ModbusSensor::dump_config() { LOG_SENSOR(TAG, "Modbus Controller Sensor", this); } -void ModbusSensor::parse_and_publish(const std::vector &data) { - float result = payload_to_float(data, *this); +void ModbusSensor::parse_and_publish(std::span data) { + float result = payload_to_float(data, *this, this->offset); // Is there a lambda registered // call it with the pre converted value and the raw data array diff --git a/esphome/components/modbus_controller/sensor/modbus_sensor.h b/esphome/components/modbus_controller/sensor/modbus_sensor.h index 1d11aa4d66..61fdaacd10 100644 --- a/esphome/components/modbus_controller/sensor/modbus_sensor.h +++ b/esphome/components/modbus_controller/sensor/modbus_sensor.h @@ -13,8 +13,8 @@ class ModbusSensor final : public Component, public sensor::Sensor, public Senso ModbusSensor(modbus::EntityType register_type, uint16_t start_address, uint8_t offset, uint32_t bitmask, SensorValueType value_type, int register_count, uint16_t skip_updates, bool force_new_range) { this->register_type = register_type; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->bitmask = bitmask; this->sensor_value_type = value_type; this->register_count = register_count; @@ -22,9 +22,9 @@ class ModbusSensor final : public Component, public sensor::Sensor, public Senso this->force_new_range = force_new_range; } - void parse_and_publish(const std::vector &data) override; + void parse_and_publish(std::span data) override; void dump_config() override; - using transform_func_t = optional (*)(ModbusSensor *, float, const std::vector &); + using transform_func_t = optional (*)(ModbusSensor *, float, std::span); void set_template(transform_func_t f) { this->transform_func_ = f; } diff --git a/esphome/components/modbus_controller/switch/modbus_switch.cpp b/esphome/components/modbus_controller/switch/modbus_switch.cpp index b8cdbf018d..adbd812348 100644 --- a/esphome/components/modbus_controller/switch/modbus_switch.cpp +++ b/esphome/components/modbus_controller/switch/modbus_switch.cpp @@ -27,16 +27,17 @@ void ModbusSwitch::set_assumed_state(bool assumed_state) { this->assumed_state_ bool ModbusSwitch::assumed_state() { return this->assumed_state_; } -void ModbusSwitch::parse_and_publish(const std::vector &data) { +void ModbusSwitch::parse_and_publish(std::span data) { bool value = false; + // For coils/discrete inputs this is the bit index; for registers it is the byte offset. + const size_t offset = this->offset; switch (this->register_type) { case modbus::EntityType::DISCRETE_INPUT: case modbus::EntityType::COIL: - // offset for coil is the actual number of the coil not the byte offset - value = modbus::helpers::bit_from_packed(this->offset, data); + value = modbus::helpers::bit_from_packed(offset, data); break; default: - value = modbus::helpers::get_data(data, this->offset) & this->bitmask; + value = modbus::helpers::get_data(data.data(), offset) & this->bitmask; break; } @@ -51,8 +52,8 @@ void ModbusSwitch::parse_and_publish(const std::vector &data) { } } - ESP_LOGV(TAG, "Publish '%s': new value = %s type = %d address = %X offset = %x", this->get_name().c_str(), - ONOFF(value), (int) this->register_type, this->start_address, this->offset); + ESP_LOGV(TAG, "Publish '%s': new value = %s type = %d address = %X offset = %zx", this->get_name().c_str(), + ONOFF(value), (int) this->register_type, this->start_address, offset); this->publish_state(value); } @@ -92,18 +93,16 @@ void ModbusSwitch::write_state(bool state) { // offset for coil and discrete inputs is the coil/register number not bytes if (this->use_write_multiple_) { std::vector states{state}; - cmd = ModbusCommandItem::create_write_multiple_coils(this->parent_, this->start_address + this->offset, states); + cmd = ModbusCommandItem::create_write_multiple_coils(this->parent_, this->write_address(), states); } else { - cmd = ModbusCommandItem::create_write_single_coil(this->parent_, this->start_address + this->offset, state); + cmd = ModbusCommandItem::create_write_single_coil(this->parent_, this->write_address(), state); } } else { - // since offset is in bytes and a register is 16 bits we get the start by adding offset/2 if (this->use_write_multiple_) { std::vector bool_states(1, state ? (0xFFFF & this->bitmask) : 0); - cmd = ModbusCommandItem::create_write_multiple_command(this->parent_, this->start_address + this->offset / 2, 1, - bool_states); + cmd = ModbusCommandItem::create_write_multiple_command(this->parent_, this->write_address(), 1, bool_states); } else { - cmd = ModbusCommandItem::create_write_single_command(this->parent_, this->start_address + this->offset / 2, + cmd = ModbusCommandItem::create_write_single_command(this->parent_, this->write_address(), state ? 0xFFFF & this->bitmask : 0u); } } diff --git a/esphome/components/modbus_controller/switch/modbus_switch.h b/esphome/components/modbus_controller/switch/modbus_switch.h index 0d5456aa63..e5b8cf5c21 100644 --- a/esphome/components/modbus_controller/switch/modbus_switch.h +++ b/esphome/components/modbus_controller/switch/modbus_switch.h @@ -13,15 +13,15 @@ class ModbusSwitch final : public Component, public switch_::Switch, public Sens ModbusSwitch(modbus::EntityType register_type, uint16_t start_address, uint8_t offset, uint32_t bitmask, uint16_t skip_updates, bool force_new_range) { this->register_type = register_type; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->bitmask = bitmask; this->sensor_value_type = SensorValueType::BIT; this->skip_updates = skip_updates; this->register_count = 1; if (register_type == modbus::EntityType::HOLDING || register_type == modbus::EntityType::COIL) { - this->start_address += offset; - this->offset = 0; + this->set_address(this->start_address + offset); + this->set_offset_from_start_address(0); } this->force_new_range = force_new_range; }; @@ -30,10 +30,10 @@ class ModbusSwitch final : public Component, public switch_::Switch, public Sens void dump_config() override; void set_assumed_state(bool assumed_state); void set_state(bool state) { this->state = state; } - void parse_and_publish(const std::vector &data) override; + void parse_and_publish(std::span data) override; void set_parent(ModbusController *parent) { this->parent_ = parent; } - using transform_func_t = optional (*)(ModbusSwitch *, bool, const std::vector &); + using transform_func_t = optional (*)(ModbusSwitch *, bool, std::span); using write_transform_func_t = optional (*)(ModbusSwitch *, bool, std::vector &); void set_template(transform_func_t f) { this->publish_transform_func_ = f; } void set_write_template(write_transform_func_t f) { this->write_transform_func_ = f; } diff --git a/esphome/components/modbus_controller/text_sensor/modbus_textsensor.cpp b/esphome/components/modbus_controller/text_sensor/modbus_textsensor.cpp index 5626515638..31b3fb3e55 100644 --- a/esphome/components/modbus_controller/text_sensor/modbus_textsensor.cpp +++ b/esphome/components/modbus_controller/text_sensor/modbus_textsensor.cpp @@ -8,10 +8,11 @@ static const char *const TAG = "modbus_controller.text_sensor"; void ModbusTextSensor::dump_config() { LOG_TEXT_SENSOR("", "Modbus Controller Text Sensor", this); } -void ModbusTextSensor::parse_and_publish(const std::vector &data) { +void ModbusTextSensor::parse_and_publish(std::span data) { std::string output_str{}; uint8_t items_left = this->response_bytes; - uint8_t index = this->offset; + const size_t start_offset = this->offset; + size_t index = start_offset; while ((items_left > 0) && index < data.size()) { uint8_t b = data[index]; switch (this->encode_) { @@ -25,7 +26,7 @@ void ModbusTextSensor::parse_and_publish(const std::vector &data) { case RawEncoding::COMMA: { // max 5: optional ','(1) + uint8(3) + null, for both ",%d" and "%d" char dec_buf[5]; - snprintf(dec_buf, sizeof(dec_buf), index != this->offset ? ",%d" : "%d", b); + snprintf(dec_buf, sizeof(dec_buf), index != start_offset ? ",%d" : "%d", b); output_str += dec_buf; break; } diff --git a/esphome/components/modbus_controller/text_sensor/modbus_textsensor.h b/esphome/components/modbus_controller/text_sensor/modbus_textsensor.h index c7381d7ddd..e8de46b55a 100644 --- a/esphome/components/modbus_controller/text_sensor/modbus_textsensor.h +++ b/esphome/components/modbus_controller/text_sensor/modbus_textsensor.h @@ -15,8 +15,8 @@ class ModbusTextSensor final : public Component, public text_sensor::TextSensor, ModbusTextSensor(modbus::EntityType register_type, uint16_t start_address, uint8_t offset, uint8_t register_count, uint16_t response_bytes, RawEncoding encode, uint16_t skip_updates, bool force_new_range) { this->register_type = register_type; - this->start_address = start_address; - this->offset = offset; + this->set_address(start_address); + this->set_offset_from_start_address(offset); this->response_bytes = response_bytes; this->register_count = register_count; this->encode_ = encode; @@ -28,8 +28,8 @@ class ModbusTextSensor final : public Component, public text_sensor::TextSensor, void dump_config() override; - void parse_and_publish(const std::vector &data) override; - using transform_func_t = optional (*)(ModbusTextSensor *, std::string, const std::vector &); + void parse_and_publish(std::span data) override; + using transform_func_t = optional (*)(ModbusTextSensor *, std::string, std::span); void set_template(transform_func_t f) { this->transform_func_ = f; } protected: diff --git a/tests/components/modbus_controller/common.yaml b/tests/components/modbus_controller/common.yaml index aa2855c2b0..a0db1e7888 100644 --- a/tests/components/modbus_controller/common.yaml +++ b/tests/components/modbus_controller/common.yaml @@ -118,6 +118,60 @@ sensor: value_type: U_WORD lambda: |- return x / 10.0; + # Non-mergeable sensor sharing the start address of modbus_sensor1 (different register_count): + # must join the same range, never open a second range keyed on the same (address, type). + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_shared_addr + name: Test Sensor Shared Address + register_type: holding + address: 0x9001 + value_type: U_DWORD + # Sensors sharing one start address with distinct byte offsets (mixed register counts, so they take + # the shared-start path: each resolves to exactly its configured offset, no accumulation). + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_shared_offs0 + name: Test Sensor Shared Offset Base + register_type: holding + address: 0x9020 + value_type: U_DWORD + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_shared_offs1 + name: Test Sensor Shared Offset Low Word + register_type: holding + address: 0x9020 + value_type: U_WORD + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_shared_offs2 + name: Test Sensor Shared Offset High Word + register_type: holding + address: 0x9020 + value_type: U_WORD + offset: 2 + # Raw-decode lambda in the documented style: `item->offset` locates this sensor's data in the range + # response, and the compatibility helpers accept the span the lambda is handed. + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_raw_lambda + name: Test Sensor Raw Lambda + register_type: holding + address: 0x9050 + value_type: U_WORD + lambda: |- + return modbus_controller::get_data(data, item->offset) * 0.1f; + # force_new_range sensors sort before plain ones, so this high-address forced sensor is grouped + # first and the lower-address plain sensors above must still get their own ranges. + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_sensor_forced_high + name: Test Sensor Forced High Address + register_type: holding + address: 0x9040 + value_type: U_WORD + force_new_range: true switch: - platform: modbus_controller @@ -158,3 +212,22 @@ text_sensor: response_size: 4 lambda: |- return "Modified: " + x; + # A register reporting FEWER bytes than 2*register_count (response_size: 3 for 2 registers), followed + # by a contiguous sensor: the follower's byte position must track the actual 3 bytes, not underflow. + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_text_sensor_narrow + name: Test Text Sensor Narrow Response + register_type: holding + address: 0x9030 + register_count: 2 + response_size: 3 + raw_encode: HEXBYTES + - platform: modbus_controller + modbus_controller_id: modbus_controller1 + id: modbus_text_sensor_after_narrow + name: Test Text Sensor After Narrow + register_type: holding + address: 0x9032 + register_count: 1 + raw_encode: HEXBYTES diff --git a/tests/components/modbus_controller/sensor_item_position_test.cpp b/tests/components/modbus_controller/sensor_item_position_test.cpp new file mode 100644 index 0000000000..2fb679ee07 --- /dev/null +++ b/tests/components/modbus_controller/sensor_item_position_test.cpp @@ -0,0 +1,89 @@ +#include + +#include +#include + +#include "esphome/components/modbus_controller/modbus_controller.h" + +namespace esphome::modbus_controller::testing { + +namespace { + +// Minimal concrete SensorItem so the position/address accessors can be exercised directly. +class TestSensorItem : public SensorItem { + public: + void parse_and_publish(std::span /*data*/) override {} +}; + +// Builds an item the way a platform constructor does, before ranges are built. +TestSensorItem make_item(modbus::EntityType type, uint16_t address, uint8_t offset) { + TestSensorItem item; + item.register_type = type; + item.set_address(address); + item.set_offset_from_start_address(offset); + return item; +} + +} // namespace + +// A freshly constructed item is already usable: its resolved position is the offset as configured and +// its range base is its own address, which is what an item that never gets polled relies on. +TEST(SensorItemPosition, ConstructionSeedsResolvedPositionAndRangeBase) { + auto item = make_item(modbus::EntityType::HOLDING, 0x9001, 4); + EXPECT_EQ(item.offset_from_start_address, 4); + EXPECT_EQ(item.offset, 4); + EXPECT_EQ(item.range_start_address, 0x9001); +} + +// A write lands on the register the sensor reads from. The resolved position is relative to the range's +// first register, which may be earlier than the sensor's own address, so both are needed to get there. +TEST(SensorItemPosition, WriteAddressForRegisters) { + auto item = make_item(modbus::EntityType::HOLDING, 0x9003, 0); + item.range_start_address = 0x9001; + item.offset = 4; + EXPECT_EQ(item.write_address(), 0x9003); +} + +// Coils index bits, so the resolved offset is a bit count and is added to the range base directly. +TEST(SensorItemPosition, WriteAddressForCoils) { + auto item = make_item(modbus::EntityType::COIL, 0x15, 0); + item.range_start_address = 0x10; + item.offset = 5; + EXPECT_EQ(item.write_address(), 0x15); + EXPECT_TRUE(item.addresses_bits()); +} + +// An item that is never polled keeps the range base its constructor set, so its write address is still +// its own address plus its configured offset - a switch with assumed_state, or an output. +TEST(SensorItemPosition, WriteAddressWithoutAGroupedRange) { + auto item = make_item(modbus::EntityType::HOLDING, 0x9010, 2); + EXPECT_EQ(item.write_address(), 0x9011); +} + +// A sensor re-using a register after one with a non-zero offset resolves past that offset, and its +// write address follows the same position - the behaviour releases before the range rework had. +TEST(SensorItemPosition, ReUseChainWriteAddressFollowsResolvedPosition) { + auto item = make_item(modbus::EntityType::HOLDING, 0x9001, 4); + item.range_start_address = 0x9001; + item.offset = 6; // 4 configured, plus the 2 the previous sensor on this register resolved to + EXPECT_EQ(item.write_address(), 0x9004); +} + +// Registers address 16-bit words; only coils and discrete inputs address bits. +TEST(SensorItemPosition, AddressesBitsOnlyForCoilAndDiscreteInput) { + EXPECT_FALSE(make_item(modbus::EntityType::HOLDING, 0, 0).addresses_bits()); + EXPECT_FALSE(make_item(modbus::EntityType::INPUT_REGISTER, 0, 0).addresses_bits()); + EXPECT_TRUE(make_item(modbus::EntityType::COIL, 0, 0).addresses_bits()); + EXPECT_TRUE(make_item(modbus::EntityType::DISCRETE_INPUT, 0, 0).addresses_bits()); +} + +// A span payload reaches payload_to_number() unqualified from inside this namespace: SensorValueType +// lives in modbus::helpers, so argument-dependent lookup finds the helper. Declaring a same-signature +// forwarder here would make the call ambiguous rather than convenient, which is why none exists. +TEST(SensorItemPosition, UnqualifiedPayloadToNumberResolvesToTheHelper) { + const uint8_t bytes[] = {0x01, 0x02}; + auto value = payload_to_number(std::span(bytes), SensorValueType::U_WORD, 0, 0xFFFFFFFF); + EXPECT_EQ(value, 0x0102); +} + +} // namespace esphome::modbus_controller::testing diff --git a/tests/integration/fixtures/uart_mock_modbus_grouping.yaml b/tests/integration/fixtures/uart_mock_modbus_grouping.yaml new file mode 100644 index 0000000000..a5394f1d05 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_grouping.yaml @@ -0,0 +1,234 @@ +esphome: + name: uart-mock-modbus-group + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +uart: + baud_rate: 115200 + port: /dev/null + +uart_mock: + - id: virtual_uart_dev + baud_rate: 9600 + rx_full_threshold: 120 + rx_timeout: 2 + auto_start: false + debug: + responses: + # One entry per range the controller polls. A frame the controller does not send goes unanswered, + # so these also pin the grouping: an extra or differently shaped read fails the test. + - expect_tx: [0x01, 0x01, 0x00, 0x10, 0x00, 0x02, 0xBC, 0x0E] # coils 0x10 count 2 + inject_rx: [0x01, 0x01, 0x01, 0x01, 0x90, 0x48] # bit0 set, bit1 clear + - expect_tx: [0x01, 0x03, 0x01, 0x60, 0x00, 0x01, 0x85, 0xE8] # holding 0x160 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x60, 0xB9, 0xFC] # 352 + - expect_tx: [0x01, 0x03, 0x01, 0x00, 0x00, 0x01, 0x85, 0xF6] # holding 0x100 count 1 + inject_rx: [0x01, 0x03, 0x04, 0x01, 0x11, 0x02, 0x22, 0x2A, 0xB3] # 4 bytes: 273 then 546 + - expect_tx: [0x01, 0x03, 0x01, 0x20, 0x00, 0x04, 0x44, 0x3F] # holding 0x120 count 4 + inject_rx: [0x01, 0x03, 0x08, 0x12, 0x34, 0x56, 0x78, 0x9A, 0xBC, 0xDE, 0xF0, 0x7A, 0x25] + - expect_tx: [0x01, 0x03, 0x01, 0x30, 0x00, 0x02, 0xC5, 0xF8] # holding 0x130 count 2 + inject_rx: [0x01, 0x03, 0x06, 0x0A, 0xAA, 0xFF, 0xFF, 0x0B, 0xBB, 0x7E, 0xA0] # 6 bytes + - expect_tx: [0x01, 0x03, 0x01, 0x40, 0x00, 0x01, 0x84, 0x22] # holding 0x140 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x40, 0xB8, 0x24] # 320 + - expect_tx: [0x01, 0x03, 0x01, 0x45, 0x00, 0x01, 0x94, 0x23] # holding 0x145 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x45, 0x78, 0x27] # 325 + - expect_tx: [0x01, 0x03, 0x01, 0x50, 0x00, 0x02, 0xC5, 0xE6] # holding 0x150 count 2 + inject_rx: [0x01, 0x03, 0x04, 0x01, 0x50, 0x01, 0x51, 0x3B, 0xB2] # 336, 337 + - expect_tx: [0x01, 0x03, 0x01, 0x80, 0x00, 0x02, 0xC4, 0x1F] # holding 0x180 count 2 + inject_rx: [0x01, 0x03, 0x06, 0x11, 0x11, 0x22, 0x22, 0x33, 0x33, 0x20, 0xA0] # 6 bytes + # 0x181 answers with the same value whether it is read on its own or as part of the block above, + # so the sensor there is pinned to one value regardless of which range it lands in. + - expect_tx: [0x01, 0x03, 0x01, 0x81, 0x00, 0x01, 0xD5, 0xDE] # holding 0x181 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x33, 0x33, 0xEC, 0xA1] # 13107 + - expect_tx: [0x01, 0x03, 0x01, 0x70, 0x00, 0x03, 0x05, 0xEC] # holding 0x170 count 3 + inject_rx: [0x01, 0x03, 0x06, 0x00, 0x2A, 0x1B, 0x2C, 0x03, 0x0D, 0x3E, 0xAB] # 6 bytes + - expect_tx: [0x01, 0x03, 0x01, 0x61, 0x00, 0x01, 0xD4, 0x28] # holding 0x161 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x61, 0x78, 0x3C] # 353 + +modbus: + uart_id: virtual_uart_dev + send_wait_time: 200ms + turnaround_time: 10ms + +modbus_controller: + - address: 1 + id: modbus_controller_ok + max_cmd_retries: 2 + update_interval: never + +# Each block below is a distinct address range exercising one grouping relationship. The blocks are far +# enough apart that they never merge into each other. +sensor: + # A - two sensors on one register that returns more bytes than its count implies (response_size), + # reading different halves of it. + - platform: modbus_controller + name: "reuse_lo" + address: 0x100 + register_type: holding + value_type: U_WORD + response_size: 4 + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "reuse_hi" + address: 0x100 + register_type: holding + value_type: U_WORD + offset: 2 + response_size: 4 + modbus_controller_id: modbus_controller_ok + + # C - plain contiguous registers of differing widths. + - platform: modbus_controller + name: "ext_word" + address: 0x120 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "ext_next" + address: 0x121 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "ext_dword" + address: 0x122 + register_type: holding + value_type: U_DWORD + modbus_controller_id: modbus_controller_ok + + # D - a wide (response_size) register followed by a contiguous one: the follower must start after the + # bytes the wide register actually returned, not after 2 * register_count. + - platform: modbus_controller + name: "wide_first" + address: 0x130 + register_type: holding + value_type: U_WORD + response_size: 4 + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "wide_next" + address: 0x131 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + + # E - a gap: these must never share a range. + - platform: modbus_controller + name: "gap_low" + address: 0x140 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "gap_high" + address: 0x145 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + + # F - contiguous registers where the second asks for a slower rate. + - platform: modbus_controller + name: "rate_first" + address: 0x150 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "rate_slow" + address: 0x151 + register_type: holding + value_type: U_WORD + skip_updates: 5 + modbus_controller_id: modbus_controller_ok + + # B - a wide value and one of its halves share a start address, with a contiguous sensor after them. + # The differing offsets give these a defined order, unlike two sensors that differ only in width. + - platform: modbus_controller + name: "shared_dword" + address: 0x170 + register_type: holding + value_type: U_DWORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "shared_high" + address: 0x170 + register_type: holding + value_type: U_WORD + offset: 2 + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "shared_after" + address: 0x172 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + + # I - a register that returns more bytes than its count implies, sharing its address with a plain + # wider sensor. Whether the sensor after them is read as part of that block or on its own, it must + # decode 0x181 - never the bytes that lie two into the block, which is where the widened register + # count alone would put it. + - platform: modbus_controller + name: "masked_wide" + address: 0x180 + register_type: holding + value_type: U_WORD + response_size: 4 + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "masked_pair" + address: 0x180 + register_type: holding + value_type: U_DWORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "masked_after" + address: 0x181 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + + # H - a sensor pinned to its own range, followed by a contiguous one. + - platform: modbus_controller + name: "forced_first" + address: 0x160 + register_type: holding + value_type: U_WORD + force_new_range: true + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "forced_next" + address: 0x161 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + +binary_sensor: + # G - contiguous coils, addressed by bit. + - platform: modbus_controller + name: "coil_first" + address: 0x10 + register_type: coil + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "coil_next" + address: 0x11 + register_type: coil + modbus_controller_id: modbus_controller_ok + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + on_press: + - lambda: |- + id(virtual_uart_dev).start_scenario(); + id(modbus_controller_ok).set_update_interval(1000); + id(modbus_controller_ok).start_poller(); diff --git a/tests/integration/fixtures/uart_mock_modbus_shared_address.yaml b/tests/integration/fixtures/uart_mock_modbus_shared_address.yaml new file mode 100644 index 0000000000..25574d0c42 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_shared_address.yaml @@ -0,0 +1,160 @@ +esphome: + name: uart-mock-modbus-shared + +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_dev + baud_rate: 9600 + rx_full_threshold: 120 + rx_timeout: 2 + auto_start: false + debug: + responses: + # Three sensors, one frame. At 0x9001 a U_WORD (1 register) and a U_DWORD (2 registers) share the + # start address but cannot merge, so the range widens to count 2. A third sensor at 0x9002 falls + # inside the widened range and must read its slice of the same response rather than splitting into + # a second overlapping poll. The single expect_tx pins the "one frame on the wire" contract - any + # duplicate or overlapping range would put an extra frame on the bus and fail to match. + - expect_tx: [0x01, 0x03, 0x90, 0x01, 0x00, 0x02, 0xB8, 0xCB] # Read holding 0x9001 count 2 on device 1 + inject_rx: [0x01, 0x03, 0x04, 0x03, 0x97, 0x02, 0x91, 0x8B, 0x57] # 0x9001=0x0397, 0x9002=0x0291 + # A force_new_range sensor at a HIGH address (0x30) sorts before the plain sensor at a LOW address + # (0x10). The two must poll as separate ranges: the covered branch's lower-bound check prevents the + # 0x10 sensor from being absorbed into the forced 0x30 range with a wrapped byte offset. + - expect_tx: [0x01, 0x03, 0x00, 0x30, 0x00, 0x01, 0x84, 0x05] # Read holding 0x30 count 1 (forced range) + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x11, 0x79, 0xD8] # 0x30 = 0x0111 = 273 + - expect_tx: [0x01, 0x03, 0x00, 0x10, 0x00, 0x01, 0x85, 0xCF] # Read holding 0x10 count 1 (own range) + inject_rx: [0x01, 0x03, 0x02, 0x02, 0x22, 0x39, 0x3D] # 0x10 = 0x0222 = 546 + # A wide sensor (U_QWORD at 0x100, 4 registers) followed by plain sensors at 0x101 and 0x103. + # None of them merge, so all three poll separately - exactly as before the range refactor. The + # 0x103 sensor sits at the wide range's tail address, so it must not anchor a re-use join on a + # mid-range predecessor and inherit its byte offset. + - expect_tx: [0x01, 0x03, 0x01, 0x00, 0x00, 0x04, 0x45, 0xF5] # Read holding 0x100 count 4 + inject_rx: [0x01, 0x03, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x64, 0x94, 0x3C] # = 100 + - expect_tx: [0x01, 0x03, 0x01, 0x01, 0x00, 0x01, 0xD4, 0x36] # Read holding 0x101 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0x41, 0x79, 0xE4] # 0x101 = 0x0141 = 321 + - expect_tx: [0x01, 0x03, 0x01, 0x03, 0x00, 0x01, 0x75, 0xF6] # Read holding 0x103 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x01, 0xA5, 0x79, 0xAF] # 0x103 = 0x01A5 = 421 + # A widened shared-address range at 0x200 plus a sensor at 0x201 carrying its own skip_updates. + # The sensor must keep its own range so the polling rates stay independent; if it were folded into + # the widened range it would decode 0x201 from THAT response (2, not 777) and drag the range's + # rate down to its own. + - expect_tx: [0x01, 0x03, 0x02, 0x00, 0x00, 0x02, 0xC5, 0xB3] # Read holding 0x200 count 2 + inject_rx: [0x01, 0x03, 0x04, 0x01, 0x41, 0x00, 0x02, 0x2A, 0x1A] # 0x200=0x0141, 0x201=0x0002 + - expect_tx: [0x01, 0x03, 0x02, 0x01, 0x00, 0x01, 0xD4, 0x72] # Read holding 0x201 count 1 + inject_rx: [0x01, 0x03, 0x02, 0x03, 0x09, 0x78, 0xB2] # 0x201 = 0x0309 = 777 + +modbus: + uart_id: virtual_uart_dev + send_wait_time: 200ms + turnaround_time: 10ms + +modbus_controller: + - address: 1 + id: modbus_controller_ok + max_cmd_retries: 2 + update_interval: never + +sensor: + # Word sensor at 0x9001 (1 register) + - platform: modbus_controller + name: "shared_word" + address: 0x9001 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + # Dword sensor at the SAME address 0x9001 (2 registers) - non-mergeable, shares the range start + - platform: modbus_controller + name: "shared_dword" + address: 0x9001 + register_type: holding + value_type: U_DWORD + modbus_controller_id: modbus_controller_ok + # Word sensor at 0x9002 - inside the widened range, reads bytes 2-3 of the same response + - platform: modbus_controller + name: "covered_word" + address: 0x9002 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + # Forced sensor at a high address: sorts first, opens its own isolated range + - platform: modbus_controller + name: "forced_high" + address: 0x30 + register_type: holding + value_type: U_WORD + force_new_range: true + modbus_controller_id: modbus_controller_ok + # Plain sensor at a lower address: must get its own range, never absorbed into the forced one + - platform: modbus_controller + name: "plain_low" + address: 0x10 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + # Wide sensor spanning 0x100-0x103; the two sensors below sit inside its span but do not merge + - platform: modbus_controller + name: "wide_qword" + address: 0x100 + register_type: holding + value_type: U_QWORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "inside_wide" + address: 0x101 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + # At the wide range's tail address: must decode its own poll, not inherit a mid-range byte offset + - platform: modbus_controller + name: "tail_of_wide" + address: 0x103 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + # Shared address 0x200: the dword widens the range the word opened (or vice versa) + - platform: modbus_controller + name: "rate_word" + address: 0x200 + register_type: holding + value_type: U_WORD + modbus_controller_id: modbus_controller_ok + - platform: modbus_controller + name: "rate_dword" + address: 0x200 + register_type: holding + value_type: U_DWORD + modbus_controller_id: modbus_controller_ok + # Inside the widened range but with its own skip_updates: must NOT be folded in, or the two sensors + # above would silently drop to this sensor's polling rate + - platform: modbus_controller + name: "own_rate" + address: 0x201 + register_type: holding + value_type: U_WORD + skip_updates: 100 + modbus_controller_id: modbus_controller_ok + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + on_press: + - lambda: |- + id(virtual_uart_dev).start_scenario(); + id(modbus_controller_ok).set_update_interval(1000); + id(modbus_controller_ok).start_poller(); diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 2c437341c6..ce707fb0e0 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -330,3 +330,119 @@ async def test_uart_mock_modbus_server_controller_multiple( await tracker.setup_and_start_scenario(client) await tracker.await_all(futures) _assert_no_modbus_errors(error_log_lines, warning_log_lines) + + +@pytest.mark.asyncio +async def test_uart_mock_modbus_grouping( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Pins how sensors are grouped into polled ranges across the combinations that matter. + + Each block in the fixture covers one relationship between neighbouring sensors - sharing a wide + register, contiguous, separated by a gap, differing polling rates, coils, and a pinned range - so + that the frames on the wire and the byte each sensor decodes from are locked down. + """ + + line_callback, error_log_lines, warning_log_lines = _make_modbus_line_callback() + + # Values are those the component produced before the range rework, captured from it directly. + expected_values = { + # one register returning 4 bytes, read as two halves + "reuse_lo": 273, + "reuse_hi": 546, + # contiguous registers, mixed widths + "ext_word": 4660, + "ext_next": 22136, + "ext_dword": pytest.approx(2596069120), + # a wide register pushes its neighbour past the bytes it actually returned + "wide_first": 2730, + "wide_next": 3003, + # a gap keeps them apart + "gap_low": 320, + "gap_high": 325, + # contiguous, second one polling more slowly + "rate_first": 336, + "rate_slow": 337, + # a wide value, one of its halves, and the register after it + "shared_dword": pytest.approx(2759468), + "shared_high": 6956, + "shared_after": 781, + # a wide register hidden behind a wider plain sibling, and the sensor after them + "masked_wide": 4369, + "masked_pair": pytest.approx(286335522), + "masked_after": 13107, + # pinned range, and the contiguous sensor after it + "forced_first": 352, + "forced_next": 353, + } + tracker = SensorTracker(list(expected_values.keys())) + futures = tracker.expect_all(expected_values) + + 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_all(futures) + # Every frame sent must match one the mock answers, so an unexpected read (a range that split, + # merged or changed length) shows up here as an unanswered request. This is what pins the coil + # grouping too, since binary sensors carry no numeric state to compare. + _assert_no_modbus_errors(error_log_lines, warning_log_lines) + + +@pytest.mark.asyncio +async def test_uart_mock_modbus_shared_address( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Sensors sharing and overlapping one register range must all decode from a single read. + + A U_WORD and a U_DWORD share start address 0x9001 (non-mergeable, so the range widens to 2 + registers) and a third U_WORD at 0x9002 falls inside the widened range. A regression guard for the + range-grouping rewrite: without the same-address fallback the shared sensors land in duplicate + ranges and one never publishes; without the in-range join the 0x9002 sensor splits into a second + overlapping frame that the mock (which expects exactly one read) never answers. + + A force_new_range sensor at 0x30 plus a plain sensor at 0x10 pin the covered branch's lower-bound + check: the forced sensor sorts first, and without the bound the lower-address sensor is absorbed + into the forced range with a wrapped byte offset and never polls its own register. + + A U_QWORD at 0x100 with plain sensors at 0x101 and 0x103 pins that non-merging sensors inside a + wide sensor's span keep polling separately, and that the sensor at the span's tail address does not + anchor a re-use join on a mid-range predecessor (which would make it decode that sensor's bytes). + + A sensor at 0x201 carrying skip_updates sits inside a widened shared-address range at 0x200 but + keeps its own range, so polling rates stay independent; folding it in would also make it decode + 0x201 out of the shared response (2) instead of its own poll (777). + """ + + line_callback, error_log_lines, warning_log_lines = _make_modbus_line_callback() + + # 0x9001 = 0x0397 (919); 0x9001..0x9002 = 0x03970291 (60228241, approx: not exact in float32); + # 0x9002 = 0x0291 (657); 0x30 = 0x0111 (273); 0x10 = 0x0222 (546) + expected_values = { + "shared_word": 919, + "shared_dword": pytest.approx(60228241), + "covered_word": 657, + "forced_high": 273, + "plain_low": 546, + "wide_qword": 100, + "inside_wide": 321, + "tail_of_wide": 421, + "rate_word": 321, + "rate_dword": pytest.approx(21037058), + "own_rate": 777, + } + tracker = SensorTracker(list(expected_values.keys())) + futures = tracker.expect_all(expected_values) + + 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_all(futures) + _assert_no_modbus_errors(error_log_lines, warning_log_lines)