Miscellaneous cleanups for HAL, wpilibc, and wpilibj JNI (#589)

* Static functions in the HAL implementation were placed in the hal namespace
* "using namespace" declarations in HAL/cpp/Log.h and Timer.cpp were replaced
  with "using" declarations for std::chrono
* An extra include was removed from AnalogGyro.cpp
* InterruptableSensorBase's constructor was defaulted
* Newlines were added to some wpilibc integration tests for grouping
* A variable in HALUtil.h was renamed to follow the style guide

Supersedes #586

hal/include/HAL/cpp/Log.h

@@ -100,26 +100,27 @@ inline std::string NowTime() {
   llvm::SmallString<128> buf;
   llvm::raw_svector_ostream oss(buf);
 
-  using namespace std::chrono;
-  auto now = system_clock::now().time_since_epoch();
+  using std::chrono::duration_cast;
+
+  auto now = std::chrono::system_clock::now().time_since_epoch();
 
   // Hours
-  auto count = duration_cast<hours>(now).count() % 24;
+  auto count = duration_cast<std::chrono::hours>(now).count() % 24;
   if (count < 10) oss << "0";
   oss << count << ":";
 
   // Minutes
-  count = duration_cast<minutes>(now).count() % 60;
+  count = duration_cast<std::chrono::minutes>(now).count() % 60;
   if (count < 10) oss << "0";
   oss << count << ":";
 
   // Seconds
-  count = duration_cast<seconds>(now).count() % 60;
+  count = duration_cast<std::chrono::seconds>(now).count() % 60;
   if (count < 10) oss << "0";
   oss << count << ".";
 
   // Milliseconds
-  oss << duration_cast<milliseconds>(now).count() % 1000;
+  oss << duration_cast<std::chrono::milliseconds>(now).count() % 1000;
 
   return oss.str();
 }

hal/lib/athena/Accelerometer.cpp

@@ -77,6 +77,8 @@ enum Register {
   kReg_OffZ = 0x31
 };
 
+namespace hal {
+
 static void writeRegister(Register reg, uint8_t data);
 static uint8_t readRegister(Register reg);
 
@@ -182,6 +184,8 @@ static double unpackAxis(int16_t raw) {
   }
 }
 
+}  // namespace hal
+
 extern "C" {
 
 /**

hal/lib/athena/AnalogGyro.cpp

@@ -7,7 +7,6 @@
 
 #include "HAL/AnalogGyro.h"
 
-#include <chrono>
 #include <thread>
 
 #include "AnalogInternal.h"

hal/lib/athena/Power.cpp

@@ -13,6 +13,8 @@
 
 using namespace hal;
 
+namespace hal {
+
 static std::unique_ptr<tPower> power;
 
 static void initializePower(int32_t* status) {
@@ -21,6 +23,8 @@ static void initializePower(int32_t* status) {
   }
 }
 
+}  // namespace hal
+
 extern "C" {
 
 /**

wpilibc/athena/include/InterruptableSensorBase.h

@@ -22,8 +22,9 @@ class InterruptableSensorBase : public SensorBase {
     kBoth = 0x101,
   };
 
-  InterruptableSensorBase();
+  InterruptableSensorBase() = default;
   virtual ~InterruptableSensorBase() = default;
+
   virtual HAL_Handle GetPortHandleForRouting() const = 0;
   virtual AnalogTriggerType GetAnalogTriggerTypeForRouting() const = 0;
   virtual void RequestInterrupts(

wpilibc/athena/src/InterruptableSensorBase.cpp

@@ -13,8 +13,6 @@
 
 using namespace frc;
 
-InterruptableSensorBase::InterruptableSensorBase() {}
-
 /**
  * Request one of the 8 interrupts asynchronously on this digital input.
  *

wpilibc/athena/src/Timer.cpp

@@ -44,7 +44,10 @@ double GetClock() { return Timer::GetFPGATimestamp(); }
  *         on Saturday.
  */
 double GetTime() {
-  using namespace std::chrono;
+  using std::chrono::duration;
+  using std::chrono::duration_cast;
+  using std::chrono::system_clock;
+
   return duration_cast<duration<double>>(system_clock::now().time_since_epoch())
       .count();
 }

wpilibcIntegrationTests/src/CounterTest.cpp

@@ -63,44 +63,56 @@ class CounterTest : public testing::Test {
  */
 TEST_F(CounterTest, CountTalon) {
   Reset();
+
   /* Run the motor forward and determine if the counter is counting. */
   m_talon->Set(1.0);
   Wait(0.5);
+
   EXPECT_NE(0.0, m_talonCounter->Get()) << "The counter did not count (talon)";
+
   /* Set the motor to 0 and determine if the counter resets to 0. */
   m_talon->Set(0.0);
   Wait(0.5);
   m_talonCounter->Reset();
+
   EXPECT_FLOAT_EQ(0.0, m_talonCounter->Get())
       << "The counter did not restart to 0 (talon)";
 }
 
 TEST_F(CounterTest, CountVictor) {
   Reset();
+
   /* Run the motor forward and determine if the counter is counting. */
   m_victor->Set(1.0);
   Wait(0.5);
+
   EXPECT_NE(0.0, m_victorCounter->Get())
       << "The counter did not count (victor)";
+
   /* Set the motor to 0 and determine if the counter resets to 0. */
   m_victor->Set(0.0);
   Wait(0.5);
   m_victorCounter->Reset();
+
   EXPECT_FLOAT_EQ(0.0, m_victorCounter->Get())
       << "The counter did not restart to 0 (jaguar)";
 }
 
 TEST_F(CounterTest, CountJaguar) {
   Reset();
+
   /* Run the motor forward and determine if the counter is counting. */
   m_jaguar->Set(1.0);
   Wait(0.5);
+
   EXPECT_NE(0.0, m_jaguarCounter->Get())
       << "The counter did not count (jaguar)";
+
   /* Set the motor to 0 and determine if the counter resets to 0. */
   m_jaguar->Set(0.0);
   Wait(0.5);
   m_jaguarCounter->Reset();
+
   EXPECT_FLOAT_EQ(0.0, m_jaguarCounter->Get())
       << "The counter did not restart to 0 (jaguar)";
 }
@@ -111,42 +123,54 @@ TEST_F(CounterTest, CountJaguar) {
  */
 TEST_F(CounterTest, TalonGetStopped) {
   Reset();
+
   /* Set the Max Period of the counter and run the motor */
   m_talonCounter->SetMaxPeriod(kMaxPeriod);
   m_talon->Set(1.0);
   Wait(0.5);
+
   EXPECT_FALSE(m_talonCounter->GetStopped()) << "The counter did not count.";
+
   /* Stop the motor and wait until the Max Period is exceeded */
   m_talon->Set(0.0);
   Wait(kMotorDelay);
+
   EXPECT_TRUE(m_talonCounter->GetStopped())
       << "The counter did not stop counting.";
 }
 
 TEST_F(CounterTest, VictorGetStopped) {
   Reset();
+
   /* Set the Max Period of the counter and run the motor */
   m_victorCounter->SetMaxPeriod(kMaxPeriod);
   m_victor->Set(1.0);
   Wait(0.5);
+
   EXPECT_FALSE(m_victorCounter->GetStopped()) << "The counter did not count.";
+
   /* Stop the motor and wait until the Max Period is exceeded */
   m_victor->Set(0.0);
   Wait(kMotorDelay);
+
   EXPECT_TRUE(m_victorCounter->GetStopped())
       << "The counter did not stop counting.";
 }
 
 TEST_F(CounterTest, JaguarGetStopped) {
   Reset();
+
   /* Set the Max Period of the counter and run the motor */
   m_jaguarCounter->SetMaxPeriod(kMaxPeriod);
   m_jaguar->Set(1.0);
   Wait(0.5);
+
   EXPECT_FALSE(m_jaguarCounter->GetStopped()) << "The counter did not count.";
+
   /* Stop the motor and wait until the Max Period is exceeded */
   m_jaguar->Set(0.0);
   Wait(kMotorDelay);
+
   EXPECT_TRUE(m_jaguarCounter->GetStopped())
       << "The counter did not stop counting.";
 }

wpilibcIntegrationTests/src/FilterNoiseTest.cpp

@@ -34,8 +34,6 @@ std::ostream& operator<<(std::ostream& os, const FilterNoiseTestType& type) {
   return os;
 }
 
-using std::chrono::system_clock;
-
 constexpr double kStdDev = 10.0;
 
 /**

wpilibcIntegrationTests/src/MotorInvertingTest.cpp

@@ -38,6 +38,7 @@ class MotorInvertingTest
  protected:
   SpeedController* m_speedController;
   Encoder* m_encoder;
+
   void SetUp() override {
     switch (GetParam()) {
       case TEST_VICTOR:
@@ -59,6 +60,7 @@ class MotorInvertingTest
         break;
     }
   }
+
   void TearDown() override {
     delete m_speedController;
     delete m_encoder;
@@ -73,54 +75,82 @@ class MotorInvertingTest
 
 TEST_P(MotorInvertingTest, InvertingPositive) {
   Reset();
+
   m_speedController->Set(motorSpeed);
+
   Wait(delayTime);
+
   bool initDirection = m_encoder->GetDirection();
   m_speedController->SetInverted(true);
   m_speedController->Set(motorSpeed);
+
   Wait(delayTime);
+
   EXPECT_TRUE(m_encoder->GetDirection() != initDirection)
       << "Inverting with Positive value does not change direction";
+
   Reset();
 }
+
 TEST_P(MotorInvertingTest, InvertingNegative) {
   Reset();
+
   m_speedController->SetInverted(false);
   m_speedController->Set(-motorSpeed);
+
   Wait(delayTime);
+
   bool initDirection = m_encoder->GetDirection();
   m_speedController->SetInverted(true);
   m_speedController->Set(-motorSpeed);
+
   Wait(delayTime);
+
   EXPECT_TRUE(m_encoder->GetDirection() != initDirection)
       << "Inverting with Negative value does not change direction";
+
   Reset();
 }
+
 TEST_P(MotorInvertingTest, InvertingSwitchingPosToNeg) {
   Reset();
+
   m_speedController->SetInverted(false);
   m_speedController->Set(motorSpeed);
+
   Wait(delayTime);
+
   bool initDirection = m_encoder->GetDirection();
   m_speedController->SetInverted(true);
   m_speedController->Set(-motorSpeed);
+
   Wait(delayTime);
+
   EXPECT_TRUE(m_encoder->GetDirection() == initDirection)
       << "Inverting with Switching value does change direction";
+
   Reset();
 }
+
 TEST_P(MotorInvertingTest, InvertingSwitchingNegToPos) {
   Reset();
+
   m_speedController->SetInverted(false);
   m_speedController->Set(-motorSpeed);
+
   Wait(delayTime);
+
   bool initDirection = m_encoder->GetDirection();
   m_speedController->SetInverted(true);
   m_speedController->Set(motorSpeed);
+
   Wait(delayTime);
+
   EXPECT_TRUE(m_encoder->GetDirection() == initDirection)
       << "Inverting with Switching value does change direction";
+
   Reset();
 }
+
 INSTANTIATE_TEST_CASE_P(Test, MotorInvertingTest,
                         testing::Values(TEST_VICTOR, TEST_JAGUAR, TEST_TALON));

wpilibcIntegrationTests/src/PIDToleranceTest.cpp

@@ -19,42 +19,57 @@ class PIDToleranceTest : public testing::Test {
   const double setpoint = 50.0;
   const double range = 200;
   const double tolerance = 10.0;
+
   class fakeInput : public PIDSource {
    public:
     double val = 0;
+
     void SetPIDSourceType(PIDSourceType pidSource) {}
+
     PIDSourceType GetPIDSourceType() { return PIDSourceType::kDisplacement; }
+
     double PIDGet() { return val; }
   };
+
   class fakeOutput : public PIDOutput {
     void PIDWrite(double output) {}
   };
+
   fakeInput inp;
   fakeOutput out;
   PIDController* pid;
+
   void SetUp() override {
     pid = new PIDController(0.5, 0.0, 0.0, &inp, &out);
     pid->SetInputRange(-range / 2, range / 2);
   }
+
   void TearDown() override { delete pid; }
+
   void Reset() { inp.val = 0; }
 };
 
 TEST_F(PIDToleranceTest, Absolute) {
   Reset();
+
   pid->SetAbsoluteTolerance(tolerance);
   pid->SetSetpoint(setpoint);
   pid->Enable();
+
   EXPECT_FALSE(pid->OnTarget())
       << "Error was in tolerance when it should not have been. Error was "
       << pid->GetAvgError();
+
   inp.val = setpoint + tolerance / 2;
   Wait(1.0);
+
   EXPECT_TRUE(pid->OnTarget())
       << "Error was not in tolerance when it should have been. Error was "
       << pid->GetAvgError();
+
   inp.val = setpoint + 10 * tolerance;
   Wait(1.0);
+
   EXPECT_FALSE(pid->OnTarget())
       << "Error was in tolerance when it should not have been. Error was "
       << pid->GetAvgError();
@@ -62,23 +77,30 @@ TEST_F(PIDToleranceTest, Absolute) {
 
 TEST_F(PIDToleranceTest, Percent) {
   Reset();
+
   pid->SetPercentTolerance(tolerance);
   pid->SetSetpoint(setpoint);
   pid->Enable();
+
   EXPECT_FALSE(pid->OnTarget())
       << "Error was in tolerance when it should not have been. Error was "
       << pid->GetAvgError();
+
   inp.val = setpoint +
             (tolerance) / 200 *
                 range;  // half of percent tolerance away from setpoint
   Wait(1.0);
+
   EXPECT_TRUE(pid->OnTarget())
       << "Error was not in tolerance when it should have been. Error was "
       << pid->GetAvgError();
+
   inp.val =
       setpoint +
       (tolerance) / 50 * range;  // double percent tolerance away from setPoint
+
   Wait(1.0);
+
   EXPECT_FALSE(pid->OnTarget())
       << "Error was in tolerance when it should not have been. Error was "
       << pid->GetAvgError();

wpilibcIntegrationTests/src/VisionTest.cpp

@@ -12,6 +12,7 @@ using namespace frc;
 class VisionTester : public VisionPipeline {
  public:
   virtual ~VisionTester() = default;
+
   void Process(cv::Mat& mat) override {}
   void TestThing() {}
 };

wpilibj/src/athena/cpp/lib/HALUtil.cpp

@@ -86,13 +86,13 @@ constexpr const char JNI_wpilibjPrefix[] = "edu.wpi.first.wpilibj";
 extern const char JNI_wpilibjPrefix[] = "edu.wpi.first.wpilibj";
 #endif
 
-void ReportError(JNIEnv *env, int32_t status, bool do_throw) {
+void ReportError(JNIEnv *env, int32_t status, bool doThrow) {
   if (status == 0) return;
   if (status == HAL_HANDLE_ERROR) {
     ThrowHalHandleException(env, status);
   }
   const char *message = HAL_GetErrorMessage(status);
-  if (do_throw && status < 0) {
+  if (doThrow && status < 0) {
     llvm::SmallString<1024> buf;
     llvm::raw_svector_ostream oss(buf);
     oss << " Code: " << status << ". " << message;

wpilibj/src/athena/cpp/lib/HALUtil.h

@@ -16,13 +16,13 @@ extern JavaVM *jvm;
 
 namespace frc {
 
-void ReportError(JNIEnv *env, int32_t status, bool do_throw = true);
+void ReportError(JNIEnv *env, int32_t status, bool doThrow = true);
                  
 void ThrowError(JNIEnv *env, int32_t status, int32_t minRange, int32_t maxRange, 
                 int32_t requestedValue);
 
-inline bool CheckStatus(JNIEnv *env, int32_t status, bool do_throw = true) {
-  if (status != 0) ReportError(env, status, do_throw);
+inline bool CheckStatus(JNIEnv *env, int32_t status, bool doThrow = true) {
+  if (status != 0) ReportError(env, status, doThrow);
   return status == 0;
 }
 

wpilibj/src/athena/cpp/lib/SolenoidJNI.cpp

@@ -48,7 +48,7 @@ Java_edu_wpi_first_wpilibj_hal_SolenoidJNI_initializeSolenoidPort(
 
   // Use solenoid channels, as we have to pick one.
   CheckStatusRange(env, status, 0, HAL_GetNumSolenoidChannels(),
-                   hal::getPortHandleChannel((HAL_PortHandle)id));;
+                   hal::getPortHandleChannel((HAL_PortHandle)id));
   return (jint)handle;
 }
 

wpilibj/src/athena/cpp/lib/ThreadsJNI.cpp

@@ -70,4 +70,4 @@ JNIEXPORT jboolean JNICALL Java_edu_wpi_first_wpilibj_hal_ThreadsJNI_setCurrentT
   return (jboolean)ret;
 }
 
-}
+}