From 422f4d7910382b7de958a0a45ad9ca3c6303d758 Mon Sep 17 00:00:00 2001 From: aleks Date: Mon, 4 May 2026 08:58:09 +0200 Subject: [PATCH 1/2] fix: improve event handling timings, add kconfig values --- Kconfig | 43 +++++++++++++++++++++++++ modbus/mb_ports/tcp/port_tcp_driver.c | 9 ++++-- modbus/mb_ports/tcp/port_tcp_driver.h | 8 ++--- modbus/mb_ports/tcp/port_tcp_slave.c | 45 +++++++++++++-------------- modbus/mb_ports/tcp/port_tcp_slave.h | 2 +- modbus/mb_ports/tcp/port_tcp_utils.c | 4 +-- modbus/mb_ports/tcp/port_tcp_utils.h | 2 +- 7 files changed, 78 insertions(+), 35 deletions(-) diff --git a/Kconfig b/Kconfig index 4d38df0c..70b6d4d0 100644 --- a/Kconfig +++ b/Kconfig @@ -49,6 +49,49 @@ menu "Modbus configuration" The keep-alive time can be configured and should be set based on the actual needs of the application, considering the trade-off between keeping the connection alive and minimizing system load. + config FMB_TCP_SELECT_WAIT_MS + int "Modbus TCP select wait timeout (ms)" + range 10 500 + default 50 + depends on FMB_COMM_MODE_TCP_EN + help + This option represents a maximum time the driver task blocks on select function per loop iteration. + Lower values improve responsiveness when multiple masters are connected + but slightly increase CPU usage. For high-throughput applications with + many concurrent connections, reducing to 10-20 ms may help. + + config FMB_TCP_READ_TIMEOUT_MS + int "Modbus TCP socket read timeout (ms)" + range 1 500 + default 150 + depends on FMB_COMM_MODE_TCP_EN + help + The option applies to SO_RCVTIMEO when reading the MBAP header from a socket. + This is the worst-case blocking time if a partial TCP segment arrives. + On a reliable LAN, 50-100 ms is sufficient. Lower values reduce the + maximum stall that one slow connection can impose on other connections. + + config FMB_TCP_SEND_TIMEOUT_MS + int "Modbus TCP send timeout (ms)" + range 10 1000 + default 100 + depends on FMB_COMM_MODE_TCP_EN + help + This is a timeout for the socket writability check and send operation. + Before each send, the driver verifies the socket is writable via select function. + Reduce for faster failure detection; increase on high-latency links. + + config FMB_TCP_EVENT_LOOP_TICK_MS + int "Modbus TCP event loop processing budget (ms)" + range 10 300 + default 50 + depends on FMB_COMM_MODE_TCP_EN + help + This option represents the maximum time allocated to event loop run function per driver loop iteration. + The value controls how many queued events (connect, send, error, etc.) are dispatched + before returning to check sockets. Lower values reduce latency for socket + I/O at the cost of processing fewer events per iteration. + config FMB_TCP_UID_ENABLED bool "Modbus TCP enable UID (Unit Identifier) support" default n diff --git a/modbus/mb_ports/tcp/port_tcp_driver.c b/modbus/mb_ports/tcp/port_tcp_driver.c index 9ec555a0..dc9dc0d6 100644 --- a/modbus/mb_ports/tcp/port_tcp_driver.c +++ b/modbus/mb_ports/tcp/port_tcp_driver.c @@ -619,6 +619,7 @@ void mb_drv_tcp_task(void *ctx) } else { // Is the fd event triggered, process the event if (drv_obj->event_fd && FD_ISSET(drv_obj->event_fd, &readset)) { + FD_CLR(drv_obj->event_fd, &readset); mb_event_info_t mb_event = {0}; int32_t event_id = read_event(ctx, &mb_event); ESP_LOGD(TAG, "%p, fd event get: 0x%02x:%d, %s", @@ -629,8 +630,10 @@ void mb_drv_tcp_task(void *ctx) if (err != ESP_OK) { ESP_LOGE(TAG, "%p, event loop run, returns fail: %x", ctx, (int)err); } - } else if (drv_obj->listen_sock_fd && FD_ISSET(drv_obj->listen_sock_fd, &readset)) { + } + if (drv_obj->listen_sock_fd && FD_ISSET(drv_obj->listen_sock_fd, &readset)) { // If something happened on the listen socket, then it is an incoming connection. + FD_CLR(drv_obj->listen_sock_fd, &readset); ESP_LOGD(TAG, "%p, listen_sock is active.", ctx); mb_uid_info_t node_info; int sock_id = port_accept_connection(drv_obj->listen_sock_fd, &node_info); @@ -649,12 +652,12 @@ void mb_drv_tcp_task(void *ctx) } } } - } else { + } + { // socket event is ready, process each socket event mb_drv_check_suspend_shutdown(ctx); int curr_fd = 0; mb_node_info_t *node_ptr = NULL; - ESP_LOGD(TAG, "%p, socket event active: %" PRIx64, ctx, *(uint64_t *)&readset); while (((node_ptr = mb_drv_get_next_node_from_set(ctx, &curr_fd, &readset)) && (curr_fd < MB_MAX_FDS))) { if (FD_ISSET(node_ptr->sock_id, &drv_obj->conn_set)) { diff --git a/modbus/mb_ports/tcp/port_tcp_driver.h b/modbus/mb_ports/tcp/port_tcp_driver.h index e860d989..91992972 100644 --- a/modbus/mb_ports/tcp/port_tcp_driver.h +++ b/modbus/mb_ports/tcp/port_tcp_driver.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -51,9 +51,9 @@ typedef void (*mb_event_handler_fp)(void *ctx, esp_event_base_t base, int32_t id #define MB_DROP_TRANSACTION_TIME_US (1000UL * (CONFIG_FMB_TCP_KEEP_ALIVE_TOUT_SEC * 2000UL)) // drop after twice keep alive timeout is reasonable #define MB_WAIT_DONE_MS (5000) -#define MB_SELECT_WAIT_MS (200) -#define MB_TCP_SEND_TIMEOUT_MS (500) -#define MB_TCP_EVENT_LOOP_TICK_MS (50) +#define MB_SELECT_WAIT_MS (CONFIG_FMB_TCP_SELECT_WAIT_MS) +#define MB_TCP_SEND_TIMEOUT_MS (CONFIG_FMB_TCP_SEND_TIMEOUT_MS) +#define MB_TCP_EVENT_LOOP_TICK_MS (CONFIG_FMB_TCP_EVENT_LOOP_TICK_MS) #define MB_DRIVER_CONFIG_DEFAULT { \ .spin_lock = portMUX_INITIALIZER_UNLOCKED, \ diff --git a/modbus/mb_ports/tcp/port_tcp_slave.c b/modbus/mb_ports/tcp/port_tcp_slave.c index bc1645f2..e8cd0834 100644 --- a/modbus/mb_ports/tcp/port_tcp_slave.c +++ b/modbus/mb_ports/tcp/port_tcp_slave.c @@ -503,37 +503,22 @@ MB_EVENT_HANDLER(mbs_on_send_data) uint16_t msg_id = 0; int node_id = 0; (void)transaction_item_get_data(item, NULL, &msg_id, &node_id); - // Check if the TID is equal to the current received TID for this node. - // If not, means the slave was not able to process the previous transaction on time. - // The reason is too much active connections or incorrect response time or request rate in the master. - if ((node_id != pnode->index) || (tid != msg_id) || (tid != pnode->tid_counter) || (MB_GET_NODE_STATE(pnode) < MB_SOCK_STATE_CONNECTED)) { - uint64_t tick = (transaction_tick_t)transaction_item_get_tick(item); - uint64_t time_div_us = (esp_timer_get_time() - tick); - ESP_LOGD(TAG, "%p, " MB_NODE_FMT(", frame TID:0x%04" PRIx16 "!=0x%04" PRIx16 " ,%" PRIu64 " ,%" PRIu64 " ,%" PRIx16 ", slave is busy."), + // Check if the first queued transaction matches this response. + // Only a genuine cross-node mismatch or disconnected state should trigger + // the "slave busy" exception. The tid_counter comparison is not used here + // because it reflects the latest received TID, which may have been bumped + // by a master retry while the current transaction was still in-progress. + if ((node_id != pnode->index) || (tid != msg_id) || (MB_GET_NODE_STATE(pnode) < MB_SOCK_STATE_CONNECTED)) { + ESP_LOGD(TAG, "%p, " MB_NODE_FMT(", frame TID:0x%04" PRIx16 "!=0x%04" PRIx16 ", slave is busy."), ctx, (int)pnode->index, (int)pnode->sock_id, - pnode->addr_info.ip_addr_str, pnode->tid_counter, tid, esp_timer_get_time(), tick, msg_id); - ESP_LOGW(TAG, "%p, " MB_NODE_FMT(", handling time [ms]: %" PRIu64 ", exceeds slave response time in master."), - ctx, (int)pnode->index, (int)pnode->sock_id, - pnode->addr_info.ip_addr_str, (time_div_us / 1000)); - // Hard hack to respond to next transaction with an exception - // MB_TCP_MBAP_SET_FIELD(frame_entry.buf, MB_TCP_TID, pnode->tid_counter); + pnode->addr_info.ip_addr_str, tid, msg_id); // Build the exception frame to inform that slave is busy frame_entry.buf[MB_TCP_FUNC] = (frame_entry.buf[MB_TCP_FUNC] | 0x80); - frame_entry.buf[MB_TCP_LEN + 1] = 3; // Length: UID + FUNC + EXCEPTION + frame_entry.buf[MB_TCP_LEN + 1] = 3; frame_entry.buf[MB_TCP_FUNC + 1] = MB_EX_SLAVE_BUSY; ret = port_write_poll(pnode, frame_entry.buf, MB_TCP_FUNC + 2, MB_TCP_SEND_TIMEOUT_MS); mb_drv_lock(drv_obj); if (ret >= 0) { - err = transaction_set_state(port_obj->transaction, tid, TRANSMITTED); - if (err == ESP_OK) { - ESP_LOGD(TAG, "%p, " MB_NODE_FMT(", sent packet TID: 0x%04" PRIx16 ", %p."), - drv_obj, pnode->index, pnode->sock_id, - pnode->addr_info.ip_addr_str, tid, frame_entry.buf); - } else { - ESP_LOGE(TAG, "%p, " MB_NODE_FMT(", transaction set state fail for TID: 0x%04" PRIx16 ", %p."), - drv_obj, pnode->index, pnode->sock_id, - pnode->addr_info.ip_addr_str, tid, frame_entry.buf); - } if (transaction_delete(port_obj->transaction, tid) != ESP_OK) { ESP_LOGE(TAG, "Failed to remove queued TID:0x%04" PRIx16, tid); } else { @@ -545,6 +530,18 @@ MB_EVENT_HANDLER(mbs_on_send_data) (void)mb_drv_set_status_flag(drv_obj, MB_FLAG_TRANSACTION_READY); mb_drv_unlock(drv_obj); } else { + // Warn user if master sent a new request before this response was dispatched. + // The response is still sent normally — this only indicates that the master's + // response timeout is too short for the number of active connections. + uint64_t tick = (transaction_tick_t)transaction_item_get_tick(item); + uint64_t time_div_us = (esp_timer_get_time() - tick); + if (tid != pnode->tid_counter) { + ESP_LOGE(TAG, "%p, " MB_NODE_FMT(", handling time [ms]: %" PRIu64 ", exceeds slave response time in master, TID:0x%04" PRIx16 " != TID:0x%04" PRIx16), + ctx, (int)pnode->index, (int)pnode->sock_id, + pnode->addr_info.ip_addr_str, (time_div_us / 1000), + pnode->tid_counter, tid + ); + } ret = port_write_poll(pnode, frame_entry.buf, frame_entry.len, MB_TCP_SEND_TIMEOUT_MS); mb_drv_lock(drv_obj); if (ret >= 0) { diff --git a/modbus/mb_ports/tcp/port_tcp_slave.h b/modbus/mb_ports/tcp/port_tcp_slave.h index d9d6869d..ca9e2748 100644 --- a/modbus/mb_ports/tcp/port_tcp_slave.h +++ b/modbus/mb_ports/tcp/port_tcp_slave.h @@ -24,7 +24,7 @@ #if (CONFIG_FMB_COMM_MODE_TCP_EN) -#define TRANSACTION_TICKS pdMS_TO_TICKS(50) +#define TRANSACTION_TICKS pdMS_TO_TICKS(20) /** * @brief Modbus slave addr list item for the master diff --git a/modbus/mb_ports/tcp/port_tcp_utils.c b/modbus/mb_ports/tcp/port_tcp_utils.c index fe3ed785..c3d2c269 100644 --- a/modbus/mb_ports/tcp/port_tcp_utils.c +++ b/modbus/mb_ports/tcp/port_tcp_utils.c @@ -245,8 +245,8 @@ int port_read_packet(mb_node_info_t *info_ptr) info_ptr->recv_err = ERR_BUF; temp = mbap_payload_max; // read all remaining data from buffer } - - ret = port_get_buf(info_ptr, &ptemp_buf[MB_TCP_UID], temp, MB_READ_TICK); + // Sequential frame read with minimal timeout to reduce delays + ret = port_get_buf(info_ptr, &ptemp_buf[MB_TCP_UID], temp, 1); if (ret < 0) { info_ptr->recv_err = ret; return ret; diff --git a/modbus/mb_ports/tcp/port_tcp_utils.h b/modbus/mb_ports/tcp/port_tcp_utils.h index fb955540..3ab24688 100644 --- a/modbus/mb_ports/tcp/port_tcp_utils.h +++ b/modbus/mb_ports/tcp/port_tcp_utils.h @@ -75,7 +75,7 @@ extern "C" { #endif #define MB_MDNS_PORT (CONFIG_FMB_TCP_PORT_DEFAULT) -#define MB_READ_TICK (500) +#define MB_READ_TICK (CONFIG_FMB_TCP_READ_TIMEOUT_MS) #define MB_MDNS_QUERY_TIME_MS (2000) #define MB_STR_LEN_HOST 1 // "mb_node_tcp_01" From bff8193b2245c07dbc895231c662bbf5dd090453 Mon Sep 17 00:00:00 2001 From: aleks Date: Tue, 30 Jun 2026 11:56:12 +0200 Subject: [PATCH 2/2] fix: review improvements --- Kconfig | 14 +++--- modbus/mb_ports/tcp/port_tcp_driver.c | 68 +++++++++++++-------------- modbus/mb_ports/tcp/port_tcp_driver.h | 2 +- modbus/mb_ports/tcp/port_tcp_slave.c | 2 +- modbus/mb_ports/tcp/port_tcp_utils.c | 2 +- 5 files changed, 43 insertions(+), 45 deletions(-) diff --git a/Kconfig b/Kconfig index 70b6d4d0..d558c7ad 100644 --- a/Kconfig +++ b/Kconfig @@ -49,21 +49,21 @@ menu "Modbus configuration" The keep-alive time can be configured and should be set based on the actual needs of the application, considering the trade-off between keeping the connection alive and minimizing system load. - config FMB_TCP_SELECT_WAIT_MS - int "Modbus TCP select wait timeout (ms)" + config FMB_TCP_EVENT_WAIT_MS + int "Modbus TCP event wait timeout (ms)" range 10 500 default 50 depends on FMB_COMM_MODE_TCP_EN help - This option represents a maximum time the driver task blocks on select function per loop iteration. - Lower values improve responsiveness when multiple masters are connected - but slightly increase CPU usage. For high-throughput applications with - many concurrent connections, reducing to 10-20 ms may help. + This option represents a maximum time the driver task can block while waiting + for an event during each loop iteration. Lower values allow the system to handle failures + while waiting for socket IO events, though they slightly increase CPU usage. + For high-throughput applications with many concurrent connections, reducing to 10-20 ms may help. config FMB_TCP_READ_TIMEOUT_MS int "Modbus TCP socket read timeout (ms)" range 1 500 - default 150 + default 50 depends on FMB_COMM_MODE_TCP_EN help The option applies to SO_RCVTIMEO when reading the MBAP header from a socket. diff --git a/modbus/mb_ports/tcp/port_tcp_driver.c b/modbus/mb_ports/tcp/port_tcp_driver.c index dc9dc0d6..280de258 100644 --- a/modbus/mb_ports/tcp/port_tcp_driver.c +++ b/modbus/mb_ports/tcp/port_tcp_driver.c @@ -653,46 +653,44 @@ void mb_drv_tcp_task(void *ctx) } } } - { - // socket event is ready, process each socket event - mb_drv_check_suspend_shutdown(ctx); - int curr_fd = 0; - mb_node_info_t *node_ptr = NULL; - while (((node_ptr = mb_drv_get_next_node_from_set(ctx, &curr_fd, &readset)) - && (curr_fd < MB_MAX_FDS))) { - if (FD_ISSET(node_ptr->sock_id, &drv_obj->conn_set)) { - // The data is ready in the socket, read frame and queue - FD_CLR(node_ptr->sock_id, &readset); - int ret = port_read_packet(node_ptr); - if (ret > 0) { - ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame received."), ctx, (int)node_ptr->fd, - (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); - mb_drv_lock(ctx); - node_ptr->recv_time = esp_timer_get_time(); - mb_drv_unlock(ctx); - DRIVER_SEND_EVENT(ctx, MB_EVENT_RECV_DATA, node_ptr->index); - } else if (ret == ERR_TIMEOUT) { - ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame read timeout or closed connection."), ctx, (int)node_ptr->fd, - (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); - } else if (ret == ERR_BUF) { - // After retries a response with incorrect TID received, process failure. - drv_obj->event_cbs.mb_sync_event_cb(drv_obj->event_cbs.port_arg, MB_SYNC_EVENT_RECV_FAIL); - ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame error."), ctx, (int)node_ptr->fd, + // If socket events are ready, process each socket event + mb_drv_check_suspend_shutdown(ctx); + int curr_fd = 0; + mb_node_info_t *node_ptr = NULL; + while (((node_ptr = mb_drv_get_next_node_from_set(ctx, &curr_fd, &readset)) + && (curr_fd < MB_MAX_FDS))) { + if (FD_ISSET(node_ptr->sock_id, &drv_obj->conn_set)) { + // The data is ready in the socket, read frame and queue + FD_CLR(node_ptr->sock_id, &readset); + int ret = port_read_packet(node_ptr); + if (ret > 0) { + ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame received."), ctx, (int)node_ptr->fd, + (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); + mb_drv_lock(ctx); + node_ptr->recv_time = esp_timer_get_time(); + mb_drv_unlock(ctx); + DRIVER_SEND_EVENT(ctx, MB_EVENT_RECV_DATA, node_ptr->index); + } else if (ret == ERR_TIMEOUT) { + ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame read timeout or closed connection."), ctx, (int)node_ptr->fd, + (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); + } else if (ret == ERR_BUF) { + // After retries a response with incorrect TID received, process failure. + drv_obj->event_cbs.mb_sync_event_cb(drv_obj->event_cbs.port_arg, MB_SYNC_EVENT_RECV_FAIL); + ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", frame error."), ctx, (int)node_ptr->fd, + (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); + } else { + if (ret == ERR_CONN) { + ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", connection lost."), ctx, (int)node_ptr->fd, (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); } else { - if (ret == ERR_CONN) { - ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", connection lost."), ctx, (int)node_ptr->fd, - (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str); - } else { - ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", critical read error=%d, errno=%u."), ctx, (int)node_ptr->fd, - (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str, (int)ret, (unsigned)errno); - } - DRIVER_SEND_EVENT(ctx, MB_EVENT_ERROR, node_ptr->index, ret); + ESP_LOGD(TAG, "%p, "MB_NODE_FMT(", critical read error=%d, errno=%u."), ctx, (int)node_ptr->fd, + (int)node_ptr->sock_id, node_ptr->addr_info.ip_addr_str, (int)ret, (unsigned)errno); } + DRIVER_SEND_EVENT(ctx, MB_EVENT_ERROR, node_ptr->index, ret); } - curr_fd++; - mb_drv_check_suspend_shutdown(ctx); } + curr_fd++; + mb_drv_check_suspend_shutdown(ctx); } } } diff --git a/modbus/mb_ports/tcp/port_tcp_driver.h b/modbus/mb_ports/tcp/port_tcp_driver.h index 91992972..42c7eae8 100644 --- a/modbus/mb_ports/tcp/port_tcp_driver.h +++ b/modbus/mb_ports/tcp/port_tcp_driver.h @@ -51,7 +51,7 @@ typedef void (*mb_event_handler_fp)(void *ctx, esp_event_base_t base, int32_t id #define MB_DROP_TRANSACTION_TIME_US (1000UL * (CONFIG_FMB_TCP_KEEP_ALIVE_TOUT_SEC * 2000UL)) // drop after twice keep alive timeout is reasonable #define MB_WAIT_DONE_MS (5000) -#define MB_SELECT_WAIT_MS (CONFIG_FMB_TCP_SELECT_WAIT_MS) +#define MB_SELECT_WAIT_MS (CONFIG_FMB_TCP_EVENT_WAIT_MS) #define MB_TCP_SEND_TIMEOUT_MS (CONFIG_FMB_TCP_SEND_TIMEOUT_MS) #define MB_TCP_EVENT_LOOP_TICK_MS (CONFIG_FMB_TCP_EVENT_LOOP_TICK_MS) diff --git a/modbus/mb_ports/tcp/port_tcp_slave.c b/modbus/mb_ports/tcp/port_tcp_slave.c index e8cd0834..a263ada8 100644 --- a/modbus/mb_ports/tcp/port_tcp_slave.c +++ b/modbus/mb_ports/tcp/port_tcp_slave.c @@ -536,7 +536,7 @@ MB_EVENT_HANDLER(mbs_on_send_data) uint64_t tick = (transaction_tick_t)transaction_item_get_tick(item); uint64_t time_div_us = (esp_timer_get_time() - tick); if (tid != pnode->tid_counter) { - ESP_LOGE(TAG, "%p, " MB_NODE_FMT(", handling time [ms]: %" PRIu64 ", exceeds slave response time in master, TID:0x%04" PRIx16 " != TID:0x%04" PRIx16), + ESP_LOGW(TAG, "%p, " MB_NODE_FMT(", handling time [ms]: %" PRIu64 ", exceeds slave response time in master, TID:0x%04" PRIx16 " != TID:0x%04" PRIx16), ctx, (int)pnode->index, (int)pnode->sock_id, pnode->addr_info.ip_addr_str, (time_div_us / 1000), pnode->tid_counter, tid diff --git a/modbus/mb_ports/tcp/port_tcp_utils.c b/modbus/mb_ports/tcp/port_tcp_utils.c index c3d2c269..3a1d2280 100644 --- a/modbus/mb_ports/tcp/port_tcp_utils.c +++ b/modbus/mb_ports/tcp/port_tcp_utils.c @@ -246,7 +246,7 @@ int port_read_packet(mb_node_info_t *info_ptr) temp = mbap_payload_max; // read all remaining data from buffer } // Sequential frame read with minimal timeout to reduce delays - ret = port_get_buf(info_ptr, &ptemp_buf[MB_TCP_UID], temp, 1); + ret = port_get_buf(info_ptr, &ptemp_buf[MB_TCP_UID], temp, MB_READ_TICK); if (ret < 0) { info_ptr->recv_err = ret; return ret;