From 9a545b434ad801b63032e7714c597c585d59cd22 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 14:25:36 +0100 Subject: [PATCH 1/9] configure.ac, tests/unit-test-(server|client).c: on WIN32 use setvbuf() to auto-flush so logs are comprehensible Signed-off-by: Jim Klimov --- configure.ac | 2 +- tests/unit-test-client.c | 10 ++++++++++ tests/unit-test-server.c | 10 ++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 2ff58266..72d736b3 100644 --- a/configure.ac +++ b/configure.ac @@ -125,7 +125,7 @@ AC_CHECK_DECLS([__CYGWIN__]) AC_SEARCH_LIBS(accept, network socket) # Checks for library functions. -AC_CHECK_FUNCS([accept4 gai_strerror getaddrinfo gettimeofday select socket strerror strlcpy]) +AC_CHECK_FUNCS([accept4 gai_strerror getaddrinfo gettimeofday select socket strerror strlcpy setvbuf]) # Required for MinGW with GCC v4.8.1 on Win7 AC_DEFINE(WINVER, 0x0501, _) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index b3ac76fa..57d715b2 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -98,6 +98,16 @@ int main(int argc, char *argv[]) memset(last_test_title, 0, sizeof(last_test_title)); +#ifdef _WIN32 +# ifdef HAVE_SETVBUF + // auto-flush so logs are comprehensible when the + // same console collects output of server and client + // https://stackoverflow.com/a/214292/4715872 + setvbuf(stdout, NULL, _IOLBF, 0); + setvbuf(stderr, NULL, _IOLBF, 0); +# endif +#endif + if (argc > 1) { if (strcmp(argv[1], "tcp") == 0) { use_backend = TCP; diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c index ea4455f1..e26b6f0a 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -44,6 +44,16 @@ int main(int argc, char *argv[]) int header_length; char *ip_or_device = NULL; +#ifdef _WIN32 +# ifdef HAVE_SETVBUF + // auto-flush so logs are comprehensible when the + // same console collects output of server and client + // https://stackoverflow.com/a/214292/4715872 + setvbuf(stdout, NULL, _IOLBF, 0); + setvbuf(stderr, NULL, _IOLBF, 0); +# endif +#endif + if (argc > 1) { if (strcmp(argv[1], "tcp") == 0) { use_backend = TCP; From 909755ec0827fc030b5aaccf3bfef9e072f57f58 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 17:46:39 +0100 Subject: [PATCH 2/9] tests/unit-test-server.c: fflush() during the loop to minimize broken console logs on WIN32 Signed-off-by: Jim Klimov --- tests/unit-test-server.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c index e26b6f0a..965ba5a2 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -179,6 +179,9 @@ int main(int argc, char *argv[]) for (;;) { do { +#ifdef _WIN32 + fflush(stdout); +#endif rc = modbus_receive(ctx, query); /* Filtered queries return 0 */ } while (rc == 0); @@ -260,6 +263,9 @@ int main(int argc, char *argv[]) } } +#ifdef _WIN32 + fflush(stdout); +#endif rc = modbus_reply(ctx, query, rc, mb_mapping); if (rc == -1) { break; From 0254f288efda6b25fc53bd08409fa09b02da7303 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:13:32 +0100 Subject: [PATCH 3/9] tests/unit-test-client.c: sleep more and flush again before the "7ms > 5ms" test case It fails or passes randomly on different platforms Win32/mingw, FreeBSD, MacOS X maybe something remains in buffer? We'll see if extra sleep helps. Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 57d715b2..3dfafb4b 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -731,6 +731,9 @@ int main(int argc, char *argv[]) /* Wait remaining bytes before flushing */ usleep(11 * 5000); modbus_flush(ctx); + /* ...and then some, just in case (e.g. kernel delays, clock jitter, multi-CPU...) */ + usleep(1000000); + modbus_flush(ctx); /* Timeout of 7ms between bytes */ TEST_TITLE("2/2 Adapted byte timeout (7ms > 5ms)"); From b0b538c038e42dcc5876e0c841d397b6b2254af1 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:36:24 +0100 Subject: [PATCH 4/9] tests/unit-test-client.c: flush before "4/5 modbus_write_and_read_registers" so the buffer does not contaminate the result on some systems Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 3dfafb4b..b831aa8f 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -298,6 +298,10 @@ int main(int argc, char *argv[]) : UT_INPUT_REGISTERS_NB; memset(tab_rp_registers, 0, nb_points * sizeof(uint16_t)); + /* Wait remaining bytes before flushing */ + usleep(1000000); + modbus_flush(ctx); + TEST_TITLE("4/5 modbus_write_and_read_registers"); /* Write registers to zero from tab_rp_registers and store read registers into tab_rp_registers. So the read registers must set to 0, except the From dac1cf8abf6796d946ac1561c8edf429ce66eea6 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:38:38 +0100 Subject: [PATCH 5/9] tests/unit-test-client.c: on WIN32 replace "7ms > 5ms" test case with "33ms > 20ms" Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 10 ++++++++++ tests/unit-test-server.c | 8 ++++++-- tests/unit-test.h.in | 5 ++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index b831aa8f..c68b08c8 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -739,11 +739,21 @@ int main(int argc, char *argv[]) usleep(1000000); modbus_flush(ctx); +#ifdef _WIN32 + /* Timeout of 20ms between bytes, allow for 2*16+1 + * Windows sleep seems to be at least 15ms always + */ + TEST_TITLE("2/2 Adapted byte timeout (33ms > 20ms)"); + modbus_set_byte_timeout(ctx, 0, 33000); + rc = modbus_read_registers( + ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); +#else /* Timeout of 7ms between bytes */ TEST_TITLE("2/2 Adapted byte timeout (7ms > 5ms)"); modbus_set_byte_timeout(ctx, 0, 7000); rc = modbus_read_registers( ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS, 1, tab_rp_registers); +#endif ASSERT_TRUE(rc == 1, "FAILED (rc: %d != 1)", rc); } diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c index 965ba5a2..1aab0d43 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -221,7 +221,7 @@ int main(int argc, char *argv[]) } else if (address == UT_REGISTERS_ADDRESS_SLEEP_500_MS) { printf("Sleep 0.5 s before replying\n"); usleep(500000); - } else if (address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS) { + } else if (address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS || address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS) { /* Test low level only available in TCP mode */ /* Catch the reply and send reply byte a byte */ uint8_t req[] = "\x00\x1C\x00\x00\x00\x05\xFF\x03\x02\x00\x00"; @@ -236,7 +236,11 @@ int main(int argc, char *argv[]) req[1] = query[1]; for (i = 0; i < req_length; i++) { printf("(%.2X)", req[i]); - usleep(5000); + if (address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS) { + usleep(5000); + } else if (address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS) { + usleep(20000); + } rc = send(w_s, (const char *) (req + i), 1, MSG_NOSIGNAL); if (rc == -1) { break; diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in index 3d83967a..d6427c59 100644 --- a/tests/unit-test.h.in +++ b/tests/unit-test.h.in @@ -54,8 +54,11 @@ const uint16_t UT_REGISTERS_ADDRESS_SPECIAL = 0x170; const uint16_t UT_REGISTERS_ADDRESS_INVALID_TID_OR_SLAVE = 0x171; /* The server will wait for 1 second before replying to test timeout */ const uint16_t UT_REGISTERS_ADDRESS_SLEEP_500_MS = 0x172; -/* The server will wait for 5 ms before sending each byte */ +/* The server will wait for 5 ms before sending each byte + * WARNING: this may be too short for WIN32 */ const uint16_t UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS = 0x173; +/* The server will wait for 20 ms before sending each byte */ +const uint16_t UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS = 0x174; /* If the following value is used, a bad response is sent. It's better to test with a lower value than From 79742cf58bf666e6ad8cfb9b6d6e28526a3133c2 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 20:15:06 +0100 Subject: [PATCH 6/9] tests/unit-test-client.c: apply "33ms > 20ms" test also to FreeBSD and OpenBSD Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index c68b08c8..89f88461 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -739,10 +739,15 @@ int main(int argc, char *argv[]) usleep(1000000); modbus_flush(ctx); -#ifdef _WIN32 - /* Timeout of 20ms between bytes, allow for 2*16+1 - * Windows sleep seems to be at least 15ms always - */ +#if defined(_WIN32) || defined(__FreeBSD__) || defined(__OpenBSD__) + /* Timeout of 20ms between bytes, allow for 2*16+1 + * Windows sleep seems to be at least 15ms always + * Windows sleep seems to be at least 15ms always. + * For some reason, FreeBSD 12 and OpenBSD 6.5 also + * tended to fail with 7ms variant as "gmake check" + * but pass in + * gmake -j 8 && ( ./tests/unit-test-server|cat & sleep 1 ; ./tests/unit-test-client|cat ) + */ TEST_TITLE("2/2 Adapted byte timeout (33ms > 20ms)"); modbus_set_byte_timeout(ctx, 0, 33000); rc = modbus_read_registers( From c2bc486aa216121dcb95d15a883bd76197f05e4c Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:34:27 +0000 Subject: [PATCH 7/9] tests/unit-test-client.c: separate *BSD tests with "66ms > 20ms" into their own category Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 89f88461..34b0ca79 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -739,19 +739,25 @@ int main(int argc, char *argv[]) usleep(1000000); modbus_flush(ctx); -#if defined(_WIN32) || defined(__FreeBSD__) || defined(__OpenBSD__) - /* Timeout of 20ms between bytes, allow for 2*16+1 - * Windows sleep seems to be at least 15ms always - * Windows sleep seems to be at least 15ms always. - * For some reason, FreeBSD 12 and OpenBSD 6.5 also - * tended to fail with 7ms variant as "gmake check" - * but pass in - * gmake -j 8 && ( ./tests/unit-test-server|cat & sleep 1 ; ./tests/unit-test-client|cat ) - */ +#if defined(_WIN32) + /* Timeout of 20ms between bytes, allow for 2*16+1 + * Windows sleep seems to be at least 15ms always. + */ TEST_TITLE("2/2 Adapted byte timeout (33ms > 20ms)"); modbus_set_byte_timeout(ctx, 0, 33000); rc = modbus_read_registers( ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); +#elif defined(__FreeBSD__) || defined(__OpenBSD__) + /* For some reason, FreeBSD 12 and OpenBSD 6.5 also + * tended to fail with 7ms and even 33ms variants + * as "gmake check", but passed in + * gmake -j 8 && ( ./tests/unit-test-server|cat & sleep 1 ; ./tests/unit-test-client|cat ) + * An even longer timeout seems to satisfy all of them. + */ + TEST_TITLE("2/2 Adapted byte timeout (66ms > 20ms)"); + modbus_set_byte_timeout(ctx, 0, 66000); + rc = modbus_read_registers( + ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); #else /* Timeout of 7ms between bytes */ TEST_TITLE("2/2 Adapted byte timeout (7ms > 5ms)"); From 5350474da9fff18e1581429b72b3db6a2fc43d63 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:44:47 +0000 Subject: [PATCH 8/9] tests/unit-test-client.c: revise test case "Adapted byte timeout (Xms > Yms)" to increase the timeouts and fit all laggy systems better Signed-off-by: Jim Klimov --- tests/unit-test-client.c | 55 +++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 34b0ca79..527940c9 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -739,33 +739,42 @@ int main(int argc, char *argv[]) usleep(1000000); modbus_flush(ctx); -#if defined(_WIN32) - /* Timeout of 20ms between bytes, allow for 2*16+1 - * Windows sleep seems to be at least 15ms always. - */ - TEST_TITLE("2/2 Adapted byte timeout (33ms > 20ms)"); - modbus_set_byte_timeout(ctx, 0, 33000); - rc = modbus_read_registers( - ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); -#elif defined(__FreeBSD__) || defined(__OpenBSD__) - /* For some reason, FreeBSD 12 and OpenBSD 6.5 also - * tended to fail with 7ms and even 33ms variants - * as "gmake check", but passed in - * gmake -j 8 && ( ./tests/unit-test-server|cat & sleep 1 ; ./tests/unit-test-client|cat ) - * An even longer timeout seems to satisfy all of them. - */ - TEST_TITLE("2/2 Adapted byte timeout (66ms > 20ms)"); - modbus_set_byte_timeout(ctx, 0, 66000); - rc = modbus_read_registers( - ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); -#else /* Timeout of 7ms between bytes */ - TEST_TITLE("2/2 Adapted byte timeout (7ms > 5ms)"); + TEST_TITLE("2/2-A Adapted byte timeout (7ms > 5ms)"); modbus_set_byte_timeout(ctx, 0, 7000); rc = modbus_read_registers( ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS, 1, tab_rp_registers); -#endif - ASSERT_TRUE(rc == 1, "FAILED (rc: %d != 1)", rc); + if (rc == 1) { + ASSERT_TRUE(rc == 1, "FAILED (rc: %d != 1)", rc); + } else { + /* Timeout of 20ms between bytes, allow for 2*16+1 + * Windows sleep seems to be at least 15ms always. + */ + usleep(1000000); + modbus_flush(ctx); + TEST_TITLE("2/2-B Adapted byte timeout (33ms > 20ms)"); + modbus_set_byte_timeout(ctx, 0, 33000); + rc = modbus_read_registers( + ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); + + if (rc == 1) { + ASSERT_TRUE(rc == 1, "FAILED (rc: %d != 1)", rc); + } else { + /* For some reason, FreeBSD 12 and OpenBSD 6.5 also + * tended to fail with 7ms and even 33ms variants + * as "gmake check", but passed in + * gmake -j 8 && ( ./tests/unit-test-server|cat & sleep 1 ; ./tests/unit-test-client|cat ) + * An even longer timeout seems to satisfy all of them. + */ + usleep(1000000); + modbus_flush(ctx); + TEST_TITLE("2/2-C Adapted byte timeout (66ms > 20ms)"); + modbus_set_byte_timeout(ctx, 0, 66000); + rc = modbus_read_registers( + ctx, UT_REGISTERS_ADDRESS_BYTE_SLEEP_20_MS, 1, tab_rp_registers); + ASSERT_TRUE(rc == 1, "FAILED (rc: %d != 1)", rc); + } + } } /* Restore original byte timeout */ From db24fedcbeacc0c7620b9020af25c3acd0f43efa Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 18 Jan 2025 19:07:45 +0100 Subject: [PATCH 9/9] src/modbus.c: _modbus_receive_msg(): if ctx->backend->select() failed on WIN32, and we follow MODBUS_ERROR_RECOVERY_LINK, do modbus_flush(ctx) At least this allows unit-tests to pass... Signed-off-by: Jim Klimov --- src/modbus.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/modbus.c b/src/modbus.c index af3e9781..2517efd7 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -427,6 +427,8 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) wsa_err = WSAGetLastError(); // no equivalent to ETIMEDOUT when select fails on Windows + // but we still need it to forget the invalid answer + modbus_flush(ctx); if (wsa_err == WSAENETDOWN || wsa_err == WSAENOTSOCK) { modbus_close(ctx); modbus_connect(ctx);