siemens / meta-iot2050

SIMATIC IOT2050 Isar/Debian Board Support Package
MIT License
129 stars 76 forks source link

Arduino UART_RTS/UART_CTS abnormal #461

Closed andyliao5555 closed 11 months ago

andyliao5555 commented 1 year ago

I made an OS by myself, which is V01.03.01-213-g7ffd195, and I found that the behavior of RTS/CTS was the same as previous version. Actually I wrote a section of code to verify the RTS CTS functions. I connected the IO0-IO1 as the UART rx-UART tx, and connected the IO2-IO3 as the UART cts-UART rts. For example: On V01.03.01-213-g7ffd195: ./b.out on root@iot2050-debian:/# ./b.out on [ 8501.118117] pinctrl-single 4301c000.pinctrl: invalid function d1-uart0-ctsn in map table [DEBUG]turn:0, write 1024 success, total 1024 send [DEBUG]turn:1, write 1024 success, total 2048 send [DEBUG]turn:2, write 1024 success, total 3072 send [DEBUG]turn:3, write 1024 success, total 4096 send ^C root@iot2050-debian:/# AVS@[1100 1160 1160]

same code and setup, On V01.03.01.01-0-g845a732: ./b.out on root@iot2050-debian:/# gcc mraa_uart_test.c -o b.out -lmraa root@iot2050-debian:/# ./b.out on [DEBUG]turn:0, write 1024 success, total 1024 send [DEBUG]turn:1, write 1024 success, total 2048 send [DEBUG]turn:2, write 1024 success, total 3072 send [DEBUG]turn:3, write 1024 success, total 4096 send [DEBUG]turn:4, write 1024 success, total 5120 send [DEBUG]turn:5, write 1024 success, total 6144 send [DEBUG]turn:6, write 1024 success, total 7168 send ^C root@iot2050-debian:/#

I noticed two abnormalities from this two cases: 1). previous version g845a732 had no error messages from kernel, but g7ffd195 had pinctrl errors from kernel side. 2). I made totally 20 times send data in my code, above 2 cases, the sending will stop when I enabled the CTS/RTS function, but obviously, I can only send 4 buffers of data on g7ffd195 instead of 7 buffers on g845a732, if its related to the error from the pinctrl subsystem?

andyliao5555 commented 1 year ago

I will paste my code here:

include

include

include

include

include

include

include "mraa/uart.h"

define SEND_LEN 1024

define TEST_SUCCESS 0

define TEST_FAILED -1

define UART_PORT 0

define UART_BAUD 9600

define SEND_COUNT 20

typedef enum { TEST_FALSE = 0, TEST_TRUE = 1, } test_bool;

int main(int argc, char *argv[]) { if (argc != 2) { printf ("param error, please refer to the usage below:\n"); printf ("###############################\n"); printf ("#./a.out [arg] #\n"); printf ("#arg-on: enable RTS/CTS #\n"); printf ("#arg-off:disable RTS/CTS #\n"); printf ("###############################\n"); return TEST_FAILED; }

char *str = argv[1];
test_bool flow_ctrl = TEST_FALSE; 
if (strcmp(str, "on") == 0) {
    flow_ctrl = TEST_TRUE;
} else if (strcmp(str, "off") == 0) {
    flow_ctrl = TEST_FALSE;
} else {
    printf ("argv[1] param error, must be on or off, but:%s\n", argv[1]);
    return TEST_FAILED;
}

char send_buf[SEND_LEN] = {0};
memset(send_buf, 0x81, sizeof(send_buf));

mraa_result_t mraa_ret = MRAA_SUCCESS;
mraa_uart_context dev = mraa_uart_init(UART_PORT);
if (dev == NULL) {
    printf ("mraa_uart_init(%d) failed!\n", UART_PORT);
    return TEST_FAILED;
}

mraa_ret = mraa_uart_set_flowcontrol(dev, TEST_FALSE, flow_ctrl);
if (mraa_ret != MRAA_SUCCESS) {
    printf ("mraa_uart_set_flowcontrol failed!\n");
    goto uart_stop;
}

mraa_ret = mraa_uart_set_baudrate(dev, UART_BAUD);
if (mraa_ret != MRAA_SUCCESS) {
    printf ("mraa_uart_set_baudrate for %d failed!\n", UART_BAUD);
    goto uart_stop;
}

/* 8 byte, no parity, 1 stop */
mraa_ret = mraa_uart_set_mode(dev, 8, MRAA_UART_PARITY_NONE, 1);
if (mraa_ret != MRAA_SUCCESS) {
    printf ("mraa_uart_set_mode failed!\n");
    goto uart_stop;
}

int i;
int bytesend = 0;
int total_byte = 0;
for (i = 0; i < SEND_COUNT; i++) {
    bytesend = mraa_uart_write(dev, send_buf, sizeof(send_buf));
    if (bytesend < 0) {
        printf ("turn:%d write failed, why:%s\n", i, strerror(errno));
        goto uart_stop;
    }
    total_byte += bytesend;
    printf ("[DEBUG]turn:%d, write %d success, total %d send\n", i, bytesend, total_byte);
    sleep(1);
}

mraa_uart_stop(dev);
return TEST_SUCCESS;

uart_stop: mraa_uart_stop(dev); return TEST_FAILED; }

jan-kiszka commented 1 year ago

"the behavior of RTS/CTS was the same as previous version" - the is a "not" missing, right?

Is this a regression of d8af4f999cd6999a68872d15060bd69d213c4130, could you check that?

andyliao5555 commented 1 year ago

sorry, I true miss the 'not'. say it clearly, g7ffd195 was the version I compiled myself, which included the pinmux complementation based on pinmux-select. g845a732 was the version early back to Feb. 2023, the pinmux implementation was not the same as g7ffd195 . I compared the behaviors of this 2 versions, there are some errors and differences.

andyliao5555 commented 1 year ago

"the behavior of RTS/CTS was the same as previous version" - the is a "not" missing, right?

Is this a regression of d8af4f9, could you check that?

please check the IO2 config in mraa_siemens_iot2050() of iot2050.c,the uart ctsn function name and group name was wrong, should be d2-uart0-ctsn instead of d1-uart0-ctsn, which caused the pinctrl subsystem error and abnormality in cts rts behavior.

jan-kiszka commented 1 year ago

Thanks for debugging! @benbrenson

benbrenson commented 1 year ago

Yepp. Thanks for debugging!

Bugfixes coming ASAP.

benbrenson commented 11 months ago

Seems I fixed that issue. PR is going to arrive the next hour. I changed your test application a bit in order to reach the "SEND_COUNT". If you empty the RX fifo, sending continues.... You can also avoid the sleep statement which will speed up test processing in some circumstances.

int i;
int bytesend = 0;
int byteread = 0;
int total_byte = 0;
for (i = 0; i < SEND_COUNT; i++) {
    bytesend = mraa_uart_write(dev, send_buf, sizeof(send_buf));
    if (bytesend < 0) {
        printf ("turn:%d write failed, why:%s\n", i, strerror(errno));
        goto uart_stop;
    }

    byteread = bytesend;

    while(byteread) {
        i = mraa_uart_read(dev, read_buf, sizeof(read_buf));
        if (i < 0) {
            printf ("turn:%d read failed, why:%s\n", i, strerror(errno));
            goto uart_stop;
        }
        byteread -= i;
    }

    total_byte += bytesend;
    printf ("[DEBUG]turn:%d, write %d success, total %d send\n", i, bytesend, total_byte);
}
root@iot2050-bennie:~# ./uart_cts_rts on
[DEBUG]turn:0, write 1024 success, total 1024 send
[DEBUG]turn:1, write 1024 success, total 2048 send
[DEBUG]turn:2, write 1024 success, total 3072 send
[DEBUG]turn:3, write 1024 success, total 4096 send
[DEBUG]turn:4, write 1024 success, total 5120 send
[DEBUG]turn:5, write 1024 success, total 6144 send
[DEBUG]turn:6, write 1024 success, total 7168 send
[DEBUG]turn:7, write 1024 success, total 8192 send
[DEBUG]turn:8, write 1024 success, total 9216 send
[DEBUG]turn:9, write 1024 success, total 10240 send
[DEBUG]turn:10, write 1024 success, total 11264 send
[DEBUG]turn:11, write 1024 success, total 12288 send
[DEBUG]turn:12, write 1024 success, total 13312 send
[DEBUG]turn:13, write 1024 success, total 14336 send
[DEBUG]turn:14, write 1024 success, total 15360 send
[DEBUG]turn:15, write 1024 success, total 16384 send
[DEBUG]turn:16, write 1024 success, total 17408 send
[DEBUG]turn:17, write 1024 success, total 18432 send
[DEBUG]turn:18, write 1024 success, total 19456 send
[DEBUG]turn:19, write 1024 success, total 20480 send
andyliao5555 commented 11 months ago

Thanks! I'm planning the regression soon.

Also,please pay attention to a hidden bug in iot2050_mux_debugfs() of iot2050.c line 90: ret = fprintf(fd, "%s %s\n", mux, mux); The pinmux-select debugfs file mechanism was based on a kernel patch: https://github.com/torvalds/linux/commit/6199f6becc869d30ca9394ca0f7a484bf9d598eb, in which the author explained the usage in the commit message:

To activate function pinmux-i2c1 on group pinmux-i2c1-pins: echo "pinmux-i2c1-pins pinmux-i2c1" > pinmux-select

First section of the string pinmux-i2c1-pins was the group name, second section of the string pinmux-i2c1 was the function name, back to iot2050_mux_debugfs(), the mux was only the group name: strncpy(mux, group, MRAA_PIN_NAME_SIZE);

Back to structure regmux_info_t, which designed to put the group name into member pmx_group[x] and put the function name into pmx_function[x], and we sure did this in the configure code iot2050_setup_pins().

The group name and function name may be the same on the platform IOT2050, but can not assume it in the code, should be separated as it was configured.

benbrenson commented 11 months ago

Yes, you're right. Technically this change makes no difference since both group and function are the same on iot2050. On other platforms instead this will result in a bug. I'll fix that for the sake of correctness. Maybe you can postpone the regression a bit so we can close that completely.

benbrenson commented 11 months ago

After internal discussion I would say that we leave that as it is. The pinmux-select depends on device tree definitions and the responsible driver on all AM6xxx platforms is pinctrl-single.c.

This driver doesn't distinguish between group and function and always set group = function during DT parsing. In MRAA we imitate this behavior and it reflects the pinctrl-single.c implementation (more or less).

As long as we support AM6xxx platforms this won't be an issue. Maybe a comment section in the code to clarify that would be enough (at least when it comes to upstreaming to MRAA)

benbrenson commented 10 months ago

@andyliao5555 any news regarding regression?