Unclear Macro Defs Between Serial_Slave and Serial_Master

[JarinKurashige]

Checklist

• Checked the issue tracker for similar issues to ensure this is not a duplicate
• Read the documentation to confirm the issue is not addressed there and your configuration is set correctly
• Tested with the latest version to ensure the issue hasn't been fixed

How often does this bug occurs?

always

Expected behavior

I except macros that are named the same to behave the same.

In serial_master.c
#define HOLD_OFFSET(field) ((uint16_t)(offsetof(holding_reg_params_t, field) + 1))

In serial_slave.c
#define HOLD_OFFSET(field) ((uint16_t)(offsetof(holding_reg_params_t, field) >> 1))

Actual behavior (suspected bug)

I am trying to understand why the Master and Slave use slightly different initialization macros. I cannot get my Master to consistently communicate the same message to the Slave due to this weird nuance.

Error logs or terminal output

typedef struct __attribute__((packed)) {
        uint16_t command;
        uint16_t file_name[MAX_FILE_NAME_STRING_LEN];
        uint16_t ack;
} holding_reg_params_t;

typedef struct __attribute__((packed)) {
        uint16_t status;
        uint16_t data_ready;
        uint16_t buffer_size;
        uint16_t chunk_size;
        uint16_t buffer[KB_TO_BYTES(CONFIG_OFFLOAD_BUFFER_SIZE) / sizeof(uint16_t)];
} input_reg_params_t;

In master:
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_HOLD_REG_START(command),       TEST_HOLD_REG_SIZE(command),            HOLD_OFFSET(command));    
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_HOLD_REG_START(file_name),     TEST_HOLD_REG_SIZE(file_name),          HOLD_OFFSET(file_name));
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_HOLD_REG_START(ack),           TEST_HOLD_REG_SIZE(ack),                HOLD_OFFSET(ack));
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_INPUT_REG_START(status),       TEST_INPUT_REG_SIZE(status),            INPUT_OFFSET(status));   
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_INPUT_REG_START(data_ready),   TEST_INPUT_REG_SIZE(data_ready),        INPUT_OFFSET(data_ready));
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_INPUT_REG_START(buffer_size),  TEST_INPUT_REG_SIZE(buffer_size),       INPUT_OFFSET(buffer_size));
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_INPUT_REG_START(chunk_size),   TEST_INPUT_REG_SIZE(chunk_size),        INPUT_OFFSET(chunk_size));
        ESP_LOGI(TAG, "Start: %zu, size: %zu, offset: %zu", TEST_INPUT_REG_START(buffer),       TEST_INPUT_REG_SIZE(buffer),            INPUT_OFFSET(buffer));

Outputs:
I (491) MASTER_TEST: Start: 0, size: 1, offset: 1
I (491) MASTER_TEST: Start: 1, size: 13, offset: 3
I (491) MASTER_TEST: Start: 14, size: 1, offset: 29
I (501) MASTER_TEST: Start: 0, size: 1, offset: 1
I (501) MASTER_TEST: Start: 1, size: 1, offset: 3
I (511) MASTER_TEST: Start: 2, size: 1, offset: 5
I (511) MASTER_TEST: Start: 3, size: 1, offset: 7

In slave:
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", HOLDING_REG_START_ADDRESS, sizeof(holding_reg_params.command), HOLDING_REG_OFFSET(command));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", HOLDING_REG_START_ADDRESS, sizeof(holding_reg_params.file_name), HOLDING_REG_OFFSET(file_name));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", HOLDING_REG_START_ADDRESS, sizeof(holding_reg_params.ack), HOLDING_REG_OFFSET(ack));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", INPUT_REG_START_ADDRESS, sizeof(input_reg_params.status), INPUT_REG_OFFSET(status));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", INPUT_REG_START_ADDRESS, sizeof(input_reg_params.data_ready), INPUT_REG_OFFSET(data_ready));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", INPUT_REG_START_ADDRESS, sizeof(input_reg_params.buffer_size), INPUT_REG_OFFSET(buffer_size));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", INPUT_REG_START_ADDRESS, sizeof(input_reg_params.chunk_size), INPUT_REG_OFFSET(chunk_size));
        ESP_LOGW(F_TAG, "start: %zu, size: %zu, offset: %zu", INPUT_REG_START_ADDRESS, sizeof(input_reg_params.buffer), INPUT_REG_OFFSET(buffer));

Outputs:
W (1926) MODBUS-TASK: start: 0, size: 2, offset: 0
W (1936) MODBUS-TASK: start: 0, size: 26, offset: 1
W (1946) MODBUS-TASK: start: 0, size: 2, offset: 14
W (1966) MODBUS-TASK: start: 0, size: 2, offset: 0
W (1976) MODBUS-TASK: start: 0, size: 2, offset: 1
W (1986) MODBUS-TASK: start: 0, size: 2, offset: 2
W (1996) MODBUS-TASK: start: 0, size: 2, offset: 3
W (2006) MODBUS-TASK: start: 0, size: 8192, offset: 4

Steps to reproduce the behavior

1. Review the serial examples in the esp-modbus repo

Project release version

Latest

System architecture

Intel/AMD 64-bit (modern PC, older Mac)

Operating system

Linux

Operating system version

Ubuntu 24.04

Shell

Bash

Additional context

No response

[JarinKurashige]

I currently have the master looping to write:

                req = (mb_param_request_t){
                        .slave_addr     = MB_DEVICE_ADDR1,
                        .command        = 0x06,
                        .reg_start      = 0x0000,
                        .reg_size       = 1
                };

                req = (mb_param_request_t){
                        .slave_addr     = MB_DEVICE_ADDR1,
                        .command        = 0x06,
                        .reg_start      = 0x0008,
                        .reg_size       = 1
                };

                req = (mb_param_request_t){
                        .slave_addr     = MB_DEVICE_ADDR1,
                        .command        = 0x06,
                        .reg_start      = 0x0000,
                        .reg_size       = 1
                };

                req = (mb_param_request_t){
                        .slave_addr     = MB_DEVICE_ADDR1,
                        .command        = 0x06,
                        .reg_start      = 0x0008,
                        .reg_size       = 1
                };

W (10440) MODBUS-TASK: 0x3c0b4e98   00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
W (10450) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (10460) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e98, mb_offset: 0
W (12430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (12430) MODBUS-TASK: 0x3c0b4e98   00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
W (12440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (12450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (17430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (17430) MODBUS-TASK: 0x3c0b4e98   01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
W (17440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (17450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (19430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (19430) MODBUS-TASK: 0x3c0b4e98   01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
W (19440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (19450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e98, mb_offset: 0
W (39430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (39430) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (39440) MODBUS-TASK: 0x3c0b4ea8   01 00                                             |..|
W (39450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (41430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (41430) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (41440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (41450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (46430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (46430) MODBUS-TASK: 0x3c0b4e98   01 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (46440) MODBUS-TASK: 0x3c0b4ea8   01 00                                             |..|
W (46450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e9a, mb_offset: 1
W (48430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (48430) MODBUS-TASK: 0x3c0b4e98   01 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (48440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (48450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e98, mb_offset: 0
W (68430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (68440) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (68440) MODBUS-TASK: 0x3c0b4ea8   01 00                                             |..|
W (68450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (70430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (70430) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (70440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (70450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (75430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (75430) MODBUS-TASK: 0x3c0b4e98   01 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (75440) MODBUS-TASK: 0x3c0b4ea8   01 00                                             |..|
W (75450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e98, mb_offset: 0
W (77430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (77430) MODBUS-TASK: 0x3c0b4e98   01 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (77440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (77450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (97430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (97440) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (97440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (97450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4ea8, mb_offset: 8
W (99430) : [             ./components/modbus/modbus_task.c:362] [              MODBUS-TASK] Holding regs
W (99430) MODBUS-TASK: 0x3c0b4e98   00 00 56 5f 30 30 30 32  32 30 2e 41 56 49 00 00  |..V_000220.AVI..|
W (99440) MODBUS-TASK: 0x3c0b4ea8   00 00                                             |..|
W (99450) : [             ./components/modbus/modbus_task.c:120] [     _proceess-reg-update] We received: Address: 0x3c0b4e9a, mb_offset: 1

So I have no idea how the slave is receiving a offset of 1, let alone the offsets being in an order that I do not expect.

[JarinKurashige]

I have no idea how I found the issue, but it is related to the mbc_slave_check_event function. You can supposedly give it any number of event bit mask values, but the function completely ignores whatever bitmask you give it. Right now it is best to let all values through and then handle your own logic to pick out the event bits you want to handle rather than having the driver do it for you.

[JarinKurashige]

I stand corrected, I understand what is happening. mbc_slave_check_event is running properly, but it doesn't filter out items passed into the mbc_slave_get_param_info function. You would have to iterate over mbc_slave_get_param_info until you match your filter.

This could actually lead to undefined and code breaking behavior. If the slave is only waiting on WRITE events, but the master decides to do: READ, READ, ..., READ for the size of the modbus event queue, it will overflow the queue and crash

[JarinKurashige]

Could there please be clearer documentation on how to use the filter and maybe a more robust example? Thank you!

[alisitsyn]

@JarinKurashige , thank you for the issue and the questions related to examples.

#define HOLD_OFFSET(field) ((uint16_t)(offsetof(holding_reg_params_t, field) + 1))
I am trying to understand why the Master and Slave use slightly different initialization macros. I cannot get my Master to consistently communicate the same message to the Slave due to this weird nuance.

The functionality of the macro HOLD_OFFSET(field) is different for the master and slave code. The master uses this macro to get byte offset in the mapped holding register structure plus one. The slave has the modified macro which gets the offset in registers required for registration of mapped area. I can agree that it is better to add prefixes for each macro to clarify the difference which might not be clear for users.

I have no idea how I found the issue, but it is related to the mbc_slave_check_event function. You can supposedly give it any number of event bit mask values, but the function completely ignores whatever bitmask you give it.

The slave task calls the mbc_slave_check_event() function which will block until the Modbus master accesses the slave with specific events defined as a event group. The slave task can then get information about the data being accessed. This function returns the bits that correspond to specific events been activated.
The filtering functionality is not realized to capture only expected events, clear all others in the queue and can be fixed as part of this issue. However, this does not prevent to filter and process the events correctly in your event task.

Please refer to: Configuring Slave Data Access

You would have to iterate over mbc_slave_get_param_info until you match your filter.

Once the events happened, your task needs to iterate through the events and handle access to each parameter in your structure.

Example to get event when holding or input registers accessed in the slave.

Please refer to: event processing task example

This could actually lead to undefined and code breaking behavior. If the slave is only waiting on WRITE events, but the master decides to do: READ, READ, ..., READ for the size of the modbus event queue, it will overflow the queue and crash

This is not so clear but in your task you need to handle all events for your slave and reset the queues for unsupported events.
The documentation can be updated as well as filtering behavior.

So I have no idea how the slave is receiving a offset of 1, let alone the offsets being in an order that I do not expect.

Please provide the example(s) which can allow to reproduce the issue you observed. Otherwise, I can only guess.

Could there please be clearer documentation on how to use the filter and maybe a more robust example? Thank you!

Please refer to: the example to filter the events

The example macro like below can be used to check if the master accesses specific fields:

#define HOLD_OFFSET(field) ((uint16_t)(offsetof(holding_reg_params_t, field) >> 1))
#define INPUT_OFFSET(field) ((uint16_t)(offsetof(input_reg_params_t, field) >> 1))
#define GET_FIELD_OFFSET(field) ((uint16_t)(offsetof(holding_reg_params_t, field)) + 1)

// The macro checks if the address offset is in range
#define GET_ADDR_OFFSET(field_addr) ( \
(((uint32_t)field_addr >= (uint32_t)&holding_reg_params) && \
((uint32_t)field_addr <= ((uint32_t)&holding_reg_params + sizeof(holding_reg_params)) ) ) ? \
                        (((uint32_t)field_addr - (uint32_t)&holding_reg_params) + 1) : 0 \
)

// checks if the specific field in the structure is accessed and assign the value accordingly.
#define IS_FIELD_IN_RANGE(start, size, field, val) ( __extension__( { \
bool in_range = (GET_ADDR_OFFSET(start) <= GET_FIELD_OFFSET(field)) && \
                                                (GET_ADDR_OFFSET((uint32_t)start + (size << 1) - 1) >= GET_FIELD_OFFSET(field)); \
                                                val = in_range ? holding_reg_params.field : 0; in_range; \
}) )

static void slave_operation_func(void *arg)
{
...
for(;holding_reg_params.holding_data0 < MB_CHAN_DATA_MAX_VAL;) {
        // Check for read/write events of Modbus master for certain events
        //mb_event_group_t event = mbc_slave_check_event(MB_READ_WRITE_MASK);
        // Get parameter information from parameter queue
        mbc_slave_get_param_info(&reg_info, portMAX_DELAY);
        const char* rw_str = (reg_info.type & MB_READ_MASK) ? "READ" : "WRITE";
        // Filter events and process them accordingly
        if (reg_info.type & (MB_EVENT_HOLDING_REG_WR | MB_EVENT_HOLDING_REG_RD)) { // 
            ESP_LOGI(TAG, "HOLDING %s (%u us), ADDR:%u, TYPE:%u, INST_ADDR:0x%.4x, SIZE:%u",
                    rw_str,
                    (uint32_t)reg_info.time_stamp,
                    (uint32_t)reg_info.mb_offset,
                    (uint32_t)reg_info.type,
                    (uint32_t)reg_info.address,
                    (uint32_t)reg_info.size);
            // a variable that keeps the value of the action when being accessed
            
            __attribute__((unused)) uint32_t value32 = *(uint32_t*)reg_info.address;
            
            if (IS_FIELD_IN_RANGE(reg_info.address, reg_info.size, holding_data0, value32)) {
                if (reg_info.type & MB_EVENT_HOLDING_REG_RD) {
                    float temperature = get_temperature_data(false); // convert to fahrenheit
                    mbc_slave_lock(ctx);
                    holding_reg_params.holding_data0 = temperature;
                    mbc_slave_unlock(ctx); 
                    disp_printf(0, 16, 16, "Temperature:%.1f", temperature);
                } // only read access is handled
            } 
....
}

[alisitsyn]

@JarinKurashige , why do you close the issue ? You don't need this to be addressed? Please provide the feedback otherwise. Thanks.

[JarinKurashige]

Hi @alisitsyn, I understand the caveats of the current logic. There aren't necessarily logic holes as long as you understand the logic is all.

[alisitsyn]

@JarinKurashige , the original filtering logic was postponed on some stage. I agree some improvement is required to address this.