[TSP] Move Bind() to Start (#1538)

Fixes UB with static init. Turns out starting threads in static init doesn't work on windows.
PhotonVision · Nov 9, 2024 · 14f7155 · 14f7155
1 parent d188c37
commit 14f7155
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 23 deletions.
diff --git 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
@@ -156,17 +156,11 @@ wpi::tsp::TimeSyncClient::TimeSyncClient(std::string_view server,
                                          std::function<uint64_t()> 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
@@ -101,13 +101,13 @@ wpi::tsp::TimeSyncServer::TimeSyncServer(int port,
                                          std::function<uint64_t()> 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
@@ -53,6 +53,7 @@ class TimeSyncServer {
   wpi::Logger m_logger;
   std::function<uint64_t()> m_timeProvider;
   SharedUdpPtr m_udp;
+  int m_port;
 
   std::thread m_listener;