github/esphome
2
0
Fork 0
mirror of https://github.com/esphome/esphome.git synced 2026-09-27 15:00:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

[modbus] Hub and helpers cleanup; tighten queue_pdu validation (#18847)

Browse Source
This commit is contained in:
Bonne Eggleston
2026-08-28 13:16:41 -05:00
committed by GitHub
parent 8db07d0de5
commit 768ab5b672
9 changed files with 207 additions and 275 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/components/modbus/modbus_client_hub_test.cpp
+57 -24
View File
@@ -775,7 +775,7 @@ TEST(ModbusClientHubBroadcast, DeliversNoTerminalToTypedDevice) {
// 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).
// retiring it. Writes and custom/unknown codes still go through (covered in the neighboring tests).
TEST(ModbusClientHubBroadcast, RefusesReadBroadcast) {
NullUART uart;
NoResponseProbeHub hub;
@@ -814,9 +814,8 @@ TEST(ModbusClientHubBroadcast, AcceptsCustomBroadcast) {
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.
// An exception-flagged code (0x80 bit set) is never a valid request - that bit is response-only - so
// queue_pdu refuses it up front, before the broadcast guard, whatever its base code.
TEST(ModbusClientHubBroadcast, RefusesExceptionFlaggedCustomBroadcast) {
NullUART uart;
NoResponseProbeHub hub;
@@ -833,6 +832,50 @@ TEST(ModbusClientHubBroadcast, RefusesExceptionFlaggedCustomBroadcast) {
EXPECT_EQ(device.sent_count_, 0); // never transmitted
}
// FC23 (read/write multiple) has a read half that expects a reply, so the Modbus spec does not allow it
// as a broadcast. is_function_code_read() covers it, so the broadcast guard refuses it despite its write
// half.
TEST(ModbusClientHubBroadcast, RefusesReadWriteMultipleBroadcast) {
NullUART uart;
NoResponseProbeHub hub;
hub.set_uart_parent(&uart);
hub.setup();
BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS);
// fc, read start+qty, write start+qty, byte count, one data word.
const uint8_t read_write_multiple[] = {0x17, 0x00, 0x00, 0x00, 0x01, 0x00, 0x10, 0x00, 0x01, 0x02, 0xBE, 0xEF};
EXPECT_FALSE(device.queue_pdu(read_write_multiple)); // its read half could never be answered
EXPECT_EQ(hub.entries(), 0u);
}
// FC 0x18 (read FIFO queue) is not a "read" by is_function_code_read(), but the hub has an explicit
// response-length rule for it - it demonstrably expects a reply, so it cannot broadcast.
TEST(ModbusClientHubBroadcast, RefusesKnownLengthNonWriteBroadcast) {
NullUART uart;
NoResponseProbeHub hub;
hub.set_uart_parent(&uart);
hub.setup();
BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS);
const uint8_t read_fifo[] = {0x18, 0x00, 0x10}; // fc, FIFO pointer address
EXPECT_FALSE(device.queue_pdu(read_fifo));
EXPECT_EQ(hub.entries(), 0u);
}
// A code that is neither a read nor exception-flagged (here 0x63, unassigned) is fire-and-forget on a
// broadcast: the hub can't know it isn't a vendor write, so it is accepted and delivered to all devices.
TEST(ModbusClientHubBroadcast, AcceptsNonReadUnknownBroadcast) {
NullUART uart;
NoResponseProbeHub hub;
hub.set_uart_parent(&uart);
hub.setup();
BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS);
const uint8_t unknown[] = {0x63, 0x00, 0x01};
EXPECT_TRUE(device.queue_pdu(unknown)); // not a read, so not refused
EXPECT_EQ(hub.entries(), 1u);
}
namespace {
// tx_blocked() clear for send_next_frame_'s gate, then blocked for send_frame_'s post-delay re-check.
class RejectPostDelayHub : public NoResponseProbeHub {
@@ -1882,30 +1925,20 @@ TEST(ModbusClientHubPriority, ResendFromOnResponseAbsorbsIntoCompletingCommand)
EXPECT_FALSE(hub.queued(0).options.continuous); // the one-shot re-send downgraded the poll
}
// An exception-flagged function code is never silently re-sendable, even though the read check
// masks the exception bit: its duplicate takes the drop path like any other non-read.
TEST(ModbusClientHubPriority, ExceptionFlaggedDuplicateDroppedNotPromoted) {
// The exception bit marks a response, so a request carrying it is refused outright.
TEST(ModbusClientHubPriority, ExceptionFlaggedPduRefused) {
NoResponseProbeHub hub;
SentCountingDevice device(&hub, 0x02);
const uint8_t weird[] = {0x83, 0x01, 0x00, 0x00, 0x02}; // read-shaped but exception-flagged
EXPECT_TRUE(device.queue_pdu(weird));
EXPECT_FALSE(device.queue_pdu(weird)); // non-requeueable: cap of one, so the duplicate is refused
// The 0x80 exception flag is a response-only bit; a request must never set it. queue_pdu refuses an
// exception-flagged PDU up front - nothing is queued - whether its base code reads (0x83 = 0x03 | 0x80)
// or writes (0x86 = 0x06 | 0x80).
const uint8_t read_shaped[] = {0x83, 0x01, 0x00, 0x00, 0x02};
const uint8_t write_shaped[] = {0x86, 0x00, 0x10, 0xBE, 0xEF};
EXPECT_FALSE(device.queue_pdu(read_shaped));
EXPECT_FALSE(device.queue_pdu(write_shaped));
hub.sweep_for_test();
ASSERT_EQ(hub.queued_frames(), 1u);
EXPECT_EQ(hub.queued(0).pending, 1u);
EXPECT_EQ(device.not_sent_count_, 0);
// The write-shaped twin (0x86 masks to WRITE_SINGLE_REGISTER) must not take WRITE-class
// ordering either: exception-flagged codes are excluded from the mutates classification.
const uint8_t weird_write[] = {0x86, 0x00, 0x10, 0xBE, 0xEF};
device.queue_pdu(weird_write);
ASSERT_EQ(hub.queued_frames(), 2u);
EXPECT_EQ(hub.queued(1).priority(), CommandPriority::READ); // not WRITE
const ModbusDeviceCommand *next = hub.next_ready();
ASSERT_NE(next, nullptr);
EXPECT_EQ(next->frame.pdu()[0], 0x83); // FIFO by age: it did not jump the older entry
EXPECT_EQ(hub.queued_frames(), 0u);
}
namespace {
tests/components/modbus/modbus_helpers_test.cpp
+6
View File
@@ -63,6 +63,12 @@ TEST(ModbusClientFrameLength, TooShortReturnsMinimum) {
EXPECT_EQ(client_frame_length(frame, 1), MIN_FRAME_SIZE);
}
TEST(ModbusClientFrameLength, ExceptionFlaggedIsTheExceptionShape) {
// Sized at 2 so an exception-flagged request fails its CRC at once instead of being scanned for.
const uint8_t exception_request[] = {0x83, 0x02};
EXPECT_EQ(client_pdu_length(exception_request, sizeof(exception_request)), 2);
}
TEST(ModbusClientFrameLength, ReadAndWriteSingleAreFixed) {
// basic_register request fixture is a read-holding request -> 8 bytes
const uint8_t read[] = {0x01, 0x03, 0x00, 0x03, 0x00, 0x01, 0x74, 0x0A};
tests/components/modbus/modbus_unknown_function_test.cpp
+23 -9
View File
@@ -54,7 +54,8 @@ class TestServerHub : public ModbusServerHub {
// The frame-length parsers have explicit cases for exactly these 13 codes; every other value - the
// assigned-but-unimplemented management codes, both user-defined ranges, and all unassigned codes -
// must classify as unknown length. The exception flag masks off first.
// must classify as unknown length. Exception replies are always the 2-byte spec shape, so every
// 0x80-set code is known length.
TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) {
for (uint8_t fc : {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x0F, 0x10, 0x14, 0x15, 0x16, 0x17, 0x18}) {
EXPECT_FALSE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc);
@@ -62,11 +63,13 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) {
for (uint8_t fc : {0x07, 0x08, 0x0B, 0x0C, 0x11, 0x2A, 0x41, 0x48, 0x49, 0x64, 0x6E, 0x00, 0x7F}) {
EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc);
}
// Exception replies classify by their base code.
// Every exception-flagged code is known length (the 2-byte spec exception shape), whatever its base.
EXPECT_FALSE(helpers::is_function_code_unknown_length(0x83));
EXPECT_TRUE(helpers::is_function_code_unknown_length(0x87));
// Strictly wider than the user-defined ranges: every custom code is unknown-length, but not vice versa.
for (int fc = 0; fc <= 0xFF; fc++) {
EXPECT_FALSE(helpers::is_function_code_unknown_length(0x87));
EXPECT_FALSE(helpers::is_function_code_unknown_length(0xC9));
// Strictly wider than the user-defined ranges below 0x80: every non-exception custom code is
// unknown-length, but not vice versa.
for (int fc = 0; fc <= 0x7F; fc++) {
if (helpers::is_function_code_custom(fc))
EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << fc;
}
@@ -75,10 +78,10 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) {
// Derived contract check: the helper must say "unknown" exactly when both length parsers fall
// through to default. With a zero-filled max-size PDU every explicit case returns at least 2
// (file records bottom out at 2, FIFO at 3) and only default returns MIN_PDU_SIZE, so comparing
// against MIN_PDU_SIZE detects a case added to either switch without updating the helper. The
// loop stops at 0x7F: above it the helper masks the exception flag off while client_pdu_length()
// switches on the unmasked byte and server_pdu_length() early-returns the exception length.
for (int fc = 0; fc <= 0x7F; fc++) {
// against MIN_PDU_SIZE detects a case added to either switch without updating the helper. Both
// parsers early-return the 2-byte exception shape above 0x7F, which the helper's own exception
// early-return mirrors, so the whole byte range is covered.
for (int fc = 0; fc <= 0xFF; fc++) {
const uint8_t pdu[MAX_PDU_SIZE] = {static_cast<uint8_t>(fc)}; // zero header fields
EXPECT_EQ(helpers::is_function_code_unknown_length(fc),
helpers::client_pdu_length(pdu, sizeof(pdu)) == MIN_PDU_SIZE)
@@ -89,6 +92,17 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) {
}
}
// Broadcastable = writes plus unknown codes (possible vendor writes); everything known to expect a
// reply is not. Classifies the underlying code: the exception bit masks off first (0x85 as 0x05).
TEST(ModbusUnknownFunction, BroadcastableClassification) {
for (uint8_t fc : {0x05, 0x06, 0x0F, 0x10, 0x16, 0x49, 0x63, 0x6E, 0x85, 0xC9}) {
EXPECT_TRUE(helpers::is_function_code_broadcastable(fc)) << "fc 0x" << std::hex << int(fc);
}
for (uint8_t fc : {0x01, 0x02, 0x03, 0x04, 0x14, 0x15, 0x17, 0x18, 0x83, 0x97}) {
EXPECT_FALSE(helpers::is_function_code_broadcastable(fc)) << "fc 0x" << std::hex << int(fc);
}
}
// A response with a function code outside the user-defined ranges (0x49) has no length case in
// server_pdu_length(), so the parser must find the frame end by CRC scan - the same way it already
// handles user-defined codes. Frame: address + FC 0x49 + 3 data bytes + CRC = 7 bytes. Without the