github/esphome
2
0
Fork 0
mirror of https://github.com/esphome/esphome.git synced 2026-08-22 22:26:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

[modbus] Fold the repeated PDU validation idioms into shared helpers (#17888)

Browse Source 
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Bonne Eggleston
2026-07-27 10:44:15 -10:00
committed by GitHub
co-authored by Claude Fable 5
parent 19511f5787
commit 5eeb780538
2 changed files with 50 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
esphome/components/modbus/modbus_definitions.h
+4 -1
View File
@@ -123,6 +123,9 @@ static constexpr uint16_t MAX_FRAME_SIZE = 256;
* with any subscript. Writes and forwarding are defensive: set() drops out-of-range bits and * with any subscript. Writes and forwarding are defensive: set() drops out-of-range bits and
* bytes() clamps to the real span, because those paths touch buffers and the wire directly. * bytes() clamps to the real span, because those paths touch buffers and the wire directly.
*/ */
/// Bits pack 8 per data byte, rounded up to whole bytes.
constexpr size_t packed_bit_bytes(size_t bits) { return (bits + 7) / 8; }
class PackedBits { class PackedBits {
public: public:
PackedBits(std::span<const uint8_t> data, uint16_t count) : data_(data), count_(count) {} PackedBits(std::span<const uint8_t> data, uint16_t count) : data_(data), count_(count) {}
@@ -134,7 +137,7 @@ class PackedBits {
/// over a larger buffer - forwarding this span onto the wire can never leak trailing buffer content. /// over a larger buffer - forwarding this span onto the wire can never leak trailing buffer content.
/// Clamped to the actual span so a view over a too-short buffer stays detectable instead of UB. /// Clamped to the actual span so a view over a too-short buffer stays detectable instead of UB.
std::span<const uint8_t> bytes() const { std::span<const uint8_t> bytes() const {
return this->data_.first(std::min<size_t>((this->count_ + 7) / 8, this->data_.size())); return this->data_.first(std::min<size_t>(packed_bit_bytes(this->count_), this->data_.size()));
} }
private: private:
esphome/components/modbus/modbus_helpers.cpp
+46 -41
View File
@@ -7,6 +7,19 @@ namespace esphome::modbus::helpers {
static const char *const TAG = "modbus_helpers"; static const char *const TAG = "modbus_helpers";
// A quantity/address pair is standard when the quantity is non-zero, within the per-table maximum,
// and the range [start_address, start_address + quantity) stays inside the 16-bit address space
// (the 32-bit promotion is the overflow guard - a 16-bit sum could wrap and pass).
static bool quantity_in_range(uint16_t start_address, uint16_t quantity, uint16_t max_quantity) {
return quantity != 0 && quantity <= max_quantity && uint32_t(start_address) + quantity <= 0x10000u;
}
// The spec allows exactly ON (0xFF00) and OFF (0x0000) for a single-coil value, on the request and
// on its echoed response alike.
static bool is_canonical_coil_value(uint8_t high_byte, uint8_t low_byte) {
return (high_byte == 0xFF || high_byte == 0x00) && low_byte == 0x00;
}
uint16_t server_pdu_length(const uint8_t *frame, size_t size) { uint16_t server_pdu_length(const uint8_t *frame, size_t size) {
if (size < MIN_PDU_SIZE) if (size < MIN_PDU_SIZE)
return MIN_PDU_SIZE; return MIN_PDU_SIZE;
@@ -82,11 +95,12 @@ bool is_server_pdu_standard(const uint8_t *pdu, size_t size) {
if (server_pdu_length(pdu, size) != size) if (server_pdu_length(pdu, size) != size)
return false; return false;
switch (static_cast<FunctionCode>(pdu[0])) { const auto function_code = static_cast<FunctionCode>(pdu[0]);
switch (function_code) {
case FunctionCode::READ_COILS: case FunctionCode::READ_COILS:
case FunctionCode::READ_DISCRETE_INPUTS: case FunctionCode::READ_DISCRETE_INPUTS:
// A conformant bit-read response carries at least one packed byte (up to 2000 bits = 250 bytes). // A conformant bit-read response carries at least one packed byte (up to 2000 bits = 250 bytes).
return pdu[1] != 0 && pdu[1] <= uint8_t((MAX_NUM_OF_COILS_TO_READ + 7) / 8); return pdu[1] != 0 && pdu[1] <= uint8_t(packed_bit_bytes(MAX_NUM_OF_COILS_TO_READ));
case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_HOLDING_REGISTERS:
case FunctionCode::READ_INPUT_REGISTERS: case FunctionCode::READ_INPUT_REGISTERS:
// Registers are 2 bytes each: the byte count must be a non-zero even count within the read maximum. // Registers are 2 bytes each: the byte count must be a non-zero even count within the read maximum.
@@ -99,15 +113,13 @@ bool is_server_pdu_standard(const uint8_t *pdu, size_t size) {
case FunctionCode::WRITE_MULTIPLE_COILS: case FunctionCode::WRITE_MULTIPLE_COILS:
case FunctionCode::WRITE_MULTIPLE_REGISTERS: { case FunctionCode::WRITE_MULTIPLE_REGISTERS: {
// The response echoes start address and quantity: bound them like the request side does. // The response echoes start address and quantity: bound them like the request side does.
const bool bits = static_cast<FunctionCode>(pdu[0]) == FunctionCode::WRITE_MULTIPLE_COILS; const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS;
const uint16_t start_address = get_data<uint16_t>(pdu, 1);
const uint16_t quantity = get_data<uint16_t>(pdu, 3);
const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE; const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE;
return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u; return quantity_in_range(get_data<uint16_t>(pdu, 1), get_data<uint16_t>(pdu, 3), max_quantity);
} }
case FunctionCode::WRITE_SINGLE_COIL: case FunctionCode::WRITE_SINGLE_COIL:
// The response echoes the request, so the same ON/OFF constraint applies. // The response echoes the request, so the same ON/OFF constraint applies.
return (pdu[3] == 0xFF || pdu[3] == 0x00) && pdu[4] == 0x00; return is_canonical_coil_value(pdu[3], pdu[4]);
default: default:
return true; // All other function codes validated by length alone return true; // All other function codes validated by length alone
} }
@@ -117,45 +129,38 @@ bool is_client_pdu_standard(const uint8_t *pdu, size_t size) {
if (client_pdu_length(pdu, size) != size) if (client_pdu_length(pdu, size) != size)
return false; return false;
switch (static_cast<FunctionCode>(pdu[0])) { const auto function_code = static_cast<FunctionCode>(pdu[0]);
switch (function_code) {
case FunctionCode::READ_COILS: case FunctionCode::READ_COILS:
case FunctionCode::READ_DISCRETE_INPUTS: case FunctionCode::READ_DISCRETE_INPUTS:
case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_HOLDING_REGISTERS:
case FunctionCode::READ_INPUT_REGISTERS: { case FunctionCode::READ_INPUT_REGISTERS: {
const bool bits = static_cast<FunctionCode>(pdu[0]) == FunctionCode::READ_COILS || const bool bits =
static_cast<FunctionCode>(pdu[0]) == FunctionCode::READ_DISCRETE_INPUTS; function_code == FunctionCode::READ_COILS || function_code == FunctionCode::READ_DISCRETE_INPUTS;
const uint16_t start_address = get_data<uint16_t>(pdu, 1);
const uint16_t quantity = get_data<uint16_t>(pdu, 3);
const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_READ : MAX_NUM_OF_REGISTERS_TO_READ; const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_READ : MAX_NUM_OF_REGISTERS_TO_READ;
return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u; return quantity_in_range(get_data<uint16_t>(pdu, 1), get_data<uint16_t>(pdu, 3), max_quantity);
} }
case FunctionCode::WRITE_MULTIPLE_COILS: case FunctionCode::WRITE_MULTIPLE_COILS:
case FunctionCode::WRITE_MULTIPLE_REGISTERS: { case FunctionCode::WRITE_MULTIPLE_REGISTERS: {
const bool bits = static_cast<FunctionCode>(pdu[0]) == FunctionCode::WRITE_MULTIPLE_COILS; const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS;
const uint16_t start_address = get_data<uint16_t>(pdu, 1);
const uint16_t quantity = get_data<uint16_t>(pdu, 3); const uint16_t quantity = get_data<uint16_t>(pdu, 3);
const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE; const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE;
// Coils are packed 8 per data byte; registers are 2 bytes each. // Coils are packed 8 per data byte; registers are 2 bytes each.
const size_t expected_data_bytes = bits ? (static_cast<size_t>(quantity) + 7) / 8 : quantity * 2; const size_t expected_data_bytes = bits ? packed_bit_bytes(quantity) : quantity * 2;
return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u && return quantity_in_range(get_data<uint16_t>(pdu, 1), quantity, max_quantity) && pdu[5] == expected_data_bytes;
pdu[5] == expected_data_bytes;
} }
case FunctionCode::READ_FILE_RECORD: case FunctionCode::READ_FILE_RECORD:
case FunctionCode::WRITE_FILE_RECORD: case FunctionCode::WRITE_FILE_RECORD:
return pdu[1] <= MAX_PDU_SIZE - 2; return pdu[1] <= MAX_PDU_SIZE - 2;
case FunctionCode::READ_WRITE_MULTIPLE_REGISTERS: { case FunctionCode::READ_WRITE_MULTIPLE_REGISTERS: {
const uint16_t start_address_read = get_data<uint16_t>(pdu, 1);
const uint16_t quantity_read = get_data<uint16_t>(pdu, 3);
const uint16_t start_address_write = get_data<uint16_t>(pdu, 5);
const uint16_t quantity_write = get_data<uint16_t>(pdu, 7); const uint16_t quantity_write = get_data<uint16_t>(pdu, 7);
return quantity_read != 0 && quantity_read <= MAX_NUM_OF_REGISTERS_TO_READ && quantity_write != 0 && return quantity_in_range(get_data<uint16_t>(pdu, 1), get_data<uint16_t>(pdu, 3), MAX_NUM_OF_REGISTERS_TO_READ) &&
quantity_write <= MAX_NUM_OF_REGISTERS_TO_WRITE_RW && quantity_in_range(get_data<uint16_t>(pdu, 5), quantity_write, MAX_NUM_OF_REGISTERS_TO_WRITE_RW) &&
(uint32_t) start_address_read + quantity_read <= 0x10000u && pdu[9] == quantity_write * 2;
(uint32_t) start_address_write + quantity_write <= 0x10000u && pdu[9] == quantity_write * 2;
} }
case FunctionCode::WRITE_SINGLE_COIL: case FunctionCode::WRITE_SINGLE_COIL:
// The one variable field in an otherwise fixed-shape PDU: the spec allows exactly ON/OFF. // The one variable field in an otherwise fixed-shape PDU: the spec allows exactly ON/OFF.
return (pdu[3] == 0xFF || pdu[3] == 0x00) && pdu[4] == 0x00; return is_canonical_coil_value(pdu[3], pdu[4]);
default: default:
return true; // All other function codes validated by length alone return true; // All other function codes validated by length alone
} }
@@ -292,6 +297,14 @@ static void append_pdu_header(StaticVector<uint8_t, CAP> &pdu, FunctionCode func
pdu.push_back(second >> 0); pdu.push_back(second >> 0);
} }
// Zero the unused bits of a multi-coil write's final data byte, as the spec requires. Kept in one
// place so the generic and typed coil builders produce identical wire bytes for the same write.
static void mask_trailing_pad_bits(std::span<uint8_t> data, uint16_t bit_count) {
if (data.empty() || bit_count % 8 == 0)
return;
data.back() &= static_cast<uint8_t>((1 << (bit_count % 8)) - 1);
}
ReadPdu create_read_pdu(FunctionCode function_code, uint16_t start_address, uint16_t number_of_entities) { ReadPdu create_read_pdu(FunctionCode function_code, uint16_t start_address, uint16_t number_of_entities) {
ReadPdu pdu; // declared before every return so NRVO fires (all paths return the same object) ReadPdu pdu; // declared before every return so NRVO fires (all paths return the same object)
if (number_of_entities == 0) { if (number_of_entities == 0) {
@@ -394,8 +407,7 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address,
} }
// The spec allows exactly ON (0xFF00) and OFF (0x0000) for a single-coil write - the same rule // The spec allows exactly ON (0xFF00) and OFF (0x0000) for a single-coil write - the same rule
// is_client_pdu_standard() enforces, so a built frame cannot be misclassified on reply. // is_client_pdu_standard() enforces, so a built frame cannot be misclassified on reply.
if (function_code == FunctionCode::WRITE_SINGLE_COIL && if (function_code == FunctionCode::WRITE_SINGLE_COIL && !is_canonical_coil_value(values[0], values[1])) {
((values[0] != 0xFF && values[0] != 0x00) || values[1] != 0x00)) {
ESP_LOGE(TAG, "Invalid single-coil value %02X%02X (must be FF00 or 0000), dropping request", values[0], ESP_LOGE(TAG, "Invalid single-coil value %02X%02X (must be FF00 or 0000), dropping request", values[0],
values[1]); values[1]);
return pdu; return pdu;
@@ -409,8 +421,7 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address,
// non-standard on reply, and the spec bound keeps the PDU within capacity by construction. // non-standard on reply, and the spec bound keeps the PDU within capacity by construction.
// Checked before the header append: a failed check must return an empty PDU, not a 5-byte partial one. // Checked before the header append: a failed check must return an empty PDU, not a 5-byte partial one.
const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS; const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS;
const size_t expected_len = const size_t expected_len = bits ? packed_bit_bytes(number_of_entities) : static_cast<size_t>(number_of_entities) * 2;
bits ? (static_cast<size_t>(number_of_entities) + 7) / 8 : static_cast<size_t>(number_of_entities) * 2;
if (values_len != expected_len) { if (values_len != expected_len) {
ESP_LOGE(TAG, "values_len %zu does not match %u entities (expected %zu) for function code %02X, dropping request", ESP_LOGE(TAG, "values_len %zu does not match %u entities (expected %zu) for function code %02X, dropping request",
values_len, number_of_entities, expected_len, static_cast<uint8_t>(function_code)); values_len, number_of_entities, expected_len, static_cast<uint8_t>(function_code));
@@ -420,11 +431,8 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address,
pdu.push_back(values_len); // Byte count is required for write multiple pdu.push_back(values_len); // Byte count is required for write multiple
for (size_t i = 0; i < values_len; i++) for (size_t i = 0; i < values_len; i++)
pdu.push_back(values[i]); pdu.push_back(values[i]);
// Zero the unused bits of the final byte as the spec requires, matching the typed coil builder if (bits)
// so both produce identical wire bytes for the same write. mask_trailing_pad_bits(pdu, number_of_entities);
if (bits && number_of_entities % 8 != 0) {
pdu[pdu.size() - 1] &= static_cast<uint8_t>((1 << (number_of_entities % 8)) - 1);
}
return pdu; return pdu;
} }
@@ -484,7 +492,7 @@ static void build_write_coils_pdu(PduBuffer &pdu, uint16_t start_address, Packed
ESP_LOGE(TAG, "Write of %u coils at %u runs past the 16-bit address space, dropping request", count, start_address); ESP_LOGE(TAG, "Write of %u coils at %u runs past the 16-bit address space, dropping request", count, start_address);
return; return;
} }
const size_t byte_count = (count + 7) / 8; const size_t byte_count = packed_bit_bytes(count);
if (packed_bits.size() < byte_count) { if (packed_bits.size() < byte_count) {
ESP_LOGE(TAG, "packed_bits (%zu bytes) does not cover %u coils (%zu bytes), dropping request", packed_bits.size(), ESP_LOGE(TAG, "packed_bits (%zu bytes) does not cover %u coils (%zu bytes), dropping request", packed_bits.size(),
count, byte_count); count, byte_count);
@@ -495,10 +503,7 @@ static void build_write_coils_pdu(PduBuffer &pdu, uint16_t start_address, Packed
for (size_t i = 0; i != byte_count; i++) { for (size_t i = 0; i != byte_count; i++) {
pdu.push_back(packed_bits[i]); pdu.push_back(packed_bits[i]);
} }
// Zero the unused bits of the final byte, as the spec requires mask_trailing_pad_bits(pdu, count);
if (count % 8 != 0) {
pdu[pdu.size() - 1] &= static_cast<uint8_t>((1 << (count % 8)) - 1);
}
} }
PduBuffer create_write_coils_pdu(uint16_t start_address, PackedBits bits) { PduBuffer create_write_coils_pdu(uint16_t start_address, PackedBits bits) {
@@ -515,7 +520,7 @@ PduBuffer create_write_coils_pdu(uint16_t start_address, std::span<const bool> v
MAX_NUM_OF_COILS_TO_WRITE); MAX_NUM_OF_COILS_TO_WRITE);
return pdu; return pdu;
} }
StaticVector<uint8_t, (MAX_NUM_OF_COILS_TO_WRITE + 7) / 8> packed; StaticVector<uint8_t, packed_bit_bytes(MAX_NUM_OF_COILS_TO_WRITE)> packed;
for (size_t i = 0; i != values.size(); i++) { for (size_t i = 0; i != values.size(); i++) {
if (i % 8 == 0) if (i % 8 == 0)
packed.push_back(0); packed.push_back(0);