[hal, wpilib] Switch PCM to be a single object that is allowed to be duplicated (#3475)

Having PCM as a singleton is a problem, as multiple things need to use it, and that gets really ugly. This changes PCM's to be a reference counted object, that can be passed around and constructed from multiple places.

In Java, this is using a map to hold a data store with a ref count, and allocating new objects any time a duplicate is requested.

In C++, this uses a trick constructor to store a PCM instance in the data store itself. This instance can then be passed to base objects using std::shared_ptr's aliasing constructor, which means constructing a solenoid from a PCM is not allocating after the 1st one.

This did require removing sendable from PCM. A compressor class was added back in to act as sendable for the PCM.

After this change is finished, the only change RobotBuilder and Team Code would require is passing a module type to solenoid constructors.

Co-authored-by: sciencewhiz <sciencewhiz@users.noreply.github.com>

wpilibc/src/main/native/cpp/Compressor.cpp

@@ -0,0 +1,72 @@
+// Copyright (c) FIRST and other WPILib contributors.
+// Open Source Software; you can modify and/or share it under the terms of
+// the WPILib BSD license file in the root directory of this project.
+
+#include "frc/Compressor.h"
+
+#include <hal/FRCUsageReporting.h>
+#include <hal/Ports.h>
+#include <wpi/sendable/SendableBuilder.h>
+#include <wpi/sendable/SendableRegistry.h>
+
+#include "frc/Errors.h"
+
+using namespace frc;
+
+Compressor::Compressor(int module, PneumaticsModuleType moduleType)
+    : m_module{PneumaticsBase::GetForType(module, moduleType)} {
+  if (!m_module->ReserveCompressor()) {
+    throw FRC_MakeError(err::ResourceAlreadyAllocated, "{}", module);
+  }
+
+  SetClosedLoopControl(true);
+
+  HAL_Report(HALUsageReporting::kResourceType_Compressor, module + 1);
+  wpi::SendableRegistry::AddLW(this, "Compressor", module);
+}
+
+Compressor::Compressor(PneumaticsModuleType moduleType)
+    : Compressor{PneumaticsBase::GetDefaultForType(moduleType), moduleType} {}
+
+Compressor::~Compressor() {
+  m_module->UnreserveCompressor();
+}
+
+void Compressor::Start() {
+  SetClosedLoopControl(true);
+}
+
+void Compressor::Stop() {
+  SetClosedLoopControl(false);
+}
+
+bool Compressor::Enabled() const {
+  return m_module->GetCompressor();
+}
+
+bool Compressor::GetPressureSwitchValue() const {
+  return m_module->GetPressureSwitch();
+}
+
+double Compressor::GetCompressorCurrent() const {
+  return m_module->GetCompressorCurrent();
+}
+
+void Compressor::SetClosedLoopControl(bool on) {
+  m_module->SetClosedLoopControl(on);
+}
+
+bool Compressor::GetClosedLoopControl() const {
+  return m_module->GetClosedLoopControl();
+}
+
+void Compressor::InitSendable(wpi::SendableBuilder& builder) {
+  builder.SetSmartDashboardType("Compressor");
+  builder.AddBooleanProperty(
+      "Closed Loop Control", [=]() { return GetClosedLoopControl(); },
+      [=](bool value) { SetClosedLoopControl(value); });
+  builder.AddBooleanProperty(
+      "Enabled", [=] { return Enabled(); }, nullptr);
+  builder.AddBooleanProperty(
+      "Pressure switch", [=]() { return GetPressureSwitchValue(); }, nullptr);
+}

wpilibc/src/main/native/cpp/DoubleSolenoid.cpp

@@ -18,33 +18,20 @@
 
 using namespace frc;
 
-DoubleSolenoid::DoubleSolenoid(PneumaticsBase& module, int forwardChannel,
-                               int reverseChannel)
-    : DoubleSolenoid{std::shared_ptr<PneumaticsBase>{
-                         &module, wpi::NullDeleter<PneumaticsBase>()},
-                     forwardChannel, reverseChannel} {}
-
-DoubleSolenoid::DoubleSolenoid(PneumaticsBase* module, int forwardChannel,
-                               int reverseChannel)
-    : DoubleSolenoid{std::shared_ptr<PneumaticsBase>{
-                         module, wpi::NullDeleter<PneumaticsBase>()},
-                     forwardChannel, reverseChannel} {}
-
-DoubleSolenoid::DoubleSolenoid(std::shared_ptr<PneumaticsBase> module,
+DoubleSolenoid::DoubleSolenoid(int module, PneumaticsModuleType moduleType,
                                int forwardChannel, int reverseChannel)
-    : m_module{std::move(module)} {
-  if (!m_module->CheckSolenoidChannel(forwardChannel)) {
+    : m_module{PneumaticsBase::GetForType(module, moduleType)},
+      m_forwardChannel{forwardChannel},
+      m_reverseChannel{reverseChannel} {
+  if (!m_module->CheckSolenoidChannel(m_forwardChannel)) {
     throw FRC_MakeError(err::ChannelIndexOutOfRange, "Channel {}",
-                        forwardChannel);
+                        m_forwardChannel);
   }
-  if (!m_module->CheckSolenoidChannel(reverseChannel)) {
+  if (!m_module->CheckSolenoidChannel(m_reverseChannel)) {
     throw FRC_MakeError(err::ChannelIndexOutOfRange, "Channel {}",
-                        reverseChannel);
+                        m_reverseChannel);
   }
 
-  m_forwardChannel = forwardChannel;
-  m_reverseChannel = reverseChannel;
-
   m_forwardMask = 1 << forwardChannel;
   m_reverseMask = 1 << reverseChannel;
   m_mask = m_forwardMask | m_reverseMask;
@@ -72,6 +59,11 @@ DoubleSolenoid::DoubleSolenoid(std::shared_ptr<PneumaticsBase> module,
                                m_module->GetModuleNumber(), m_forwardChannel);
 }
 
+DoubleSolenoid::DoubleSolenoid(PneumaticsModuleType moduleType,
+                               int forwardChannel, int reverseChannel)
+    : DoubleSolenoid{PneumaticsBase::GetDefaultForType(moduleType), moduleType,
+                     forwardChannel, reverseChannel} {}
+
 DoubleSolenoid::~DoubleSolenoid() {
   m_module->UnreserveSolenoids(m_mask);
 }

wpilibc/src/main/native/cpp/PneumaticsBase.cpp

@@ -0,0 +1,26 @@
+// Copyright (c) FIRST and other WPILib contributors.
+// Open Source Software; you can modify and/or share it under the terms of
+// the WPILib BSD license file in the root directory of this project.
+
+#include "frc/PneumaticsBase.h"
+
+#include "frc/Errors.h"
+#include "frc/PneumaticsControlModule.h"
+#include "frc/SensorUtil.h"
+
+using namespace frc;
+
+std::shared_ptr<PneumaticsBase> PneumaticsBase::GetForType(
+    int module, PneumaticsModuleType moduleType) {
+  if (moduleType == PneumaticsModuleType::CTREPCM) {
+    return PneumaticsControlModule::GetForModule(module);
+  }
+  throw FRC_MakeError(err::InvalidParameter, "{}", moduleType);
+}
+
+int PneumaticsBase::GetDefaultForType(PneumaticsModuleType moduleType) {
+  if (moduleType == PneumaticsModuleType::CTREPCM) {
+    return SensorUtil::GetDefaultCTREPCMModule();
+  }
+  throw FRC_MakeError(err::InvalidParameter, "{}", moduleType);
+}

wpilibc/src/main/native/cpp/PneumaticsControlModule.cpp

@@ -5,32 +5,79 @@
 #include "frc/PneumaticsControlModule.h"
 
 #include <hal/CTREPCM.h>
+#include <wpi/NullDeleter.h>
 #include <wpi/StackTrace.h>
-#include <wpi/sendable/SendableBuilder.h>
-#include <wpi/sendable/SendableRegistry.h>
 
+#include "frc/Compressor.h"
+#include "frc/DoubleSolenoid.h"
 #include "frc/Errors.h"
 #include "frc/SensorUtil.h"
+#include "frc/Solenoid.h"
 
 using namespace frc;
 
+wpi::mutex PneumaticsControlModule::m_handleLock;
+std::unique_ptr<
+    wpi::DenseMap<int, std::weak_ptr<PneumaticsControlModule::DataStore>>>
+    PneumaticsControlModule::m_handleMap = nullptr;
+
+// Always called under lock, so we can avoid the double lock from the magic
+// static
+std::weak_ptr<PneumaticsControlModule::DataStore>&
+PneumaticsControlModule::GetDataStore(int module) {
+  if (!m_handleMap) {
+    m_handleMap = std::make_unique<wpi::DenseMap<
+        int, std::weak_ptr<PneumaticsControlModule::DataStore>>>();
+  }
+  return (*m_handleMap)[module];
+}
+
+class PneumaticsControlModule::DataStore {
+ public:
+  explicit DataStore(int module, const char* stackTrace) {
+    int32_t status = 0;
+    HAL_CTREPCMHandle handle =
+        HAL_InitializeCTREPCM(module, stackTrace, &status);
+    FRC_CheckErrorStatus(status, "Module {}", module);
+    m_moduleObject = PneumaticsControlModule{handle, module};
+    m_moduleObject.m_dataStore =
+        std::shared_ptr<DataStore>{this, wpi::NullDeleter<DataStore>()};
+  }
+
+  ~DataStore() noexcept { HAL_FreeCTREPCM(m_moduleObject.m_handle); }
+
+  DataStore(DataStore&&) = delete;
+  DataStore& operator=(DataStore&&) = delete;
+
+ private:
+  friend class PneumaticsControlModule;
+  uint32_t m_reservedMask{0};
+  bool m_compressorReserved{false};
+  wpi::mutex m_reservedLock;
+  PneumaticsControlModule m_moduleObject{HAL_kInvalidHandle, 0};
+};
+
 PneumaticsControlModule::PneumaticsControlModule()
     : PneumaticsControlModule{SensorUtil::GetDefaultCTREPCMModule()} {}
 
 PneumaticsControlModule::PneumaticsControlModule(int module) {
-  int32_t status = 0;
   std::string stackTrace = wpi::GetStackTrace(1);
-  m_handle = HAL_InitializeCTREPCM(module, stackTrace.c_str(), &status);
-  FRC_CheckErrorStatus(status, "Module {}", module);
+  std::scoped_lock lock(m_handleLock);
+  auto& res = GetDataStore(module);
+  m_dataStore = res.lock();
+  if (!m_dataStore) {
+    m_dataStore = std::make_shared<DataStore>(module, stackTrace.c_str());
+    res = m_dataStore;
+  }
+  m_handle = m_dataStore->m_moduleObject.m_handle;
   m_module = module;
-  wpi::SendableRegistry::AddLW(this, "Compressor", module);
 }
 
-PneumaticsControlModule::~PneumaticsControlModule() {
-  HAL_FreeCTREPCM(m_handle);
-}
+PneumaticsControlModule::PneumaticsControlModule(HAL_CTREPCMHandle handle,
+                                                 int module)
+    : m_handle{handle}, m_module{module} {}
 
-bool PneumaticsControlModule::GetCompressor() {
+bool PneumaticsControlModule::GetCompressor() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressor(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
@@ -43,59 +90,59 @@ void PneumaticsControlModule::SetClosedLoopControl(bool enabled) {
   FRC_CheckErrorStatus(status, "Module {}", m_module);
 }
 
-bool PneumaticsControlModule::GetClosedLoopControl() {
+bool PneumaticsControlModule::GetClosedLoopControl() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMClosedLoopControl(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
 
-bool PneumaticsControlModule::GetPressureSwitch() {
+bool PneumaticsControlModule::GetPressureSwitch() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMPressureSwitch(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
 
-double PneumaticsControlModule::GetCompressorCurrent() {
+double PneumaticsControlModule::GetCompressorCurrent() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressorCurrent(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
 
-bool PneumaticsControlModule::GetCompressorCurrentTooHighFault() {
+bool PneumaticsControlModule::GetCompressorCurrentTooHighFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressorCurrentTooHighFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetCompressorCurrentTooHighStickyFault() {
+bool PneumaticsControlModule::GetCompressorCurrentTooHighStickyFault() const {
   int32_t status = 0;
   auto result =
       HAL_GetCTREPCMCompressorCurrentTooHighStickyFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetCompressorShortedFault() {
+bool PneumaticsControlModule::GetCompressorShortedFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressorShortedFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetCompressorShortedStickyFault() {
+bool PneumaticsControlModule::GetCompressorShortedStickyFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressorShortedStickyFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetCompressorNotConnectedFault() {
+bool PneumaticsControlModule::GetCompressorNotConnectedFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMCompressorNotConnectedFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetCompressorNotConnectedStickyFault() {
+bool PneumaticsControlModule::GetCompressorNotConnectedStickyFault() const {
   int32_t status = 0;
   auto result =
       HAL_GetCTREPCMCompressorNotConnectedStickyFault(m_handle, &status);
@@ -103,13 +150,13 @@ bool PneumaticsControlModule::GetCompressorNotConnectedStickyFault() {
   return result;
 }
 
-bool PneumaticsControlModule::GetSolenoidVoltageFault() {
+bool PneumaticsControlModule::GetSolenoidVoltageFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMSolenoidVoltageFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
   return result;
 }
-bool PneumaticsControlModule::GetSolenoidVoltageStickyFault() {
+bool PneumaticsControlModule::GetSolenoidVoltageStickyFault() const {
   int32_t status = 0;
   auto result = HAL_GetCTREPCMSolenoidVoltageStickyFault(m_handle, &status);
   FRC_CheckErrorStatus(status, "Module {}", m_module);
@@ -165,27 +212,58 @@ bool PneumaticsControlModule::CheckSolenoidChannel(int channel) const {
 }
 
 int PneumaticsControlModule::CheckAndReserveSolenoids(int mask) {
-  std::scoped_lock lock{m_reservedLock};
+  std::scoped_lock lock{m_dataStore->m_reservedLock};
   uint32_t uMask = static_cast<uint32_t>(mask);
-  if ((m_reservedMask & uMask) != 0) {
-    return m_reservedMask & uMask;
+  if ((m_dataStore->m_reservedMask & uMask) != 0) {
+    return m_dataStore->m_reservedMask & uMask;
   }
-  m_reservedMask |= uMask;
+  m_dataStore->m_reservedMask |= uMask;
   return 0;
 }
 
 void PneumaticsControlModule::UnreserveSolenoids(int mask) {
-  std::scoped_lock lock{m_reservedLock};
-  m_reservedMask &= ~(static_cast<uint32_t>(mask));
+  std::scoped_lock lock{m_dataStore->m_reservedLock};
+  m_dataStore->m_reservedMask &= ~(static_cast<uint32_t>(mask));
 }
 
-void PneumaticsControlModule::InitSendable(wpi::SendableBuilder& builder) {
-  builder.SetSmartDashboardType("Compressor");
-  builder.AddBooleanProperty(
-      "Closed Loop Control", [=]() { return GetClosedLoopControl(); },
-      [=](bool value) { SetClosedLoopControl(value); });
-  builder.AddBooleanProperty(
-      "Enabled", [=] { return GetCompressor(); }, nullptr);
-  builder.AddBooleanProperty(
-      "Pressure switch", [=]() { return GetPressureSwitch(); }, nullptr);
+bool PneumaticsControlModule::ReserveCompressor() {
+  std::scoped_lock lock{m_dataStore->m_reservedLock};
+  if (m_dataStore->m_compressorReserved) {
+    return false;
+  }
+  m_dataStore->m_compressorReserved = true;
+  return true;
+}
+
+void PneumaticsControlModule::UnreserveCompressor() {
+  std::scoped_lock lock{m_dataStore->m_reservedLock};
+  m_dataStore->m_compressorReserved = false;
+}
+
+Solenoid PneumaticsControlModule::MakeSolenoid(int channel) {
+  return Solenoid{m_module, PneumaticsModuleType::CTREPCM, channel};
+}
+
+DoubleSolenoid PneumaticsControlModule::MakeDoubleSolenoid(int forwardChannel,
+                                                           int reverseChannel) {
+  return DoubleSolenoid{m_module, PneumaticsModuleType::CTREPCM, forwardChannel,
+                        reverseChannel};
+}
+
+Compressor PneumaticsControlModule::MakeCompressor() {
+  return Compressor{m_module, PneumaticsModuleType::CTREPCM};
+}
+
+std::shared_ptr<PneumaticsBase> PneumaticsControlModule::GetForModule(
+    int module) {
+  std::string stackTrace = wpi::GetStackTrace(1);
+  std::scoped_lock lock(m_handleLock);
+  auto& res = GetDataStore(module);
+  std::shared_ptr<DataStore> dataStore = res.lock();
+  if (!dataStore) {
+    dataStore = std::make_shared<DataStore>(module, stackTrace.c_str());
+    res = dataStore;
+  }
+
+  return std::shared_ptr<PneumaticsBase>{dataStore, &dataStore->m_moduleObject};
 }

wpilibc/src/main/native/cpp/Solenoid.cpp

@@ -16,22 +16,12 @@
 
 using namespace frc;
 
-Solenoid::Solenoid(PneumaticsBase& module, int channel)
-    : Solenoid{std::shared_ptr<PneumaticsBase>{
-                   &module, wpi::NullDeleter<PneumaticsBase>()},
-               channel} {}
-
-Solenoid::Solenoid(PneumaticsBase* module, int channel)
-    : Solenoid{std::shared_ptr<PneumaticsBase>{
-                   module, wpi::NullDeleter<PneumaticsBase>()},
-               channel} {}
-
-Solenoid::Solenoid(std::shared_ptr<PneumaticsBase> module, int channel)
-    : m_module{std::move(module)} {
-  if (!m_module->CheckSolenoidChannel(channel)) {
-    throw FRC_MakeError(err::ChannelIndexOutOfRange, "Channel {}", channel);
+Solenoid::Solenoid(int module, PneumaticsModuleType moduleType, int channel)
+    : m_module{PneumaticsBase::GetForType(module, moduleType)},
+      m_channel{channel} {
+  if (!m_module->CheckSolenoidChannel(m_channel)) {
+    throw FRC_MakeError(err::ChannelIndexOutOfRange, "Channel {}", m_channel);
   }
-  m_channel = channel;
   m_mask = 1 << channel;
 
   if (m_module->CheckAndReserveSolenoids(m_mask) != 0) {
@@ -44,6 +34,10 @@ Solenoid::Solenoid(std::shared_ptr<PneumaticsBase> module, int channel)
                                m_channel);
 }
 
+Solenoid::Solenoid(PneumaticsModuleType moduleType, int channel)
+    : Solenoid{PneumaticsBase::GetDefaultForType(moduleType), moduleType,
+               channel} {}
+
 Solenoid::~Solenoid() {
   m_module->UnreserveSolenoids(m_mask);
 }