[hal] Refactor C++ handle closing; check for invalid handle before cl…

…osing (#7016) Adds a close function pointer template parameter to hal::Handle. This allows default destructors in many places. The status parameter has been removed from close functions; in most places it was not used. Where it was, an error is printed instead.
wpilibsuite · Sep 7, 2024 · 496e7c1 · 496e7c1
1 parent 80f3813
commit 496e7c1
Show file tree

Hide file tree

Showing 94 changed files with 247 additions and 425 deletions.
diff --git a/crossConnIntegrationTests/src/main/native/include/LifetimeWrappers.h b/crossConnIntegrationTests/src/main/native/include/LifetimeWrappers.h
@@ -10,184 +10,69 @@
 
 namespace hlt {
 
-struct InterruptHandle {
+struct InterruptHandle : hal::Handle<HAL_InterruptHandle, HAL_CleanInterrupts> {
  public:
-  explicit InterruptHandle(int32_t* status) {
-    handle = HAL_InitializeInterrupts(status);
-  }
-  InterruptHandle(const InterruptHandle&) = delete;
-  InterruptHandle operator=(const InterruptHandle&) = delete;
-
-  InterruptHandle(InterruptHandle&&) = default;
-  InterruptHandle& operator=(InterruptHandle&&) = default;
-
-  ~InterruptHandle() { HAL_CleanInterrupts(handle); }
-
-  operator HAL_InterruptHandle() const { return handle; }
-
- private:
-  HAL_InterruptHandle handle = 0;
+  explicit InterruptHandle(int32_t* status)
+      : Handle{HAL_InitializeInterrupts(status)} {}
 };
 
-struct DMAHandle {
+struct DMAHandle : hal::Handle<HAL_DMAHandle, HAL_FreeDMA> {
  public:
-  explicit DMAHandle(int32_t* status) { handle = HAL_InitializeDMA(status); }
-  DMAHandle(const DMAHandle&) = delete;
-  DMAHandle operator=(const DMAHandle&) = delete;
-
-  DMAHandle(DMAHandle&&) = default;
-  DMAHandle& operator=(DMAHandle&&) = default;
-
-  ~DMAHandle() { HAL_FreeDMA(handle); }
-
-  operator HAL_DMAHandle() const { return handle; }
-
- private:
-  HAL_DMAHandle handle = 0;
+  explicit DMAHandle(int32_t* status) : Handle{HAL_InitializeDMA(status)} {}
 };
 
-struct AnalogInputHandle {
+struct AnalogInputHandle
+    : hal::Handle<HAL_AnalogInputHandle, HAL_FreeAnalogInputPort> {
  public:
-  AnalogInputHandle(int32_t port, int32_t* status) {
-    handle = HAL_InitializeAnalogInputPort(HAL_GetPort(port), nullptr, status);
-  }
-  AnalogInputHandle(const AnalogInputHandle&) = delete;
-  AnalogInputHandle operator=(const AnalogInputHandle&) = delete;
-
-  AnalogInputHandle(AnalogInputHandle&&) = default;
-  AnalogInputHandle& operator=(AnalogInputHandle&&) = default;
-
-  ~AnalogInputHandle() { HAL_FreeAnalogInputPort(handle); }
-
-  operator HAL_AnalogInputHandle() const { return handle; }
-
- private:
-  HAL_AnalogInputHandle handle = 0;
+  AnalogInputHandle(int32_t port, int32_t* status)
+      : Handle{HAL_InitializeAnalogInputPort(HAL_GetPort(port), nullptr,
+                                             status)} {}
 };
 
-struct AnalogOutputHandle {
+struct AnalogOutputHandle
+    : hal::Handle<HAL_AnalogOutputHandle, HAL_FreeAnalogOutputPort> {
  public:
-  AnalogOutputHandle(int32_t port, int32_t* status) {
-    handle = HAL_InitializeAnalogOutputPort(HAL_GetPort(port), nullptr, status);
-  }
-  AnalogOutputHandle(const AnalogOutputHandle&) = delete;
-  AnalogOutputHandle operator=(const AnalogOutputHandle&) = delete;
-
-  AnalogOutputHandle(AnalogOutputHandle&&) = default;
-  AnalogOutputHandle& operator=(AnalogOutputHandle&&) = default;
-
-  ~AnalogOutputHandle() { HAL_FreeAnalogOutputPort(handle); }
-
-  operator HAL_AnalogOutputHandle() const { return handle; }
-
- private:
-  HAL_AnalogOutputHandle handle = 0;
+  AnalogOutputHandle(int32_t port, int32_t* status)
+      : Handle{HAL_InitializeAnalogOutputPort(HAL_GetPort(port), nullptr,
+                                              status)} {}
 };
 
-struct AnalogTriggerHandle {
+struct AnalogTriggerHandle
+    : hal::Handle<HAL_AnalogTriggerHandle, HAL_CleanAnalogTrigger> {
  public:
-  explicit AnalogTriggerHandle(HAL_AnalogInputHandle port, int32_t* status) {
-    handle = HAL_InitializeAnalogTrigger(port, status);
-  }
-  AnalogTriggerHandle(const AnalogTriggerHandle&) = delete;
-  AnalogTriggerHandle operator=(const AnalogTriggerHandle&) = delete;
-
-  AnalogTriggerHandle(AnalogTriggerHandle&&) = default;
-  AnalogTriggerHandle& operator=(AnalogTriggerHandle&&) = default;
-
-  ~AnalogTriggerHandle() {
-    int32_t status = 0;
-    HAL_CleanAnalogTrigger(handle, &status);
-  }
-
-  operator HAL_AnalogTriggerHandle() const { return handle; }
-
- private:
-  HAL_AnalogTriggerHandle handle = 0;
+  explicit AnalogTriggerHandle(HAL_AnalogInputHandle port, int32_t* status)
+      : Handle{HAL_InitializeAnalogTrigger(port, status)} {}
 };
 
-struct DutyCycleHandle {
+struct DutyCycleHandle : hal::Handle<HAL_DutyCycleHandle, HAL_FreeDutyCycle> {
  public:
-  DutyCycleHandle(HAL_DigitalHandle port, int32_t* status) {
-    handle = HAL_InitializeDutyCycle(
-        port, HAL_AnalogTriggerType::HAL_Trigger_kInWindow, status);
-  }
-  DutyCycleHandle(const DutyCycleHandle&) = delete;
-  DutyCycleHandle operator=(const DutyCycleHandle&) = delete;
-
-  DutyCycleHandle(DutyCycleHandle&&) = default;
-  DutyCycleHandle& operator=(DutyCycleHandle&&) = default;
-
-  ~DutyCycleHandle() { HAL_FreeDutyCycle(handle); }
-
-  operator HAL_DutyCycleHandle() const { return handle; }
-
- private:
-  HAL_DutyCycleHandle handle = 0;
+  DutyCycleHandle(HAL_DigitalHandle port, int32_t* status)
+      : Handle{HAL_InitializeDutyCycle(
+            port, HAL_AnalogTriggerType::HAL_Trigger_kInWindow, status)} {}
 };
 
-struct DIOHandle {
+struct DIOHandle : hal::Handle<HAL_DigitalHandle, HAL_FreeDIOPort> {
  public:
   DIOHandle() {}
-  DIOHandle(const DIOHandle&) = delete;
-  DIOHandle operator=(const DIOHandle&) = delete;
-
-  DIOHandle(DIOHandle&&) = default;
-  DIOHandle& operator=(DIOHandle&&) = default;
-
-  DIOHandle(int32_t port, HAL_Bool input, int32_t* status) {
-    handle = HAL_InitializeDIOPort(HAL_GetPort(port), input, nullptr, status);
-  }
-
-  ~DIOHandle() { HAL_FreeDIOPort(handle); }
 
-  operator HAL_DigitalHandle() const { return handle; }
-
- private:
-  HAL_DigitalHandle handle = 0;
+  DIOHandle(int32_t port, HAL_Bool input, int32_t* status)
+      : Handle{
+            HAL_InitializeDIOPort(HAL_GetPort(port), input, nullptr, status)} {}
 };
 
-struct PWMHandle {
+struct PWMHandle : hal::Handle<HAL_DigitalHandle, HAL_FreePWMPort> {
  public:
   PWMHandle() {}
-  PWMHandle(const PWMHandle&) = delete;
-  PWMHandle operator=(const PWMHandle&) = delete;
-
-  PWMHandle(PWMHandle&&) = default;
-  PWMHandle& operator=(PWMHandle&&) = default;
-
-  PWMHandle(int32_t port, int32_t* status) {
-    handle = HAL_InitializePWMPort(HAL_GetPort(port), nullptr, status);
-  }
-
-  ~PWMHandle() {
-    int32_t status = 0;
-    HAL_FreePWMPort(handle, &status);
-  }
-
-  operator HAL_DigitalHandle() const { return handle; }
 
- private:
-  HAL_DigitalHandle handle = 0;
+  PWMHandle(int32_t port, int32_t* status)
+      : Handle{HAL_InitializePWMPort(HAL_GetPort(port), nullptr, status)} {}
 };
 
-struct RelayHandle {
+struct RelayHandle : hal::Handle<HAL_RelayHandle, HAL_FreeRelayPort> {
  public:
-  RelayHandle(int32_t port, HAL_Bool fwd, int32_t* status) {
-    handle = HAL_InitializeRelayPort(HAL_GetPort(port), fwd, nullptr, status);
-  }
-  RelayHandle(const RelayHandle&) = delete;
-  RelayHandle operator=(const RelayHandle&) = delete;
-
-  RelayHandle(RelayHandle&&) = default;
-  RelayHandle& operator=(RelayHandle&&) = default;
-
-  ~RelayHandle() { HAL_FreeRelayPort(handle); }
-
-  operator HAL_RelayHandle() const { return handle; }
-
- private:
-  HAL_RelayHandle handle = 0;
+  RelayHandle(int32_t port, HAL_Bool fwd, int32_t* status)
+      : Handle{
+            HAL_InitializeRelayPort(HAL_GetPort(port), fwd, nullptr, status)} {}
 };
 
 #define ASSERT_LAST_ERROR_STATUS(status, x)                          \

diff --git a/hal/src/main/native/athena/AnalogTrigger.cpp b/hal/src/main/native/athena/AnalogTrigger.cpp
@@ -99,8 +99,7 @@ HAL_AnalogTriggerHandle HAL_InitializeAnalogTriggerDutyCycle(
   return handle;
 }
 
-void HAL_CleanAnalogTrigger(HAL_AnalogTriggerHandle analogTriggerHandle,
-                            int32_t* status) {
+void HAL_CleanAnalogTrigger(HAL_AnalogTriggerHandle analogTriggerHandle) {
   analogTriggerHandles->Free(analogTriggerHandle);
   // caller owns the input handle.
 }

diff --git a/hal/src/main/native/athena/Counter.cpp b/hal/src/main/native/athena/Counter.cpp
@@ -63,7 +63,7 @@ HAL_CounterHandle HAL_InitializeCounter(HAL_Counter_Mode mode, int32_t* index,
   return handle;
 }
 
-void HAL_FreeCounter(HAL_CounterHandle counterHandle, int32_t* status) {
+void HAL_FreeCounter(HAL_CounterHandle counterHandle) {
   counterHandles->Free(counterHandle);
 }
 

diff --git a/hal/src/main/native/athena/DIO.cpp b/hal/src/main/native/athena/DIO.cpp
@@ -188,7 +188,7 @@ HAL_DigitalPWMHandle HAL_AllocateDigitalPWM(int32_t* status) {
   return handle;
 }
 
-void HAL_FreeDigitalPWM(HAL_DigitalPWMHandle pwmGenerator, int32_t* status) {
+void HAL_FreeDigitalPWM(HAL_DigitalPWMHandle pwmGenerator) {
   digitalPWMHandles->Free(pwmGenerator);
 }
 

diff --git a/hal/src/main/native/athena/Encoder.cpp b/hal/src/main/native/athena/Encoder.cpp
@@ -94,11 +94,9 @@ void Encoder::SetupCounter(HAL_Handle digitalSourceHandleA,
 
 Encoder::~Encoder() {
   if (m_counter != HAL_kInvalidHandle) {
-    int32_t status = 0;
-    HAL_FreeCounter(m_counter, &status);
+    HAL_FreeCounter(m_counter);
   } else {
-    int32_t status = 0;
-    HAL_FreeFPGAEncoder(m_encoder, &status);
+    HAL_FreeFPGAEncoder(m_encoder);
   }
 }
 
@@ -285,7 +283,7 @@ HAL_EncoderHandle HAL_InitializeEncoder(
   return handle;
 }
 
-void HAL_FreeEncoder(HAL_EncoderHandle encoderHandle, int32_t* status) {
+void HAL_FreeEncoder(HAL_EncoderHandle encoderHandle) {
   encoderHandles->Free(encoderHandle);
 }
 

diff --git a/hal/src/main/native/athena/FPGAEncoder.cpp b/hal/src/main/native/athena/FPGAEncoder.cpp
@@ -96,8 +96,7 @@ HAL_FPGAEncoderHandle HAL_InitializeFPGAEncoder(
   return handle;
 }
 
-void HAL_FreeFPGAEncoder(HAL_FPGAEncoderHandle fpgaEncoderHandle,
-                         int32_t* status) {
+void HAL_FreeFPGAEncoder(HAL_FPGAEncoderHandle fpgaEncoderHandle) {
   fpgaEncoderHandles->Free(fpgaEncoderHandle);
 }
 

diff --git a/hal/src/main/native/athena/FPGAEncoder.h b/hal/src/main/native/athena/FPGAEncoder.h
@@ -15,8 +15,7 @@ HAL_FPGAEncoderHandle HAL_InitializeFPGAEncoder(
     HAL_Handle digitalSourceHandleA, HAL_AnalogTriggerType analogTriggerTypeA,
     HAL_Handle digitalSourceHandleB, HAL_AnalogTriggerType analogTriggerTypeB,
     HAL_Bool reverseDirection, int32_t* index, int32_t* status);
-void HAL_FreeFPGAEncoder(HAL_FPGAEncoderHandle fpgaEncoderHandle,
-                         int32_t* status);
+void HAL_FreeFPGAEncoder(HAL_FPGAEncoderHandle fpgaEncoderHandle);
 
 /**
  * Reset the Encoder distance to zero.

diff --git a/hal/src/main/native/athena/Notifier.cpp b/hal/src/main/native/athena/Notifier.cpp
@@ -216,7 +216,7 @@ void HAL_StopNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) {
   notifier->cond.notify_all();  // wake up any waiting threads
 }
 
-void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) {
+void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle) {
   auto notifier = notifierHandles->Free(notifierHandle);
   if (!notifier) {
     return;
@@ -236,9 +236,9 @@ void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) {
     // the notifier can call back into our callback, so don't hold the lock
     // here (the atomic fetch_sub will prevent multiple parallel entries
     // into this function)
-
+    int32_t status = 0;
     if (notifierAlarm) {
-      notifierAlarm->writeEnable(false, status);
+      notifierAlarm->writeEnable(false, &status);
     }
     notifierRunning = false;
     hal::ReleaseFPGAInterrupt(kTimerInterruptNumber);

diff --git a/hal/src/main/native/athena/PWM.cpp b/hal/src/main/native/athena/PWM.cpp
@@ -10,6 +10,7 @@
 #include <thread>
 
 #include <fmt/format.h>
+#include <wpi/print.h>
 
 #include "ConstantsInternal.h"
 #include "DigitalInternal.h"
@@ -126,10 +127,9 @@ HAL_DigitalHandle HAL_InitializePWMPort(HAL_PortHandle portHandle,
 
   return handle;
 }
-void HAL_FreePWMPort(HAL_DigitalHandle pwmPortHandle, int32_t* status) {
+void HAL_FreePWMPort(HAL_DigitalHandle pwmPortHandle) {
   auto port = digitalChannelHandles->Get(pwmPortHandle, HAL_HandleEnum::PWM);
   if (port == nullptr) {
-    *status = HAL_HANDLE_ERROR;
     return;
   }
 
@@ -148,11 +148,17 @@ void HAL_FreePWMPort(HAL_DigitalHandle pwmPortHandle, int32_t* status) {
   }
 
   if (port->channel > tPWM::kNumHdrRegisters - 1) {
+    int32_t status = 0;
     int32_t bitToUnset = 1 << remapMXPPWMChannel(port->channel);
     uint16_t specialFunctions =
-        digitalSystem->readEnableMXPSpecialFunction(status);
+        digitalSystem->readEnableMXPSpecialFunction(&status);
     digitalSystem->writeEnableMXPSpecialFunction(specialFunctions & ~bitToUnset,
-                                                 status);
+                                                 &status);
+    if (status != 0) {
+      wpi::println(
+          "HAL_FreePWMPort: failed to free MXP PWM port {}, status code {}",
+          port->channel, status);
+    }
   }
 }
 

diff --git a/hal/src/main/native/athena/SerialPort.cpp b/hal/src/main/native/athena/SerialPort.cpp
@@ -141,7 +141,7 @@ HAL_SerialPortHandle HAL_InitializeSerialPortDirect(HAL_SerialPort port,
   return handle;
 }
 
-void HAL_CloseSerial(HAL_SerialPortHandle handle, int32_t* status) {
+void HAL_CloseSerial(HAL_SerialPortHandle handle) {
   auto port = serialPortHandles->Get(handle);
   serialPortHandles->Free(handle);
 

diff --git a/hal/src/main/native/cpp/jni/AddressableLEDJNI.cpp b/hal/src/main/native/cpp/jni/AddressableLEDJNI.cpp
@@ -41,7 +41,9 @@ JNIEXPORT void JNICALL
 Java_edu_wpi_first_hal_AddressableLEDJNI_free
   (JNIEnv* env, jclass, jint handle)
 {
-  HAL_FreeAddressableLED(static_cast<HAL_AddressableLEDHandle>(handle));
+  if (handle != HAL_kInvalidHandle) {
+    HAL_FreeAddressableLED(static_cast<HAL_AddressableLEDHandle>(handle));
+  }
 }
 
 /*

diff --git a/hal/src/main/native/cpp/jni/AnalogGyroJNI.cpp b/hal/src/main/native/cpp/jni/AnalogGyroJNI.cpp
@@ -57,7 +57,9 @@ JNIEXPORT void JNICALL
 Java_edu_wpi_first_hal_AnalogGyroJNI_freeAnalogGyro
   (JNIEnv* env, jclass, jint id)
 {
-  HAL_FreeAnalogGyro((HAL_GyroHandle)id);
+  if (id != HAL_kInvalidHandle) {
+    HAL_FreeAnalogGyro((HAL_GyroHandle)id);
+  }
 }
 
 /*