From 1af46133faabd4bbc4177227b1f5b8c5c2066338 Mon Sep 17 00:00:00 2001 From: aleks Date: Fri, 29 May 2026 07:54:15 +0200 Subject: [PATCH] fix: master handling of broadcast request --- .../mb_objects/functions/mbfunccoils_master.c | 33 ++++++++-- .../mb_objects/functions/mbfuncdisc_master.c | 15 ++++- .../functions/mbfuncholding_master.c | 61 ++++++++++++------- .../mb_objects/functions/mbfuncinput_master.c | 16 ++++- modbus/mb_objects/mb_master.c | 26 +++----- test_apps/test_common/test_common.c | 11 +++- 6 files changed, 111 insertions(+), 51 deletions(-) diff --git a/modbus/mb_objects/functions/mbfunccoils_master.c b/modbus/mb_objects/functions/mbfunccoils_master.c index ced88e2..b975eb7 100644 --- a/modbus/mb_objects/functions/mbfunccoils_master.c +++ b/modbus/mb_objects/functions/mbfunccoils_master.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: BSD-3-Clause * - * SPDX-FileContributor: 2020-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2020-2026 Espressif Systems (Shanghai) CO LTD */ /* * FreeModbus Library: A portable Modbus implementation for Modbus ASCII/RTU. @@ -85,12 +85,20 @@ mb_exception_t mb_error_to_exception(mb_err_enum_t error_code); mb_err_enum_t mbm_rq_read_coils(mb_base_t *inst, uint8_t snd_addr, uint16_t coil_addr, uint16_t coil_num, uint32_t tout) { uint8_t *mb_frame_ptr; + if (snd_addr > MB_ADDRESS_MAX) { return MB_EINVAL; } + + /* The broadcast coils read request is not allowed. */ + if (!snd_addr) { + return MB_ENOREG; + } + if (!mb_port_event_res_take(inst->port_obj, tout)) { return MB_EBUSY; } + inst->get_send_buf(inst, &mb_frame_ptr); inst->set_dest_addr(inst, snd_addr); @@ -118,9 +126,12 @@ mb_exception_t mbm_fn_read_coils(mb_base_t *inst, uint8_t *frame_ptr, uint16_t * mb_exception_t status = MB_EX_NONE; mb_err_enum_t reg_status = MB_EILLFUNC; - /* If this request is broadcast, and it's read mode. This request don't need execute. */ + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + if (inst->transp_obj->frm_is_bcast(inst->transp_obj)) { - status = MB_EX_NONE; + status = MB_EX_ILLEGAL_DATA_ADDRESS; } else if (*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READ_SIZE_MIN) { inst->get_send_buf(inst, &mb_frame_ptr); @@ -215,7 +226,12 @@ mb_exception_t mbm_fn_write_coil(mb_base_t *inst, uint8_t *frame_ptr, uint16_t * (void)inst; (void)frame_ptr; - if (*len_buf == (MB_PDU_FUNC_WRITE_SIZE + MB_PDU_SIZE_MIN)) { + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + + if (*len_buf == (MB_PDU_FUNC_WRITE_SIZE + MB_PDU_SIZE_MIN) + || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { status = MB_EX_NONE; } else { /* Can't be a valid write coil register request because the length @@ -298,8 +314,13 @@ mb_exception_t mbm_fn_write_multi_coils(mb_base_t *inst, uint8_t *frame_ptr, uin uint8_t byte_cnt_verify; mb_err_enum_t reg_status = MB_EILLFUNC; - /* If this request is broadcast, the *len_buf is not need check. */ - if ((*len_buf == MB_PDU_FUNC_WRITE_MUL_SIZE) || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + + /* If this request is broadcast, do not need to call r/w callback. */ + if ((*len_buf == MB_PDU_FUNC_WRITE_MUL_SIZE) + || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { inst->get_send_buf(inst, &mb_frame_ptr); reg_address = (uint16_t)(frame_ptr[MB_PDU_FUNC_WRITE_MUL_ADDR_OFF] << 8); reg_address |= (uint16_t)(frame_ptr[MB_PDU_FUNC_WRITE_MUL_ADDR_OFF + 1]); diff --git a/modbus/mb_objects/functions/mbfuncdisc_master.c b/modbus/mb_objects/functions/mbfuncdisc_master.c index 297363c..9e8d9ba 100644 --- a/modbus/mb_objects/functions/mbfuncdisc_master.c +++ b/modbus/mb_objects/functions/mbfuncdisc_master.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: BSD-3-Clause * - * SPDX-FileContributor: 2020-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2020-2026 Espressif Systems (Shanghai) CO LTD */ /* * FreeModbus Library: A portable Modbus implementation for Modbus ASCII/RTU. @@ -70,6 +70,12 @@ mb_err_enum_t mbm_rq_read_discrete_inputs(mb_base_t *inst, uint8_t snd_addr, uin if (!inst || (snd_addr > MB_ADDRESS_MAX)) { return MB_EINVAL; } + + /* The broadcast discrete read request is nt supported. */ + if (!snd_addr) { + return MB_ENOREG; + } + if (!mb_port_event_res_take(inst->port_obj, tout)) { return MB_EBUSY; } @@ -99,8 +105,13 @@ mb_exception_t mbm_fn_read_discrete_inputs(mb_base_t *inst, uint8_t *frame_ptr, mb_exception_t status = MB_EX_NONE; mb_err_enum_t reg_status = MB_EILLFUNC; + + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + if (inst->transp_obj->frm_is_bcast(inst->transp_obj)) { - status = MB_EX_NONE; + status = MB_EX_ILLEGAL_DATA_ADDRESS; } else if (*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READ_SIZE_MIN) { inst->get_send_buf(inst, &mb_frame_ptr); reg_address = (uint16_t)(mb_frame_ptr[MB_PDU_REQ_READ_ADDR_OFF] << 8); diff --git a/modbus/mb_objects/functions/mbfuncholding_master.c b/modbus/mb_objects/functions/mbfuncholding_master.c index 998648b..f3eecb9 100644 --- a/modbus/mb_objects/functions/mbfuncholding_master.c +++ b/modbus/mb_objects/functions/mbfuncholding_master.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: BSD-3-Clause * - * SPDX-FileContributor: 2020-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2020-2026 Espressif Systems (Shanghai) CO LTD */ /* * FreeModbus Library: A portable Modbus implementation for Modbus ASCII/RTU. @@ -86,7 +86,7 @@ mb_exception_t mb_error_to_exception(mb_err_enum_t error_code); /** * This function will request write holding register. * - * @param snd_addr salve address + * @param snd_addr slave address * @param reg_addr register start address * @param reg_data register data to be written * @param timeout timeout (-1 will waiting forever) @@ -113,7 +113,7 @@ mb_err_enum_t mbm_rq_write_holding_reg(mb_base_t *inst, uint8_t snd_addr, uint16 mb_frame_ptr[MB_PDU_REQ_WRITE_VALUE_OFF] = reg_data >> 8; mb_frame_ptr[MB_PDU_REQ_WRITE_VALUE_OFF + 1] = reg_data; - inst->set_send_len(inst, MB_PDU_SIZE_MIN + MB_PDU_REQ_READ_SIZE); + inst->set_send_len(inst, MB_PDU_SIZE_MIN + MB_PDU_REQ_WRITE_SIZE); (void)mb_port_event_post(inst->port_obj, EVENT(EV_FRAME_TRANSMIT | EV_TRANS_START)); return mb_port_event_wait_req_finish(inst->port_obj); @@ -124,7 +124,14 @@ mb_exception_t mbm_fn_write_holding_reg(mb_base_t *inst, uint8_t *frame_ptr, uin mb_exception_t status = MB_EX_NONE; mb_err_enum_t reg_status = MB_EILLFUNC; - if (*len_buf == (MB_PDU_SIZE_MIN + MB_PDU_FUNC_WRITE_SIZE)) { + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + + /* For broadcast request do not check the buffer size. */ + if (*len_buf == (MB_PDU_SIZE_MIN + MB_PDU_FUNC_WRITE_SIZE) + || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + ESP_LOGD(__func__, "Length: %u", *len_buf); uint16_t reg_address; reg_address = (uint16_t)(frame_ptr[MB_PDU_FUNC_WRITE_ADDR_OFF] << 8); reg_address |= (uint16_t)(frame_ptr[MB_PDU_FUNC_WRITE_ADDR_OFF + 1]); @@ -152,7 +159,7 @@ mb_exception_t mbm_fn_write_holding_reg(mb_base_t *inst, uint8_t *frame_ptr, uin /** * This function will request write multiple holding register. * - * @param snd_addr salve address + * @param snd_addr slave address * @param reg_addr register start address * @param reg_num register total number * @param data_ptr data to be written @@ -207,8 +214,13 @@ mb_exception_t mbm_fn_write_multi_holding_reg(mb_base_t *inst, uint8_t *frame_pt uint16_t byte_count; mb_err_enum_t reg_status = MB_EILLFUNC; - /* If this request is broadcast, the *len_buf is not need check. */ - if ((*len_buf == MB_PDU_SIZE_MIN + MB_PDU_FUNC_WRITE_MUL_SIZE) || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + + if ((*len_buf == MB_PDU_SIZE_MIN + MB_PDU_FUNC_WRITE_MUL_SIZE) + || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + ESP_LOGD(__func__, "Length: %u", *len_buf); inst->get_send_buf(inst, &mb_frame_ptr); reg_address = (uint16_t)(mb_frame_ptr[MB_PDU_REQ_WRITE_MUL_ADDR_OFF] << 8); reg_address |= (uint16_t)(mb_frame_ptr[MB_PDU_REQ_WRITE_MUL_ADDR_OFF + 1]); @@ -245,7 +257,7 @@ mb_exception_t mbm_fn_write_multi_holding_reg(mb_base_t *inst, uint8_t *frame_pt /** * This function will request read holding register. * - * @param snd_addr salve address + * @param snd_addr slave address * @param reg_addr register start address * @param reg_num register total number * @param timeout timeout (-1 will waiting forever) @@ -259,6 +271,12 @@ mb_err_enum_t mbm_rq_read_holding_reg(mb_base_t *inst, uint8_t snd_addr, uint16_ if (!inst || (snd_addr > MB_ADDRESS_MAX)) { return MB_EINVAL; } + + /* The broadcast read holding registers request is not supported. */ + if (!snd_addr) { + return MB_ENOREG; + } + if (!mb_port_event_res_take(inst->port_obj, tout)) { return MB_EBUSY; } @@ -287,14 +305,11 @@ mb_exception_t mbm_fn_read_holding_reg(mb_base_t *inst, uint8_t *frame_ptr, uint uint16_t reg_count; mb_err_enum_t reg_status = MB_EILLFUNC; - /* If this request is broadcast, and it's read mode. This request don't need execute. */ - bool is_broadcast = false; - - is_broadcast = inst->transp_obj->frm_is_bcast(inst->transp_obj); - - if (is_broadcast == true) { - status = MB_EX_NONE; - } else if (*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READ_SIZE_MIN) { + ESP_LOGD(__func__, "Buffer length: %u, bcast: %u", *len_buf, (unsigned)inst->transp_obj->frm_is_bcast(inst->transp_obj)); + ESP_LOG_BUFFER_HEX_LEVEL(__func__, (void *)frame_ptr, *len_buf, ESP_LOG_DEBUG); + if (inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + status = MB_EX_ILLEGAL_DATA_ADDRESS; + } else if ((*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READ_SIZE_MIN)) { inst->get_send_buf(inst, &mb_frame_ptr); reg_address = (uint16_t)(mb_frame_ptr[MB_PDU_REQ_READ_ADDR_OFF] << 8); reg_address |= (uint16_t)(mb_frame_ptr[MB_PDU_REQ_READ_ADDR_OFF + 1]); @@ -332,7 +347,7 @@ mb_exception_t mbm_fn_read_holding_reg(mb_base_t *inst, uint8_t *frame_ptr, uint /** * This function will request read and write holding register. * - * @param snd_addr salve address + * @param snd_addr slave address * @param rd_reg_addr read register start address * @param rd_reg_num read register total number * @param data_ptr data to be written @@ -393,10 +408,14 @@ mb_exception_t mbm_fn_rw_multi_holding_regs(mb_base_t *inst, uint8_t *frame_ptr, uint16_t reg_rd_address, reg_wr_address; uint8_t *mb_frame_ptr; - /* If this request is broadcast, and it's read mode. This request don't need execute. */ - if (inst->transp_obj->frm_is_bcast(inst->transp_obj)) { - status = MB_EX_NONE; - } else if (*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READWRITE_SIZE_MIN) { + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + + /* If this request is broadcast, do not check the length */ + if ((*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READWRITE_SIZE_MIN) + || inst->transp_obj->frm_is_bcast(inst->transp_obj)) { + ESP_LOGD(__func__, "Length: %u", *len_buf); inst->get_send_buf(inst, &mb_frame_ptr); reg_rd_address = (uint16_t)(mb_frame_ptr[MB_PDU_REQ_READWRITE_READ_ADDR_OFF] << 8); reg_rd_address |= (uint16_t)(mb_frame_ptr[MB_PDU_REQ_READWRITE_READ_ADDR_OFF + 1]); diff --git a/modbus/mb_objects/functions/mbfuncinput_master.c b/modbus/mb_objects/functions/mbfuncinput_master.c index 15317a6..2243a62 100644 --- a/modbus/mb_objects/functions/mbfuncinput_master.c +++ b/modbus/mb_objects/functions/mbfuncinput_master.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: BSD-3-Clause * - * SPDX-FileContributor: 2020-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2020-2026 Espressif Systems (Shanghai) CO LTD */ /* * FreeModbus Library: A portable Modbus implementation for Modbus ASCII/RTU. @@ -73,9 +73,16 @@ mb_err_enum_t mbm_rq_read_inp_reg(mb_base_t *inst, uint8_t snd_addr, uint16_t re if (!inst || (snd_addr > MB_ADDRESS_MAX)) { return MB_EINVAL; } + + /* The broadcast read input request is not supported. */ + if (!snd_addr) { + return MB_ENOREG; + } + if (!mb_port_event_res_take(inst->port_obj, tout)) { return MB_EBUSY; } + inst->get_send_buf(inst, &mb_frame_ptr); inst->set_dest_addr(inst, snd_addr); @@ -101,9 +108,12 @@ mb_exception_t mbm_fn_read_inp_reg(mb_base_t *inst, uint8_t *frame_ptr, uint16_t uint16_t reg_address; uint8_t *mb_frame_ptr; - /* If this request is broadcast, and it's read mode. This request don't need execute. */ + if (!len_buf || !frame_ptr || !inst) { + return MB_EINVAL; + } + if (inst->transp_obj->frm_is_bcast(inst->transp_obj)) { - status = MB_EX_NONE; + status = MB_EX_ILLEGAL_DATA_ADDRESS; } else if (*len_buf >= MB_PDU_SIZE_MIN + MB_PDU_FUNC_READ_SIZE_MIN) { inst->get_send_buf(inst, &mb_frame_ptr); diff --git a/modbus/mb_objects/mb_master.c b/modbus/mb_objects/mb_master.c index 208bfaa..c9e5bc9 100644 --- a/modbus/mb_objects/mb_master.c +++ b/modbus/mb_objects/mb_master.c @@ -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 */ @@ -486,7 +486,11 @@ static void mbm_set_dest_addr(mb_base_t *inst, uint8_t dest_addr) static uint8_t mbm_get_dest_addr(mb_base_t *inst) { mbm_object_t *mbm_obj = MB_GET_OBJ_CTX(inst, mbm_object_t, base); - return mbm_obj->master_dst_addr; + uint8_t dest_addr = 0; + CRITICAL_SECTION(inst->lock) { + dest_addr = mbm_obj->master_dst_addr; + } + return dest_addr; } void mbm_error_cb_respond_timeout(mb_base_t *inst, uint8_t dest_addr, const uint8_t *pdu_data, uint16_t pdu_length) @@ -517,7 +521,6 @@ mb_err_enum_t mbm_poll(mb_base_t *inst) { mbm_object_t *mbm_obj = MB_GET_OBJ_CTX(inst, mbm_object_t, base);; - uint16_t length; mb_exception_t exception; mb_err_enum_t status = MB_ENOERR; mb_event_t event; @@ -599,25 +602,16 @@ mb_err_enum_t mbm_poll(mb_base_t *inst) if (MB_OBJ(inst->transp_obj)->frm_is_bcast(inst->transp_obj) && ((mbm_obj->cur_mode == MB_RTU) || (mbm_obj->cur_mode == MB_ASCII))) { mbm_obj->rcv_frame = mbm_obj->snd_frame; + mbm_obj->pdu_rcv_len = mbm_obj->pdu_snd_len; + mbm_obj->rcv_addr = mbm_obj->master_dst_addr; } MB_RETURN_ON_FALSE(mbm_obj->rcv_frame, MB_EILLSTATE, TAG, MB_OBJ_FMT", receive buffer initialization fail.", MB_OBJ_PARENT(inst)); ESP_LOGD(TAG, MB_OBJ_FMT":EV_EXECUTE", MB_OBJ_PARENT(inst)); mbm_obj->func_code = mbm_obj->rcv_frame[MB_PDU_FUNC_OFF]; exception = MB_EX_ILLEGAL_FUNCTION; - /* If master request is broadcast, - * the master needs to execute function for all slaves. - */ - if (MB_OBJ(inst->transp_obj)->frm_is_bcast(inst->transp_obj)) { - length = mbm_obj->pdu_snd_len; - for (int j = 1; j <= MB_MASTER_TOTAL_SLAVE_NUM; j++) { - mbm_set_dest_addr(inst, j); - exception = mbm_check_invoke_handler(inst, mbm_obj->func_code, mbm_obj->rcv_frame, &length); - } - } else { - ESP_LOGD(TAG, MB_OBJ_FMT": function (0x%x), invoke handler.", MB_OBJ_PARENT(inst), (int)mbm_obj->func_code); - exception = mbm_check_invoke_handler(inst, mbm_obj->func_code, mbm_obj->rcv_frame, &mbm_obj->pdu_rcv_len); - } + ESP_LOGD(TAG, MB_OBJ_FMT": function (0x%x), addr=%u, invoke handler.", MB_OBJ_PARENT(inst), (int)mbm_obj->func_code, (unsigned)mbm_obj->rcv_addr); + exception = mbm_check_invoke_handler(inst, mbm_obj->func_code, mbm_obj->rcv_frame, &mbm_obj->pdu_rcv_len); /* If master has exception, will send error process event. Otherwise the master is idle.*/ if (exception != MB_EX_NONE) { mb_port_event_set_err_type(MB_OBJ(inst->port_obj), EV_ERROR_EXECUTE_FUNCTION); diff --git a/test_apps/test_common/test_common.c b/test_apps/test_common/test_common.c index f0c8af1..366a741 100644 --- a/test_apps/test_common/test_common.c +++ b/test_apps/test_common/test_common.c @@ -436,7 +436,11 @@ esp_err_t test_common_read_modbus_parameter(void *handle, uint16_t cid, uint16_t esp_err_t err = mbc_master_get_cid_info(handle, cid, ¶m_descriptor); if ((err != ESP_ERR_NOT_FOUND) && (param_descriptor != NULL)) { uint8_t type = 0; - err = mbc_master_get_parameter(handle, cid, (uint8_t *)par_data, &type); + if (param_descriptor->mb_slave_addr) { + err = mbc_master_get_parameter(handle, cid, (uint8_t *)par_data, &type); + } else { // if the address is broadcast, try to read from first slave intentionally + err = mbc_master_get_parameter_with(handle, cid, param_descriptor->mb_slave_addr + 1, (uint8_t *)par_data, &type); + } if (err == ESP_OK) { ESP_LOGI(TAG, "%p, CHAR #%u %s (%s) value = (0x%04x) parameter read successful.", handle, @@ -509,6 +513,7 @@ static void test_master_task(void *arg) for (cycle_counter = 0; cycle_counter <= TEST_TASK_CYCLE_COUNTER; cycle_counter++) { switch (req_type) { case RT_HOLDING_RD: + err = test_common_read_modbus_parameter(mbm_handle, CID_DEV_REG0, &holding_registers[CID_DEV_REG0]); CHECK_PAR_VALUE(CID_DEV_REG0, err, holding_registers[CID_DEV_REG0], TEST_REG_VAL1); @@ -675,7 +680,7 @@ TaskHandle_t test_common_slave_serial_create(mb_communication_info_t *pconfig, u test_common_slave_setup_start(mbs_handle); - if (priority) { + if (!priority) { priority = TEST_TASK_PRIO_SLAVE; } @@ -709,7 +714,7 @@ TaskHandle_t test_common_master_tcp_create(mb_communication_info_t *pconfig, uin TEST_ESP_OK(mbc_master_start(mbm_handle)); ESP_LOGI(TAG, "%p, modbus master start...", mbm_handle) ; - if (priority) { + if (!priority) { priority = TEST_TASK_PRIO_MASTER; }