From 14f7155a2373aa15660f7db9dc0dc1df6160f77f Mon Sep 17 00:00:00 2001 From: Matt Date: Sat, 9 Nov 2024 14:35:38 -0800 Subject: [PATCH] [TSP] Move Bind() to Start (#1538) Fixes UB with static init. Turns out starting threads in static init doesn't work on windows. --- .../main/native/cpp/photon/PhotonCamera.cpp | 30 ++++++++++++------- .../main/native/cpp/net/TimeSyncClient.cpp | 17 ++++++----- .../main/native/cpp/net/TimeSyncServer.cpp | 8 ++--- .../main/native/include/net/TimeSyncServer.h | 1 + 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/photon-lib/src/main/native/cpp/photon/PhotonCamera.cpp b/photon-lib/src/main/native/cpp/photon/PhotonCamera.cpp index e4c791664..ce0025394 100644 --- a/photon-lib/src/main/native/cpp/photon/PhotonCamera.cpp +++ b/photon-lib/src/main/native/cpp/photon/PhotonCamera.cpp @@ -60,10 +60,21 @@ inline constexpr std::string_view bfw = ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>\n" "\n\n"; -// bit of a hack -- start a TimeSync server on port 5810 (hard-coded) -static std::mutex g_timeSyncServerMutex; -static bool g_timeSyncServerStarted; -static wpi::tsp::TimeSyncServer timesyncServer{5810}; +// bit of a hack -- start a TimeSync server on port 5810 (hard-coded). We want +// to avoid calling this from static initialization +static void InitTspServer() { + // We dont impose requirements about not calling the PhotonCamera constructor + // from different threads, so i guess we need this? + static std::mutex g_timeSyncServerMutex; + static bool g_timeSyncServerStarted{false}; + static wpi::tsp::TimeSyncServer timesyncServer{5810}; + + std::lock_guard lock{g_timeSyncServerMutex}; + if (!g_timeSyncServerStarted) { + timesyncServer.Start(); + g_timeSyncServerStarted = true; + } +} namespace photon { @@ -117,13 +128,10 @@ PhotonCamera::PhotonCamera(nt::NetworkTableInstance instance, HAL_Report(HALUsageReporting::kResourceType_PhotonCamera, InstanceCount); InstanceCount++; - { - std::lock_guard lock{g_timeSyncServerMutex}; - if (!g_timeSyncServerStarted) { - timesyncServer.Start(); - g_timeSyncServerStarted = true; - } - } + // The Robot class is actually created here: + // https://github.com/wpilibsuite/allwpilib/blob/811b1309683e930a1ce69fae818f943ff161b7a5/wpilibc/src/main/native/include/frc/RobotBase.h#L33 + // so we should be fine to call this from the ctor + InitTspServer(); } PhotonCamera::PhotonCamera(const std::string_view cameraName) diff --git a/photon-targeting/src/main/native/cpp/net/TimeSyncClient.cpp b/photon-targeting/src/main/native/cpp/net/TimeSyncClient.cpp index 238366ddf..26b64ca18 100644 --- a/photon-targeting/src/main/native/cpp/net/TimeSyncClient.cpp +++ b/photon-targeting/src/main/native/cpp/net/TimeSyncClient.cpp @@ -156,17 +156,11 @@ wpi::tsp::TimeSyncClient::TimeSyncClient(std::string_view server, std::function timeProvider) : m_logger(::ClientLoggerFunc), m_timeProvider(timeProvider), - m_udp{wpi::uv::Udp::Create(m_loopRunner.GetLoop(), AF_INET)}, - m_pingTimer{wpi::uv::Timer::Create(m_loopRunner.GetLoop())}, + m_udp{}, + m_pingTimer{}, m_serverIP{server}, m_serverPort{remote_port}, m_loopDelay(ping_delay) { - struct sockaddr_in serverAddr; - uv::NameToAddr(m_serverIP, m_serverPort, &serverAddr); - - m_loopRunner.ExecSync( - [this, serverAddr](uv::Loop&) { m_udp->Connect(serverAddr); }); - // fmt::println("Starting client (with server address {}:{})", server, // remote_port); } @@ -175,6 +169,13 @@ void wpi::tsp::TimeSyncClient::Start() { // fmt::println("Connecting received"); m_loopRunner.ExecSync([this](uv::Loop&) { + struct sockaddr_in serverAddr; + uv::NameToAddr(m_serverIP, m_serverPort, &serverAddr); + + m_udp = {wpi::uv::Udp::Create(m_loopRunner.GetLoop(), AF_INET)}; + m_pingTimer = {wpi::uv::Timer::Create(m_loopRunner.GetLoop())}; + + m_udp->Connect(serverAddr); m_udp->received.connect(&wpi::tsp::TimeSyncClient::UdpCallback, this); m_udp->StartRecv(); }); diff --git a/photon-targeting/src/main/native/cpp/net/TimeSyncServer.cpp b/photon-targeting/src/main/native/cpp/net/TimeSyncServer.cpp index baf296a6b..2bf7f8f19 100644 --- a/photon-targeting/src/main/native/cpp/net/TimeSyncServer.cpp +++ b/photon-targeting/src/main/native/cpp/net/TimeSyncServer.cpp @@ -101,13 +101,13 @@ wpi::tsp::TimeSyncServer::TimeSyncServer(int port, std::function timeProvider) : m_logger{::ServerLoggerFunc}, m_timeProvider{timeProvider}, - m_udp{wpi::uv::Udp::Create(m_loopRunner.GetLoop(), AF_INET)} { - m_loopRunner.ExecSync( - [this, port](uv::Loop&) { m_udp->Bind("0.0.0.0", port); }); -} + m_udp{}, + m_port(port) {} void wpi::tsp::TimeSyncServer::Start() { m_loopRunner.ExecSync([this](uv::Loop&) { + m_udp = {wpi::uv::Udp::Create(m_loopRunner.GetLoop(), AF_INET)}; + m_udp->Bind("0.0.0.0", m_port); m_udp->received.connect(&wpi::tsp::TimeSyncServer::UdpCallback, this); m_udp->StartRecv(); }); diff --git a/photon-targeting/src/main/native/include/net/TimeSyncServer.h b/photon-targeting/src/main/native/include/net/TimeSyncServer.h index faf1b9a4c..4a250da63 100644 --- a/photon-targeting/src/main/native/include/net/TimeSyncServer.h +++ b/photon-targeting/src/main/native/include/net/TimeSyncServer.h @@ -53,6 +53,7 @@ class TimeSyncServer { wpi::Logger m_logger; std::function m_timeProvider; SharedUdpPtr m_udp; + int m_port; std::thread m_listener;