From 3df3e4ae914569db8442dde5feb037b7596130c9 Mon Sep 17 00:00:00 2001 From: Felix Exner Date: Mon, 9 Feb 2026 11:36:46 +0100 Subject: [PATCH 1/4] Fix ReverseInterfaceTest.handle_program_state Attempting to fix thread synchronization. I'm not entirely sure that that's the problem as I cannot reprocude things locally and the output suggests that there's a client connecting twice, but it's something that could be fixed. --- tests/test_reverse_interface.cpp | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/test_reverse_interface.cpp b/tests/test_reverse_interface.cpp index a365b6dd8..dee96cd2b 100644 --- a/tests/test_reverse_interface.cpp +++ b/tests/test_reverse_interface.cpp @@ -202,22 +202,33 @@ class ReverseIntefaceTest : public ::testing::Test void handleProgramState(bool program_state) { std::lock_guard lk(program_running_mutex_); - program_running_.notify_one(); + new_program_state_received_ = true; program_state_ = program_state; + program_running_.notify_one(); } bool waitForProgramState(int milliseconds = 100, bool program_state = true) { + // Wait for new state until timeout has elapsed std::unique_lock lk(program_running_mutex_); - if (program_running_.wait_for(lk, std::chrono::milliseconds(milliseconds)) == std::cv_status::no_timeout || - program_state_ == program_state) + std::chrono::steady_clock::time_point start_time = std::chrono::steady_clock::now(); + while ( + program_state_ != program_state && + std::chrono::duration_cast(std::chrono::steady_clock::now() - start_time).count() < + milliseconds) { - if (program_state_ == program_state) + if (program_running_.wait_for(lk, std::chrono::milliseconds(milliseconds / 10), + [this] { return new_program_state_received_.load(); })) { - return true; + new_program_state_received_ = false; + // Check whether the new state matches the expected state + if (program_state_ == program_state) + { + return true; + } } } - return false; + return program_state_ == program_state; } std::unique_ptr reverse_interface_; @@ -225,6 +236,7 @@ class ReverseIntefaceTest : public ::testing::Test private: std::atomic program_state_ = ATOMIC_VAR_INIT(false); + std::atomic new_program_state_received_ = ATOMIC_VAR_INIT(false); std::condition_variable program_running_; std::mutex program_running_mutex_; }; From b6b31f4c9a05db09bed66a0c1b25610c08b23d91 Mon Sep 17 00:00:00 2001 From: Felix Exner Date: Tue, 24 Feb 2026 16:04:03 +0100 Subject: [PATCH 2/4] More debugging --- src/comm/tcp_server.cpp | 14 ++++++++++++++ tests/test_reverse_interface.cpp | 3 ++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/comm/tcp_server.cpp b/src/comm/tcp_server.cpp index 8b5797746..42fb12f95 100644 --- a/src/comm/tcp_server.cpp +++ b/src/comm/tcp_server.cpp @@ -185,6 +185,20 @@ void TCPServer::handleConnect() throw std::system_error(std::error_code(errno, std::generic_category()), ss.str()); } + char addr_str[INET6_ADDRSTRLEN]; + void* addr; + // Determine address type and set addr pointer accordingly + if (client_addr.ss_family == AF_INET) + { + addr = &((struct sockaddr_in*)&client_addr)->sin_addr; + } + else + { + addr = &((struct sockaddr_in6*)&client_addr)->sin6_addr; + } + inet_ntop(client_addr.ss_family, addr, addr_str, sizeof(addr_str)); + URCL_LOG_DEBUG("Accepted new connection on port %d with from client %s", port_, addr_str); + if (client_fds_.size() < max_clients_allowed_ || max_clients_allowed_ == 0) { client_fds_.push_back(client_fd); diff --git a/tests/test_reverse_interface.cpp b/tests/test_reverse_interface.cpp index dee96cd2b..8f2a9630f 100644 --- a/tests/test_reverse_interface.cpp +++ b/tests/test_reverse_interface.cpp @@ -43,6 +43,7 @@ class TestableReverseInterface : public control::ReverseInterface public: TestableReverseInterface(const control::ReverseInterfaceConfig& config) : control::ReverseInterface(config) { + URCL_LOG_DEBUG("Created TestableReverseInterface"); } virtual void connectionCallback(const socket_t filedescriptor) @@ -557,7 +558,7 @@ TEST_F(ReverseIntefaceTest, disconnected_callbacks_are_called) int main(int argc, char* argv[]) { ::testing::InitGoogleTest(&argc, argv); - urcl::setLogLevel(LogLevel::INFO); + urcl::setLogLevel(LogLevel::DEBUG); return RUN_ALL_TESTS(); } From bae133422c08a7ab187fb87ecc64b1e86fd4377e Mon Sep 17 00:00:00 2001 From: Felix Exner Date: Thu, 26 Feb 2026 11:02:37 +0100 Subject: [PATCH 3/4] Run ReverseInterfaceTest on a custom port In order to avoid any undesired interference, we move the test away from the default port. --- include/ur_client_library/comm/tcp_server.h | 17 ++++++++- .../control/reverse_interface.h | 13 +++++++ src/comm/tcp_server.cpp | 11 ++++++ tests/test_reverse_interface.cpp | 38 ++++++++++--------- 4 files changed, 60 insertions(+), 19 deletions(-) diff --git a/include/ur_client_library/comm/tcp_server.h b/include/ur_client_library/comm/tcp_server.h index 7a74bd0fd..285eb662f 100644 --- a/include/ur_client_library/comm/tcp_server.h +++ b/include/ur_client_library/comm/tcp_server.h @@ -154,6 +154,21 @@ class TCPServer max_clients_allowed_ = max_clients_allowed; } + /*! + * \brief Get the port this server is bound to + * + * If port number 0 is passed during initialization, the server will bind to a random free port. + * In this case, this function can be used to get the actual port number the server is bound to. + * This should only be called after the server has been initialized, otherwise the returned port + * number might not be correct. + * + * \returns The port number this server is bound to + */ + int getPort() const + { + return port_; + } + private: void init(); void bind(const size_t max_num_tries, const std::chrono::milliseconds reconnection_time); @@ -197,4 +212,4 @@ class TCPServer } // namespace comm } // namespace urcl -#endif // ifndef UR_CLIENT_LIBRARY_TCP_SERVER_H_INCLUDED \ No newline at end of file +#endif // ifndef UR_CLIENT_LIBRARY_TCP_SERVER_H_INCLUDED diff --git a/include/ur_client_library/control/reverse_interface.h b/include/ur_client_library/control/reverse_interface.h index 822020791..37e1dee9a 100644 --- a/include/ur_client_library/control/reverse_interface.h +++ b/include/ur_client_library/control/reverse_interface.h @@ -194,6 +194,19 @@ class ReverseInterface return client_fd_ != INVALID_SOCKET; } + /*! + * \brief Get the port number the server is bound to. + * + * If port number 0 is passed during initialization, the server will bind to a random free port. + * In this case, this function can be used to get the actual port number the server is bound to. + * + * \returns The port number the server is bound to. + */ + int getPort() const + { + return server_.getPort(); + } + protected: virtual void connectionCallback(const socket_t filedescriptor); diff --git a/src/comm/tcp_server.cpp b/src/comm/tcp_server.cpp index 42fb12f95..daaf51e3c 100644 --- a/src/comm/tcp_server.cpp +++ b/src/comm/tcp_server.cpp @@ -170,6 +170,17 @@ void TCPServer::startListen() ss << "Failed to start listen on port " << port_; throw std::system_error(std::error_code(errno, std::generic_category()), ss.str()); } + struct sockaddr_in sin; + socklen_t len = sizeof(sin); + if (getsockname(listen_fd_, (struct sockaddr*)&sin, &len) == -1) + { + URCL_LOG_ERROR("getsockname() failed to get port number for listening socket: %s", strerror(errno)); + } + + else + { + port_ = ntohs(sin.sin_port); + } URCL_LOG_DEBUG("Listening on port %d", port_); } diff --git a/tests/test_reverse_interface.cpp b/tests/test_reverse_interface.cpp index 8f2a9630f..5b19f1ad7 100644 --- a/tests/test_reverse_interface.cpp +++ b/tests/test_reverse_interface.cpp @@ -66,7 +66,7 @@ class TestableReverseInterface : public control::ReverseInterface std::atomic connected = false; }; -class ReverseIntefaceTest : public ::testing::Test +class ReverseInterfaceTest : public ::testing::Test { protected: class Client : public comm::TCPSocket @@ -182,10 +182,11 @@ class ReverseIntefaceTest : public ::testing::Test void SetUp() { control::ReverseInterfaceConfig config; - config.port = 50001; - config.handle_program_state = std::bind(&ReverseIntefaceTest::handleProgramState, this, std::placeholders::_1); + config.port = 0; + config.handle_program_state = std::bind(&ReverseInterfaceTest::handleProgramState, this, std::placeholders::_1); reverse_interface_.reset(new TestableReverseInterface(config)); - client_.reset(new Client(50001)); + test_port_ = reverse_interface_->getPort(); + client_.reset(new Client(test_port_)); std::unique_lock lk(g_connection_mutex); g_connection_condition.wait_for(lk, std::chrono::seconds(1), [&]() { return reverse_interface_->connected.load(); }); @@ -234,6 +235,7 @@ class ReverseIntefaceTest : public ::testing::Test std::unique_ptr reverse_interface_; std::unique_ptr client_; + int test_port_; private: std::atomic program_state_ = ATOMIC_VAR_INIT(false); @@ -242,7 +244,7 @@ class ReverseIntefaceTest : public ::testing::Test std::mutex program_running_mutex_; }; -TEST_F(ReverseIntefaceTest, handle_program_state) +TEST_F(ReverseInterfaceTest, handle_program_state) { // Test that handle program state is called when the client connects to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -252,7 +254,7 @@ TEST_F(ReverseIntefaceTest, handle_program_state) EXPECT_TRUE(waitForProgramState(1000, false)); } -TEST_F(ReverseIntefaceTest, write_positions) +TEST_F(ReverseInterfaceTest, write_positions) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -269,7 +271,7 @@ TEST_F(ReverseIntefaceTest, write_positions) EXPECT_EQ(written_positions[5], ((double)received_positions[5]) / reverse_interface_->MULT_JOINTSTATE); } -TEST_F(ReverseIntefaceTest, write_trajectory_control_message) +TEST_F(ReverseInterfaceTest, write_trajectory_control_message) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -293,7 +295,7 @@ TEST_F(ReverseIntefaceTest, write_trajectory_control_message) EXPECT_EQ(toUnderlying(written_control_message), received_control_message); } -TEST_F(ReverseIntefaceTest, write_trajectory_point_number) +TEST_F(ReverseInterfaceTest, write_trajectory_point_number) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -306,7 +308,7 @@ TEST_F(ReverseIntefaceTest, write_trajectory_point_number) EXPECT_EQ(written_point_number, received_point_number); } -TEST_F(ReverseIntefaceTest, control_mode_is_forward) +TEST_F(ReverseInterfaceTest, control_mode_is_forward) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -319,7 +321,7 @@ TEST_F(ReverseIntefaceTest, control_mode_is_forward) EXPECT_EQ(toUnderlying(expected_control_mode), received_control_mode); } -TEST_F(ReverseIntefaceTest, remaining_message_points_are_zeros) +TEST_F(ReverseInterfaceTest, remaining_message_points_are_zeros) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -336,7 +338,7 @@ TEST_F(ReverseIntefaceTest, remaining_message_points_are_zeros) EXPECT_EQ(0, received_pos[5]); } -TEST_F(ReverseIntefaceTest, read_timeout) +TEST_F(ReverseInterfaceTest, read_timeout) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -365,7 +367,7 @@ TEST_F(ReverseIntefaceTest, read_timeout) EXPECT_EQ(expected_read_timeout, received_read_timeout); } -TEST_F(ReverseIntefaceTest, default_read_timeout) +TEST_F(ReverseInterfaceTest, default_read_timeout) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -392,7 +394,7 @@ TEST_F(ReverseIntefaceTest, default_read_timeout) EXPECT_EQ(expected_read_timeout, received_read_timeout); } -TEST_F(ReverseIntefaceTest, write_control_mode) +TEST_F(ReverseInterfaceTest, write_control_mode) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -451,7 +453,7 @@ TEST_F(ReverseIntefaceTest, write_control_mode) EXPECT_EQ(toUnderlying(expected_control_mode), received_control_mode); } -TEST_F(ReverseIntefaceTest, write_freedrive_control_message) +TEST_F(ReverseInterfaceTest, write_freedrive_control_message) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -475,7 +477,7 @@ TEST_F(ReverseIntefaceTest, write_freedrive_control_message) EXPECT_EQ(toUnderlying(written_freedrive_message), received_freedrive_message); } -TEST_F(ReverseIntefaceTest, deprecated_set_keep_alive_count) +TEST_F(ReverseInterfaceTest, deprecated_set_keep_alive_count) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -502,7 +504,7 @@ TEST_F(ReverseIntefaceTest, deprecated_set_keep_alive_count) EXPECT_EQ(expected_read_timeout, received_read_timeout); } -TEST_F(ReverseIntefaceTest, disconnected_callbacks_are_called) +TEST_F(ReverseInterfaceTest, disconnected_callbacks_are_called) { // Wait for the client to connect to the server EXPECT_TRUE(waitForProgramState(1000, true)); @@ -533,7 +535,7 @@ TEST_F(ReverseIntefaceTest, disconnected_callbacks_are_called) // Unregister 1. 2 should still be called disconnect_called_1 = false; disconnect_called_2 = false; - client_.reset(new Client(50001)); + client_.reset(new Client(test_port_)); EXPECT_TRUE(waitForProgramState(1000, true)); reverse_interface_->unregisterDisconnectionCallback(disconnection_callback_id_1); client_->close(); @@ -545,7 +547,7 @@ TEST_F(ReverseIntefaceTest, disconnected_callbacks_are_called) // Unregister both. None should be called disconnect_called_1 = false; disconnect_called_2 = false; - client_.reset(new Client(50001)); + client_.reset(new Client(test_port_)); EXPECT_TRUE(waitForProgramState(1000, true)); reverse_interface_->unregisterDisconnectionCallback(disconnection_callback_id_2); client_->close(); From 6aafbbd5efad0f10fd40706cc7ded525b7cd684b Mon Sep 17 00:00:00 2001 From: Felix Exner Date: Thu, 26 Feb 2026 11:14:18 +0100 Subject: [PATCH 4/4] Revert "More debugging" This reverts commit 6aa92b7b2954a3812adbe72d18c0ce7b8e30c597. --- src/comm/tcp_server.cpp | 14 -------------- tests/test_reverse_interface.cpp | 3 +-- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/comm/tcp_server.cpp b/src/comm/tcp_server.cpp index daaf51e3c..22505b6da 100644 --- a/src/comm/tcp_server.cpp +++ b/src/comm/tcp_server.cpp @@ -196,20 +196,6 @@ void TCPServer::handleConnect() throw std::system_error(std::error_code(errno, std::generic_category()), ss.str()); } - char addr_str[INET6_ADDRSTRLEN]; - void* addr; - // Determine address type and set addr pointer accordingly - if (client_addr.ss_family == AF_INET) - { - addr = &((struct sockaddr_in*)&client_addr)->sin_addr; - } - else - { - addr = &((struct sockaddr_in6*)&client_addr)->sin6_addr; - } - inet_ntop(client_addr.ss_family, addr, addr_str, sizeof(addr_str)); - URCL_LOG_DEBUG("Accepted new connection on port %d with from client %s", port_, addr_str); - if (client_fds_.size() < max_clients_allowed_ || max_clients_allowed_ == 0) { client_fds_.push_back(client_fd); diff --git a/tests/test_reverse_interface.cpp b/tests/test_reverse_interface.cpp index 5b19f1ad7..bf15fc787 100644 --- a/tests/test_reverse_interface.cpp +++ b/tests/test_reverse_interface.cpp @@ -43,7 +43,6 @@ class TestableReverseInterface : public control::ReverseInterface public: TestableReverseInterface(const control::ReverseInterfaceConfig& config) : control::ReverseInterface(config) { - URCL_LOG_DEBUG("Created TestableReverseInterface"); } virtual void connectionCallback(const socket_t filedescriptor) @@ -560,7 +559,7 @@ TEST_F(ReverseInterfaceTest, disconnected_callbacks_are_called) int main(int argc, char* argv[]) { ::testing::InitGoogleTest(&argc, argv); - urcl::setLogLevel(LogLevel::DEBUG); + urcl::setLogLevel(LogLevel::INFO); return RUN_ALL_TESTS(); }