From d682295ccd39d47d1eced3ad68bfb9aa4e947836 Mon Sep 17 00:00:00 2001 From: Tyler Veness Date: Mon, 7 Aug 2017 17:36:34 -0700 Subject: [PATCH] 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 | 13 ++++---- hal/lib/athena/Accelerometer.cpp | 4 +++ hal/lib/athena/AnalogGyro.cpp | 1 - hal/lib/athena/Power.cpp | 4 +++ .../athena/include/InterruptableSensorBase.h | 3 +- .../athena/src/InterruptableSensorBase.cpp | 2 -- wpilibc/athena/src/Timer.cpp | 5 +++- wpilibcIntegrationTests/src/CounterTest.cpp | 24 +++++++++++++++ .../src/FilterNoiseTest.cpp | 2 -- .../src/MotorInvertingTest.cpp | 30 +++++++++++++++++++ .../src/PIDToleranceTest.cpp | 22 ++++++++++++++ wpilibcIntegrationTests/src/VisionTest.cpp | 1 + wpilibj/src/athena/cpp/lib/HALUtil.cpp | 4 +-- wpilibj/src/athena/cpp/lib/HALUtil.h | 6 ++-- wpilibj/src/athena/cpp/lib/SolenoidJNI.cpp | 2 +- wpilibj/src/athena/cpp/lib/ThreadsJNI.cpp | 2 +- 16 files changed, 105 insertions(+), 20 deletions(-) diff --git a/hal/include/HAL/cpp/Log.h b/hal/include/HAL/cpp/Log.h index cfda80361f..7100320a84 100644 --- a/hal/include/HAL/cpp/Log.h +++ b/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(now).count() % 24; + auto count = duration_cast(now).count() % 24; if (count < 10) oss << "0"; oss << count << ":"; // Minutes - count = duration_cast(now).count() % 60; + count = duration_cast(now).count() % 60; if (count < 10) oss << "0"; oss << count << ":"; // Seconds - count = duration_cast(now).count() % 60; + count = duration_cast(now).count() % 60; if (count < 10) oss << "0"; oss << count << "."; // Milliseconds - oss << duration_cast(now).count() % 1000; + oss << duration_cast(now).count() % 1000; return oss.str(); } diff --git a/hal/lib/athena/Accelerometer.cpp b/hal/lib/athena/Accelerometer.cpp index 6c912f370e..5386db1af6 100644 --- a/hal/lib/athena/Accelerometer.cpp +++ b/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" { /** diff --git a/hal/lib/athena/AnalogGyro.cpp b/hal/lib/athena/AnalogGyro.cpp index 8d3d82d3d9..694c61d233 100644 --- a/hal/lib/athena/AnalogGyro.cpp +++ b/hal/lib/athena/AnalogGyro.cpp @@ -7,7 +7,6 @@ #include "HAL/AnalogGyro.h" -#include #include #include "AnalogInternal.h" diff --git a/hal/lib/athena/Power.cpp b/hal/lib/athena/Power.cpp index 4d66fe2775..2704633957 100644 --- a/hal/lib/athena/Power.cpp +++ b/hal/lib/athena/Power.cpp @@ -13,6 +13,8 @@ using namespace hal; +namespace hal { + static std::unique_ptr power; static void initializePower(int32_t* status) { @@ -21,6 +23,8 @@ static void initializePower(int32_t* status) { } } +} // namespace hal + extern "C" { /** diff --git a/wpilibc/athena/include/InterruptableSensorBase.h b/wpilibc/athena/include/InterruptableSensorBase.h index d3df64f685..a55ca5cbbc 100644 --- a/wpilibc/athena/include/InterruptableSensorBase.h +++ b/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( diff --git a/wpilibc/athena/src/InterruptableSensorBase.cpp b/wpilibc/athena/src/InterruptableSensorBase.cpp index 7545b1b84b..b5d1e5f96d 100644 --- a/wpilibc/athena/src/InterruptableSensorBase.cpp +++ b/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. * diff --git a/wpilibc/athena/src/Timer.cpp b/wpilibc/athena/src/Timer.cpp index e481d05410..f33641916f 100644 --- a/wpilibc/athena/src/Timer.cpp +++ b/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>(system_clock::now().time_since_epoch()) .count(); } diff --git a/wpilibcIntegrationTests/src/CounterTest.cpp b/wpilibcIntegrationTests/src/CounterTest.cpp index 71b94bf89f..4d5e69f861 100644 --- a/wpilibcIntegrationTests/src/CounterTest.cpp +++ b/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."; } diff --git a/wpilibcIntegrationTests/src/FilterNoiseTest.cpp b/wpilibcIntegrationTests/src/FilterNoiseTest.cpp index f99c54405b..30e5a6dae1 100644 --- a/wpilibcIntegrationTests/src/FilterNoiseTest.cpp +++ b/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; /** diff --git a/wpilibcIntegrationTests/src/MotorInvertingTest.cpp b/wpilibcIntegrationTests/src/MotorInvertingTest.cpp index c7015c850f..ea81af2001 100644 --- a/wpilibcIntegrationTests/src/MotorInvertingTest.cpp +++ b/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)); diff --git a/wpilibcIntegrationTests/src/PIDToleranceTest.cpp b/wpilibcIntegrationTests/src/PIDToleranceTest.cpp index cf3849b790..bc1efc8b0b 100644 --- a/wpilibcIntegrationTests/src/PIDToleranceTest.cpp +++ b/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(); diff --git a/wpilibcIntegrationTests/src/VisionTest.cpp b/wpilibcIntegrationTests/src/VisionTest.cpp index d9ba821aaf..7faaefa79a 100644 --- a/wpilibcIntegrationTests/src/VisionTest.cpp +++ b/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() {} }; diff --git a/wpilibj/src/athena/cpp/lib/HALUtil.cpp b/wpilibj/src/athena/cpp/lib/HALUtil.cpp index 6aac1c797c..a338b9433d 100644 --- a/wpilibj/src/athena/cpp/lib/HALUtil.cpp +++ b/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; diff --git a/wpilibj/src/athena/cpp/lib/HALUtil.h b/wpilibj/src/athena/cpp/lib/HALUtil.h index d3c9439a7e..baad654b43 100644 --- a/wpilibj/src/athena/cpp/lib/HALUtil.h +++ b/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; } diff --git a/wpilibj/src/athena/cpp/lib/SolenoidJNI.cpp b/wpilibj/src/athena/cpp/lib/SolenoidJNI.cpp index 7c8fb41d95..0f74ca09c4 100644 --- a/wpilibj/src/athena/cpp/lib/SolenoidJNI.cpp +++ b/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; } diff --git a/wpilibj/src/athena/cpp/lib/ThreadsJNI.cpp b/wpilibj/src/athena/cpp/lib/ThreadsJNI.cpp index 818f7228c7..7121b8d80a 100644 --- a/wpilibj/src/athena/cpp/lib/ThreadsJNI.cpp +++ b/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; } -} \ No newline at end of file +}