espressif / esp-modbus

ESP-Modbus - the officially suppported library for Modbus protocol (serial RS485 + TCP over WiFi or Ethernet).
Apache License 2.0
85 stars 46 forks source link

Modbus Master RTU parity errors not detected (IDFGH-9296) #20

Closed nicksenva closed 1 year ago

nicksenva commented 1 year ago

Our device acting as a Modbus master does not appear to check parity values on receive frames from the slave device. In our test setup, the ESP master is configured for even parity and connected to a slave test device which transmits back with odd parity. However, the master does not recognize the parity error and calls to mbc_master_send_request() return without error. This also occurs when the master and slave are configured for even and odd parity, respectively.

Here is a capture of the master sending a valid request frame with even parity: image

And here is the slave's invalid response which is still accepted: image

The only condition in which errors are returned is when an unexpected number of bits is received (i.e. master is configured for no parity bits but slave returns even/odd parity, or the slave returns an additional stop bit).

alisitsyn commented 1 year ago

Hello,

Please send here your sdkconfig file and your code to setup master in order to reproduce the issue. Also please enable the DEBUG log verbosity in your master project then start communication. Attach the log here.

If the master sends the request with even parity, then slave configured with odd parity should not receive the request from master and will not respond. The debug log can give you more information about parity check on master side. I can expect some issues with your configuration of odd parity on master side but even parity should work just fine.

D (25397) MB_MASTER_SERIAL: Timeout occured, processed: 9 bytes
D (25407) MB_PORT_COMMON:  xMBMasterPortSerialGetResponse default
D (25407) MB_PORT_COMMON: eMBMasterRTUReceive: Port enter critical.
D (25417) MB_PORT_COMMON: eMBMasterRTUReceive: Port exit critical
E (25427) MB_PORT_COMMON: Drop incorrect frame, receive_func(4) != send_func(1)
D (25437) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_ERROR_PROCESS
D (25437) MB_PORT_COMMON: vMBMasterErrorCBReceiveData:Callback receive data timeout failure.
D (25447) Err rcv buf: 01 00 08 00 08 
D (25447) MB_PORT_COMMON: eMBMasterWaitRequestFinish: returned event = 0x100
D (25457) MB_CONTROLLER_MASTER: mbc_serial_master_get_parameter: Bad response to get cid(8) = ESP_ERR_INVALID_RESPONSE
E (25467) MB_CONTROLLER_MASTER: mbc_master_get_parameter(73): Master get parameter failure, error=(0x108) (ESP_ERR_INVALID_RESPONSE).
E (25477) MASTER_TEST: Characteristic #8 (RelayP2) read fail, err = 0x108 (ESP_ERR_INVALID_RESPONSE).
D (25797) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25797) MB_MASTER_SERIAL: MB_uart[2] event:
D (25797) MB_MASTER_SERIAL: uart parity error. <<<<<<<<<<<<<<<<<<<< Parity errors detected and frame is not excepted
D (25807) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25807) MB_MASTER_SERIAL: MB_uart[2] event:
D (25817) MB_MASTER_SERIAL: uart parity error.
D (25817) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25827) MB_MASTER_SERIAL: MB_uart[2] event:
D (25827) MB_MASTER_SERIAL: uart parity error.
D (25837) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25837) MB_MASTER_SERIAL: MB_uart[2] event:
D (25847) MB_MASTER_SERIAL: uart parity error.
D (25847) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25857) MB_MASTER_SERIAL: MB_uart[2] event:
D (25857) MB_MASTER_SERIAL: uart parity error.
D (25867) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25867) MB_MASTER_SERIAL: MB_uart[2] event:
D (25877) MB_MASTER_SERIAL: uart parity error.
D (25877) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25887) MB_MASTER_SERIAL: MB_uart[2] event:
D (25887) MB_MASTER_SERIAL: uart parity error.
D (25897) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5 
D (25897) MB_MASTER_SERIAL: MB_uart[2] event:
D (25907) MB_MASTER_SERIAL: uart parity error.
D (25907) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 0 
D (25917) MB_MASTER_SERIAL: MB_uart[2] event:
D (25917) MB_MASTER_SERIAL: Data event, len: 8.
D (25987) MB_PORT_COMMON: xMBMasterRunResTake:Take resource (80) (41 ticks).
D (25987) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_TRANSMIT
D (25987) POLL transmit buffer: 04 00 00 00 02 
D (25987) MB_PORT_COMMON: eMBMasterRTUSend: Port enter critical.
D (25997) MB_PORT_COMMON: eMBMasterRTUSend: Port exit critical
D (25997) MB_PORT_COMMON: xMBMasterPortSerialSendRequest default
D (26007) MB_PORT_COMMON: vMBMasterPortTimersRespondTimeoutEnable Respond enable timeout.

Thank you.

nicksenva commented 1 year ago

Hello,

Here is the isolated Modbus test code from our project.

#include "string.h"
#include "esp_log.h"
#include "mbcontroller.h"
#include "sdkconfig.h"
#include "esp_modbus_common.h"

static const char *TAG = "MASTER_TEST";

#define MB_UART_PORT_NUM 1
#define MB_UART_TXD 32
#define MB_UART_RXD 35
#define MB_UART_RTS 33
#define MB_BAUD 115200
#define MB_STOP_BITS UART_STOP_BITS_1

esp_err_t modbus_get(uint16_t start_reg, uint16_t nRegs, uint16_t reg_type, void* dstBuf) {
    esp_err_t err;
    double slave_addr = 31;
    err = mbc_master_send_request(
        &(mb_param_request_t) {slave_addr, reg_type, start_reg, nRegs}, dstBuf);

    return err;
}

esp_err_t modbus_init() {
    // Initialize and start Modbus controller
    mb_communication_info_t comm = {
            .port = MB_UART_PORT_NUM,
            .mode = MB_MODE_RTU,
            .baudrate = MB_BAUD,
            .parity = UART_PARITY_EVEN
    };
    void* master_handler = NULL;
    esp_err_t err;

    err = mbc_master_init(MB_PORT_SERIAL_MASTER, &master_handler);
    MB_RETURN_ON_FALSE((master_handler != NULL), ESP_ERR_INVALID_STATE, TAG,
                                "mb controller initialization fail.");
    MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
                            "mb controller initialization fail, returns(0x%"PRIu32").",
                            (uint32_t)err);
    err = mbc_master_setup((void*)&comm);
    MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
                            "mb controller setup fail, returns(0x%"PRIu32").",
                            (uint32_t)err);
    err = uart_set_pin(MB_UART_PORT_NUM, MB_UART_TXD, MB_UART_RXD,
                         MB_UART_RTS, UART_PIN_NO_CHANGE);
    MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
            "mb serial set pin failure, uart_set_pin() returned (0x%"PRIu32").", (uint32_t)err);

    err = mbc_master_start();
    MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
                            "mb controller start fail, returns(0x%"PRIu32").",
                            (uint32_t)err);

    // Set driver mode to Half Duplex
    err = uart_set_mode(MB_UART_PORT_NUM, UART_MODE_RS485_HALF_DUPLEX);
    MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
            "mb serial set mode failure, uart_set_mode() returned (0x%"PRIu32").", (uint32_t)err);

    ESP_LOGI(TAG, "Modbus master stack initialized...");

    // Set UART stop bits
   err = uart_set_stop_bits(MB_UART_PORT_NUM, MB_STOP_BITS);
   MB_RETURN_ON_FALSE((err == ESP_OK), ESP_ERR_INVALID_STATE, TAG,
           "mb serial set stop bits failure, uart_set_stop_bits() returned (0x%"PRIu32").", (uint32_t)err);
        return err;

    return err;
}

void app_main(void)
{
    ESP_ERROR_CHECK(modbus_init());
    uint16_t val;
    esp_err_t err;

    uint16_t reg_type = 3;
    uint16_t sta
[sdkconfig.txt](https://github.com/espressif/esp-modbus/files/10572421/sdkconfig.txt)
rt_addr = 1;
    uint16_t nRegs = 1;
    while(1) {
        err = modbus_get(start_addr, nRegs, reg_type, &val);
        if(err == ESP_OK)
            ESP_LOGI(TAG, "Val read: %d", val);
        else
            ESP_LOGE(TAG, "Modbus read failed");

        vTaskDelay(1000 / portTICK_PERIOD_MS);
    }

}

This was the log output for this test with misconfigured parity between master and slave. The UART driver does recognize the parity error, but no error is returned to the top-level function call.

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 271414342, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:2, clock div:2
secure boot v2 enabled
secure boot verification succeeded
load:0x3fff00b8 len:0x2700
load:0x40078000 len:0x5be0
load:0x40080400 len:0xc6c
0x40080400: _init at ??:?

entry 0x400805c0
I (167) cpu_start: Pro cpu up.
I (167) cpu_start: Single core mode
D (175) clk: RTC_SLOW_CLK calibration value: 3467430
I (180) cpu_start: Pro cpu start user code
I (180) cpu_start: cpu freq: 240000000 Hz
I (180) cpu_start: Application information:
I (185) cpu_start: Project name:     modbus_master
I (191) cpu_start: App version:      3c63b5d
I (196) cpu_start: Compile time:     Feb  2 2023 10:37:44
I (202) cpu_start: ELF file SHA256:  8fe7ddd79943753f...
I (208) cpu_start: ESP-IDF:          v5.0-2-g26e2804066
D (214) memory_layout: Checking 6 reserved memory ranges:
D (219) memory_layout: Reserved memory range 0x3ff80000 - 0x3ff80000
D (226) memory_layout: Reserved memory range 0x3ffae000 - 0x3ffae6e0
D (232) memory_layout: Reserved memory range 0x3ffb0000 - 0x3ffb3090
D (238) memory_layout: Reserved memory range 0x3ffe0000 - 0x3ffe0440
D (245) memory_layout: Reserved memory range 0x40070000 - 0x40078000
D (251) memory_layout: Reserved memory range 0x40080000 - 0x4008e328
0x40080000: _WindowOverflow4 at C:/Users/User/workspace/git/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/xtensa_vectors.S:1742

D (258) memory_layout: Building list of available memory regions:
D (264) memory_layout: Available memory region 0x3ffae6e0 - 0x3ffb0000
D (270) memory_layout: Available memory region 0x3ffb3090 - 0x3ffb8000
D (277) memory_layout: Available memory region 0x3ffb8000 - 0x3ffc0000
D (284) memory_layout: Available memory region 0x3ffc0000 - 0x3ffc2000
D (290) memory_layout: Available memory region 0x3ffc2000 - 0x3ffc4000
D (297) memory_layout: Available memory region 0x3ffc4000 - 0x3ffc6000
D (303) memory_layout: Available memory region 0x3ffc6000 - 0x3ffc8000
D (310) memory_layout: Available memory region 0x3ffc8000 - 0x3ffca000
D (317) memory_layout: Available memory region 0x3ffca000 - 0x3ffcc000
D (323) memory_layout: Available memory region 0x3ffcc000 - 0x3ffce000
D (330) memory_layout: Available memory region 0x3ffce000 - 0x3ffd0000
D (336) memory_layout: Available memory region 0x3ffd0000 - 0x3ffd2000
D (343) memory_layout: Available memory region 0x3ffd2000 - 0x3ffd4000
D (350) memory_layout: Available memory region 0x3ffd4000 - 0x3ffd6000
D (356) memory_layout: Available memory region 0x3ffd6000 - 0x3ffd8000
D (363) memory_layout: Available memory region 0x3ffd8000 - 0x3ffda000
D (369) memory_layout: Available memory region 0x3ffda000 - 0x3ffdc000
D (376) memory_layout: Available memory region 0x3ffdc000 - 0x3ffde000
D (383) memory_layout: Available memory region 0x3ffde000 - 0x3ffe0000
D (389) memory_layout: Available memory region 0x3ffe0440 - 0x3ffe4000
D (396) memory_layout: Available memory region 0x3ffe4000 - 0x3ffe8000
D (402) memory_layout: Available memory region 0x3ffe8000 - 0x3fff0000
D (409) memory_layout: Available memory region 0x3fff0000 - 0x3fff8000
D (416) memory_layout: Available memory region 0x3fff8000 - 0x3fffc000
D (422) memory_layout: Available memory region 0x3fffc000 - 0x40000000
D (429) memory_layout: Available memory region 0x40078000 - 0x40080000
0x40080000: _WindowOverflow4 at C:/Users/User/workspace/git/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/xtensa_vectors.S:1742

D (435) memory_layout: Available memory region 0x4008e328 - 0x40090000
D (442) memory_layout: Available memory region 0x40090000 - 0x40092000
D (449) memory_layout: Available memory region 0x40092000 - 0x40094000
D (455) memory_layout: Available memory region 0x40094000 - 0x40096000
D (462) memory_layout: Available memory region 0x40096000 - 0x40098000
D (468) memory_layout: Available memory region 0x40098000 - 0x4009a000
D (475) memory_layout: Available memory region 0x4009a000 - 0x4009c000
D (482) memory_layout: Available memory region 0x4009c000 - 0x4009e000
D (488) memory_layout: Available memory region 0x4009e000 - 0x400a0000
D (495) memory_layout: Available memory region 0x3ff80000 - 0x3ff82000
I (501) heap_init: Initializing. RAM available for dynamic allocation:
D (509) heap_init: New heap initialised at 0x3ffae6e0
I (514) heap_init: At 3FFAE6E0 len 00001920 (6 KiB): DRAM
D (520) heap_init: New heap initialised at 0x3ffb3090
I (525) heap_init: At 3FFB3090 len 0002CF70 (179 KiB): DRAM
I (531) heap_init: At 3FFE0440 len 0001FBC0 (126 KiB): D/IRAM
D (538) heap_init: New heap initialised at 0x40078000
I (543) heap_init: At 40078000 len 00008000 (32 KiB): IRAM
D (549) heap_init: New heap initialised at 0x4008e328
I (554) heap_init: At 4008E328 len 00011CD8 (71 KiB): IRAM
D (560) heap_init: New heap initialised at 0x3ff80000
I (565) heap_init: At 3FF80000 len 00002000 (8 KiB): RTCRAM
D (572) intr_alloc: Connected src 46 to int 2 (cpu 0)
D (577) spi_flash: trying chip: issi
D (580) spi_flash: trying chip: gd
D (584) spi_flash: trying chip: mxic
D (587) spi_flash: trying chip: winbond
D (591) spi_flash: trying chip: generic
I (595) spi_flash: detected chip: generic
I (600) spi_flash: flash io: dio
D (604) efuse: In EFUSE_BLK0__DATA0_REG is used 1 bits starting with 2 bit
D (611) efuse: In EFUSE_BLK0__DATA0_REG is used 7 bits starting with 20 bit
W (618) flash_encrypt: Flash encryption mode is DEVELOPMENT (not secure)
D (625) efuse: In EFUSE_BLK0__DATA0_REG is used 1 bits starting with 17 bit
D (632) efuse: In EFUSE_BLK2__DATA0_REG is used 32 bits starting with 0 bit
D (639) efuse: In EFUSE_BLK2__DATA1_REG is used 32 bits starting with 0 bit
D (646) efuse: In EFUSE_BLK2__DATA2_REG is used 32 bits starting with 0 bit
D (653) efuse: In EFUSE_BLK2__DATA3_REG is used 32 bits starting with 0 bit
D (660) efuse: In EFUSE_BLK2__DATA4_REG is used 32 bits starting with 0 bit
D (667) efuse: In EFUSE_BLK2__DATA5_REG is used 32 bits starting with 0 bit
D (674) efuse: In EFUSE_BLK2__DATA6_REG is used 32 bits starting with 0 bit
D (681) efuse: In EFUSE_BLK2__DATA7_REG is used 32 bits starting with 0 bit
D (688) efuse: In EFUSE_BLK0__DATA0_REG is used 1 bits starting with 8 bit
D (695) cpu_start: calling init function: 0x400d1a30
0x400d1a30: esp_reset_reason_init at C:/Users/User/workspace/git/esp-idf/components/esp_system/port/soc/esp32/reset_reason.c:68

D (701) cpu_start: calling init function: 0x400d12e4
0x400d12e4: esp_init_app_elf_sha256 at C:/Users/User/workspace/git/esp-idf/components/esp_app_format/esp_app_desc.c:69

D (706) cpu_start: calling init function: 0x400d3c30 on core: 0
0x400d3c30: __esp_system_init_fn_esp_timer_startup_init at C:/Users/User/workspace/git/esp-idf/components/esp_timer/src/esp_timer.c:464

D (712) intr_alloc: Connected src 17 to int 3 (cpu 0)
D (717) cpu_start: calling init function: 0x400ebae0 on core: 0
0x400ebae0: __esp_system_init_fn_init_components0 at C:/Users/User/workspace/git/esp-idf/components/esp_system/startup.c:477

D (723) intr_alloc: Connected src 24 to int 9 (cpu 0)
I (728) cpu_start: Starting scheduler on PRO CPU.
D (734) heap_init: New heap initialised at 0x3ffe0440
D (734) intr_alloc: Connected src 16 to int 12 (cpu 0)
D (744) MB_PORT_COMMON: vMBPortSetMode: Port enter critical.
D (744) MB_PORT_COMMON: vMBPortSetMode: Port exit critical
D (754) MB_PORT_COMMON: eMBMasterRTUInit: Port enter critical.
I (754) uart: queue free spaces: 20
D (764) intr_alloc: Connected src 35 to int 13 (cpu 0)
D (764) MB_PORT_COMMON: xMBMasterPortSerialInit Init serial.
D (774) MB_PORT_COMMON: eMBMasterRTUInit: Port exit critical
D (774) MB_PORT_COMMON: eMBMasterRTUStart: Port enter critical.
D (784) MB_PORT_COMMON: eMBMasterRTUStart: Port exit critical
I (784) MASTER_TEST: Modbus master stack initialized...
D (794) MB_PORT_COMMON: xMBMasterRunResTake:Take resource (80) (101 ticks).
D (804) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_TRANSMIT
D (804) POLL transmit buffer: 03 00 01 00 01
D (814) MB_PORT_COMMON: eMBMasterRTUSend: Port enter critical.
D (814) MB_PORT_COMMON: eMBMasterRTUSend: Port exit critical
D (824) MB_PORT_COMMON: xMBMasterPortSerialSendRequest default
D (834) MB_PORT_COMMON: vMBMasterPortTimersRespondTimeoutEnable Respond enable timeout.
D (834) MB_MASTER_SERIAL: MB_TX_buffer sent: (9) bytes.
D (844) MB_PORT_COMMON: vMBMasterRxSemaRelease:RX semaphore is free.
D (854) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (854) MB_MASTER_SERIAL: MB_uart[1] event:
D (864) MB_MASTER_SERIAL: uart rx break.
D (864) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (874) MB_MASTER_SERIAL: MB_uart[1] event:
D (874) MB_MASTER_SERIAL: uart rx break.
D (874) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (884) MB_MASTER_SERIAL: MB_uart[1] event:
D (884) MB_MASTER_SERIAL: uart parity error.
D (894) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (894) MB_MASTER_SERIAL: MB_uart[1] event:
D (904) MB_MASTER_SERIAL: uart parity error.
D (904) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (914) MB_MASTER_SERIAL: MB_uart[1] event:
D (914) MB_MASTER_SERIAL: uart parity error.
D (924) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (924) MB_MASTER_SERIAL: MB_uart[1] event:
D (934) MB_MASTER_SERIAL: uart parity error.
D (934) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (944) MB_MASTER_SERIAL: MB_uart[1] event:
D (944) MB_MASTER_SERIAL: uart parity error.
D (954) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (954) MB_MASTER_SERIAL: MB_uart[1] event:
D (964) MB_MASTER_SERIAL: uart parity error.
D (964) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (974) MB_MASTER_SERIAL: MB_uart[1] event:
D (974) MB_MASTER_SERIAL: uart parity error.
D (984) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 0
D (984) MB_MASTER_SERIAL: MB_uart[1] event:
D (994) MB_MASTER_SERIAL: Data event, len: 7.
D (994) MB_MASTER_SERIAL: Received data: 8(bytes in buffer)

D (1004) MB_MASTER_SERIAL: Timeout occured, processed: 8 bytes
D (1004) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_SENT
D (1014) POLL sent buffer: 03 00 01 00 01
D (1014) MB_PORT_COMMON:  xMBMasterPortSerialGetResponse default
D (1024) MB_PORT_COMMON: eMBMasterRTUReceive: Port enter critical.
D (1024) MB_PORT_COMMON: eMBMasterRTUReceive: Port exit critical
D (1034) MB_PORT_COMMON: eMBMasterPoll: Packet data received successfully (0).
D (1044) POLL receive buffer: 03 02 02 d4
D (1044) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_EXECUTE
D (1054) MB_PORT_COMMON: eMBMasterPoll: set event EV_ERROR_OK
D (1054) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_ERROR_PROCESS
D (1064) MB_PORT_COMMON: vMBMasterCBRequestSuccess: Callback request success.
D (1074) MB_PORT_COMMON: eMBMasterWaitRequestFinish: returned event = 0x40
I (1074) MASTER_TEST: Val read: 724

sdkconfig.txt

nicksenva commented 1 year ago

After reviewing the source code we have discovered that the low-level UART driver is detecting the parity error in its receive ISR handler, which sends an event notification to the UART event task. This task has a case statement for logging the error, but it doesn't do anything to handle it. Is there some other location in the code where parity errors are handled, and if so what is the defined behavior? I would expect the error to be propagated and returned to the function sending the request. Is this correct? MicrosoftTeams-image

alisitsyn commented 1 year ago

@nicksenva ,

Thank you for your update and sorry for the delay with answer. The Modbus code should not handle the parity error in the event task. The event task can just catch it to log the error message. The handling is performed in the RTU handling code. If the parity error happens the received frame will not contain the correct CRC this causes the FSM to trigger error handling and the error ESP_ERR_INVALID_RESPONSE at the end.

ohitsderrick commented 1 year ago

@alisitsyn, I think the problem is that it will not fail if the parity is intentionally configured incorrectly (transmitter and receiver have different parity settings). During normal operation when there is no actual corruption, the raw data is correct and passes the CRC, but the parity errors are ignored and the UART driver still passes data to the Modbus layer as if everything is fine. Since parity is part of the Modbus standard, it seems appropriate that the driver should return an error or indicate it in some other way. Does this sound right?

alisitsyn commented 1 year ago

@ohitsderrick,

Yes, this sounds correct in this case, my fail. The frame check is applied regardless of any parity checking method used for the individual characters of the message. The parity check of UART has to be propagated to the error check result.

The patch proposed below is to fix the error and apply parity check: 0001_serial_master_slave_fix_parity_check.patch

These communication errors can also apply later to Serial Line Diagnostic Counters which are not supported for now.

@nicksenva ,

Please check the fix above with your tests. Thanks.

nicksenva commented 1 year ago

Thank you for the initial patch, it did resolve the issue. I will try your updated patch shortly. While testing I discovered a similar issue with misconfigured stop bit settings. Specifically, when the master UART is configured for 2 stop bits and the slave is configured for 1 stop bit, the framing error is not detected (or should this be a break error?).

Here is the log output for this condition. We have noticed that every read request reports there being two UART break events even when no response is returned from the slave. We also noticed that the send/received byte count are both offset by one (9 bytes reported sent in an 8 byte frame, and 8 bytes received in a 7 byte response. Could you explain these discrepancies?

Ultimately we would like any mismatch of parity and stop bit settings to return an error to the request function and to discard the data.

D (52490) MB_PORT_COMMON: xMBMasterRunResTake:Take resource (80) (101 ticks).
D (52490) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_TRANSMIT
D (52500) MB_PORT_COMMON: xMBMasterPortSerialSendRequest default
D (52500) MB_PORT_COMMON: vMBMasterPortTimersRespondTimeoutEnable Respond enable timeout.
D (52510) MB_MASTER_SERIAL: MB_TX_buffer sent: (9) bytes.
D (52520) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (52520) MB_MASTER_SERIAL: MB_uart[1] event:
D (52530) MB_MASTER_SERIAL: uart rx break.
D (52530) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (52540) MB_MASTER_SERIAL: MB_uart[1] event:
D (52540) MB_MASTER_SERIAL: uart rx break.
D (52550) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 0
D (52550) MB_MASTER_SERIAL: MB_uart[1] event:
D (52560) MB_MASTER_SERIAL: Data event, len: 7.
D (52560) MB_MASTER_SERIAL: Received data: 8(bytes in buffer)

D (52570) MB_MASTER_SERIAL: Timeout occured, processed: 8 bytes
D (52570) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_SENT
D (52580) MB_PORT_COMMON:  xMBMasterPortSerialGetResponse default
D (52580) MB_PORT_COMMON: eMBMasterPoll: Packet data received successfully (0).
D (52590) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_EXECUTE
D (52600) MB_PORT_COMMON: eMBMasterPoll: set event EV_ERROR_OK
D (52600) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_ERROR_PROCESS
D (52610) MB_PORT_COMMON: vMBMasterCBRequestSuccess: Callback request success.
D (52610) MB_PORT_COMMON: eMBMasterWaitRequestFinish: returned event = 0x40
nicksenva commented 1 year ago

The patch appears to work as intended after a few minutes of testing, here is a capture showing the behavior with a parity mismatch. For now we will continue to use the buffer reset as a quick fix until an official patch is made.

D (703610) MB_PORT_COMMON: xMBMasterRunResTake:Take resource (80) (101 ticks).
D (703620) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_TRANSMIT
D (703630) MB_PORT_COMMON: xMBMasterPortSerialSendRequest default
D (703630) MB_PORT_COMMON: vMBMasterPortTimersRespondTimeoutEnable Respond enable timeout.
D (703640) MB_MASTER_SERIAL: MB_TX_buffer sent: (9) bytes.
D (703650) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (703650) MB_MASTER_SERIAL: MB_uart[1] event:
D (703660) MB_MASTER_SERIAL: uart rx break.
D (703660) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 1
D (703670) MB_MASTER_SERIAL: MB_uart[1] event:
D (703670) MB_MASTER_SERIAL: uart rx break.
D (703680) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703680) MB_MASTER_SERIAL: MB_uart[1] event:
D (703690) MB_MASTER_SERIAL: uart parity error, count: 14.
D (703690) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703700) MB_MASTER_SERIAL: MB_uart[1] event:
D (703700) MB_MASTER_SERIAL: uart parity error, count: 15.
D (703710) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703720) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703720) MB_MASTER_SERIAL: MB_uart[1] event:
D (703730) MB_MASTER_SERIAL: uart parity error, count: 16.
D (703730) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703740) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703750) MB_MASTER_SERIAL: MB_uart[1] event:
D (703750) MB_MASTER_SERIAL: uart parity error, count: 17.
D (703760) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703760) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703770) MB_MASTER_SERIAL: MB_uart[1] event:
D (703770) MB_MASTER_SERIAL: uart parity error, count: 18.
D (703780) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703790) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703790) MB_MASTER_SERIAL: MB_uart[1] event:
D (703800) MB_MASTER_SERIAL: uart parity error, count: 19.
D (703800) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703810) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 5
D (703820) MB_MASTER_SERIAL: MB_uart[1] event:
D (703820) MB_MASTER_SERIAL: uart parity error, count: 20.
D (703830) MB_MASTER_SERIAL: xMBMasterPortRxSemaTake: RX semaphore take failure.
D (703830) MB_PORT_COMMON: xMBPortSerialWaitEvent, UART event: 0
D (703840) MB_MASTER_SERIAL: MB_uart[1] event:
D (703850) MB_MASTER_SERIAL: Data event, len: 7.
D (703850) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_FRAME_SENT
D (703860) MB_PORT_COMMON: eMBMasterPoll:EV_MASTER_ERROR_PROCESS
D (703860) MB_PORT_COMMON: vMBMasterErrorCBReceiveData:Callback receive data timeout failure.
D (703870) MB_PORT_COMMON: eMBMasterWaitRequestFinish: returned event = 0x100
E (703880) MB_CONTROLLER_MASTER: mbc_master_send_request(93): Master send request failure error=(0x108) (ESP_ERR_INVALID_RESPONSE).

On a side note, our esp-modbus component is in our project's components folder rather than the managed components so we can make modifications as necessary. We would like to clone the GitHub repo so we can pull the latest releases, but the release tags in the component registry aren't present on the Git repo. Are there any plans to keep these tags in sync?

alisitsyn commented 1 year ago

@ nicksenva ,

Thank you for the test results.

Here is the log output for this condition. We have noticed that every read request reports there being two UART break events even when no response is returned from the slave. We also noticed that the send/received byte count are both offset by one (9 bytes reported sent in an 8 byte frame, and 8 bytes received in a 7 byte response. Could you explain these discrepancies?

The break events are related to hardware issue in UART working in RS485 mode. On the break event the RX FIFO can receive one zero byte in this case which is filtered out. The debug messages shows the offset in the buffer instead of the length of the message and are misleading. It is related to workaround for UART issues.

Specifically, when the master UART is configured for 2 stop bits and the slave is configured for 1 stop bit, the framing error is not detected (or should this be a break error?).

I think the break events are related to situation described above but not related to incorrect framing detection. For example: I the master is configured for 1 stop it won't have any issue for receiver - should the slave be configured for 2 stop bits you will already completely receive each character after 1 stop bit and the second will simply be an additional 1 bit period pause (IDLE). The issues can happen If the master is configured to 1 stop and slave to 2 stop bits when you send two characters "immediately" after another and the start bit of the second character is sent after just 1 stop bit time, while the slave is still waiting for the second stop bit to arrive. In the worst case the slave's receiver will see your start bit as an invalid second stop bit and then also miss the start bit and hence the next character will be corrupted. However the slave, while working in 2 stop bit mode may also simply accept that there was a fast new start bit and so not have any problems. Therefore what actually happens is undefined. The UART driver does use the interrupt instead of DMA and does not detect any frame error in this case as per my test results. This can be investigated more but I would not say it is a critical discrepancy. As I know many Modbus devices do not detect 1 stop instead of 2 stop bits. If you disagree with this I would propose you to create new issue against UART driver in esp-idf. The issue with break conditions are reported to digital team but will need time for investigation. Also, it is better to create the separate esp-idf issue for this behavior as well to be able to track it appropriately.

would like to clone the GitHub repo so we can pull the latest releases, but the release tags in the component registry aren't present on the Git repo. Are there any plans to keep these tags in sync?

The esp-modbus project rules include tagging of major updates only, this is why the latest updates do not contain the tags. Not an issue, I can apply the tags and sync them with github repo. Tags

alisitsyn commented 1 year ago

The simplified fix has been implemented in commit 15e24c16b9aaed9ef05362a1ca47193d163228f0. The issue will be closed. Feel free to reopen the issue. The separate issue will be tracked as per esp-idf IDFGH-9386.