From a58cdc73ac4560545362973ce389ff355bf490dc Mon Sep 17 00:00:00 2001 From: Gold856 <117957790+Gold856@users.noreply.github.com> Date: Mon, 27 May 2024 16:26:17 -0400 Subject: [PATCH 1/2] Fix interrupt edges being flipped in sim --- hal/src/main/native/sim/Interrupts.cpp | 40 ++++++----- wpilibc/src/test/native/cpp/InterruptTest.cpp | 60 +++++++++++++++++ .../edu/wpi/first/wpilibj/InterruptTest.java | 67 +++++++++++++++++++ 3 files changed, 149 insertions(+), 18 deletions(-) diff --git a/hal/src/main/native/sim/Interrupts.cpp b/hal/src/main/native/sim/Interrupts.cpp index ac9f7f8ebb6..1537c73640a 100644 --- a/hal/src/main/native/sim/Interrupts.cpp +++ b/hal/src/main/native/sim/Interrupts.cpp @@ -44,7 +44,7 @@ struct Interrupt { HAL_AnalogTriggerType trigType; int64_t risingTimestamp; int64_t fallingTimestamp; - bool previousState; + bool currentState; bool fireOnUp; bool fireOnDown; int32_t callbackId; @@ -121,8 +121,8 @@ static void ProcessInterruptDigitalSynchronous(const char* name, void* param, return; } bool retVal = value->data.v_boolean; - auto previousState = interrupt->previousState; - interrupt->previousState = retVal; + auto previousState = interrupt->currentState; + interrupt->currentState = retVal; // If no change in interrupt, return; if (retVal == previousState) { return; @@ -176,8 +176,8 @@ static void ProcessInterruptAnalogSynchronous(const char* name, void* param, // Pulse interrupt interruptData->waitCond.notify_all(); } - auto previousState = interrupt->previousState; - interrupt->previousState = retVal; + auto previousState = interrupt->currentState; + interrupt->currentState = retVal; // If no change in interrupt, return; if (retVal == previousState) { return; @@ -220,7 +220,7 @@ static int64_t WaitForInterruptDigital(HAL_InterruptHandle handle, return WaitResult::Timeout; } - interrupt->previousState = SimDIOData[digitalIndex].value; + interrupt->currentState = SimDIOData[digitalIndex].value; int32_t uid = SimDIOData[digitalIndex].value.RegisterCallback( &ProcessInterruptDigitalSynchronous, @@ -252,14 +252,16 @@ static int64_t WaitForInterruptDigital(HAL_InterruptHandle handle, if (timedOut) { return WaitResult::Timeout; } - // True => false, Falling - if (interrupt->previousState) { + // We know the value has changed because we would've timed out otherwise. + // If the current state is true, the previous state was false, so this is a + // rising edge. Otherwise, it's a falling edge. + if (interrupt->currentState) { // Set our return value and our timestamps - interrupt->fallingTimestamp = hal::GetFPGATime(); - return 1 << (8 + interrupt->index); - } else { interrupt->risingTimestamp = hal::GetFPGATime(); return 1 << (interrupt->index); + } else { + interrupt->fallingTimestamp = hal::GetFPGATime(); + return 1 << (8 + interrupt->index); } } @@ -278,8 +280,8 @@ static int64_t WaitForInterruptAnalog(HAL_InterruptHandle handle, data->interruptHandle = handle; int32_t status = 0; - interrupt->previousState = GetAnalogTriggerValue( - interrupt->portHandle, interrupt->trigType, &status); + interrupt->currentState = GetAnalogTriggerValue(interrupt->portHandle, + interrupt->trigType, &status); if (status != 0) { return WaitResult::Timeout; @@ -322,14 +324,16 @@ static int64_t WaitForInterruptAnalog(HAL_InterruptHandle handle, if (timedOut) { return WaitResult::Timeout; } - // True => false, Falling - if (interrupt->previousState) { + // We know the value has changed because we would've timed out otherwise. + // If the current state is true, the previous state was false, so this is a + // rising edge. Otherwise, it's a falling edge. + if (interrupt->currentState) { // Set our return value and our timestamps - interrupt->fallingTimestamp = hal::GetFPGATime(); - return 1 << (8 + interrupt->index); - } else { interrupt->risingTimestamp = hal::GetFPGATime(); return 1 << (interrupt->index); + } else { + interrupt->fallingTimestamp = hal::GetFPGATime(); + return 1 << (8 + interrupt->index); } } diff --git a/wpilibc/src/test/native/cpp/InterruptTest.cpp b/wpilibc/src/test/native/cpp/InterruptTest.cpp index 96af18089fe..85fb2fe9fda 100644 --- a/wpilibc/src/test/native/cpp/InterruptTest.cpp +++ b/wpilibc/src/test/native/cpp/InterruptTest.cpp @@ -42,4 +42,64 @@ TEST(InterruptTest, AsynchronousInterrupt) { } ASSERT_EQ(1, counter.load()); } + +TEST(InterruptTest, RisingEdge) { + HAL_Initialize(500, 0); + + std::atomic_bool hasFiredFallingEdge{false}; + std::atomic_bool hasFiredRisingEdge{false}; + + DigitalInput di{0}; + AsynchronousInterrupt interrupt{di, [&](bool rising, bool falling) { + hasFiredFallingEdge = falling; + hasFiredRisingEdge = rising; + }}; + interrupt.SetInterruptEdges(true, true); + DIOSim digitalSim{di}; + digitalSim.SetValue(false); + frc::Wait(0.5_s); + interrupt.Enable(); + frc::Wait(20_ms); + digitalSim.SetValue(true); + frc::Wait(10_ms); + + int count = 0; + while (!hasFiredRisingEdge) { + frc::Wait(5_ms); + count++; + ASSERT_TRUE(count < 1000); + } + ASSERT_FALSE(hasFiredFallingEdge); + ASSERT_TRUE(hasFiredRisingEdge); +} + +TEST(InterruptTest, FallingEdge) { + HAL_Initialize(500, 0); + + std::atomic_bool hasFiredFallingEdge{false}; + std::atomic_bool hasFiredRisingEdge{false}; + + DigitalInput di{0}; + AsynchronousInterrupt interrupt{di, [&](bool rising, bool falling) { + hasFiredFallingEdge = falling; + hasFiredRisingEdge = rising; + }}; + interrupt.SetInterruptEdges(true, true); + DIOSim digitalSim{di}; + digitalSim.SetValue(true); + frc::Wait(0.5_s); + interrupt.Enable(); + frc::Wait(20_ms); + digitalSim.SetValue(false); + frc::Wait(10_ms); + + int count = 0; + while (!hasFiredFallingEdge) { + frc::Wait(5_ms); + count++; + ASSERT_TRUE(count < 1000); + } + ASSERT_TRUE(hasFiredFallingEdge); + ASSERT_FALSE(hasFiredRisingEdge); +} } // namespace frc diff --git a/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java b/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java index bc32b8e3464..320beadadd2 100644 --- a/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java +++ b/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java @@ -5,6 +5,7 @@ package edu.wpi.first.wpilibj; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import edu.wpi.first.wpilibj.simulation.DIOSim; @@ -43,4 +44,70 @@ void testAsynchronousInterrupt() { assertEquals(1, counter.get(), "The interrupt did not fire the expected number of times"); } } + + @Test + void testRisingEdge() { + AtomicBoolean hasFiredFallingEdge = new AtomicBoolean(false); + AtomicBoolean hasFiredRisingEdge = new AtomicBoolean(false); + + try (DigitalInput di = new DigitalInput(0); + AsynchronousInterrupt interrupt = + new AsynchronousInterrupt( + di, + (a, b) -> { + hasFiredFallingEdge.set(b); + hasFiredRisingEdge.set(a); + })) { + interrupt.setInterruptEdges(true, true); + DIOSim digitalSim = new DIOSim(di); + digitalSim.setValue(false); + Timer.delay(0.5); + interrupt.enable(); + Timer.delay(0.01); + digitalSim.setValue(true); + Timer.delay(0.01); + + int count = 0; + while (!hasFiredRisingEdge.get()) { + Timer.delay(0.005); + count++; + assertTrue(count < 1000); + } + assertFalse(hasFiredFallingEdge.get(), "The interrupt triggered on the falling edge"); + assertTrue(hasFiredRisingEdge.get(), "The interrupt did not trigger on the rising edge"); + } + } + + @Test + void testFallingEdge() { + AtomicBoolean hasFiredFallingEdge = new AtomicBoolean(false); + AtomicBoolean hasFiredRisingEdge = new AtomicBoolean(false); + + try (DigitalInput di = new DigitalInput(0); + AsynchronousInterrupt interrupt = + new AsynchronousInterrupt( + di, + (a, b) -> { + hasFiredFallingEdge.set(b); + hasFiredRisingEdge.set(a); + })) { + interrupt.setInterruptEdges(true, true); + DIOSim digitalSim = new DIOSim(di); + digitalSim.setValue(true); + Timer.delay(0.5); + interrupt.enable(); + Timer.delay(0.01); + digitalSim.setValue(false); + Timer.delay(0.01); + + int count = 0; + while (!hasFiredFallingEdge.get()) { + Timer.delay(0.005); + count++; + assertTrue(count < 1000); + } + assertTrue(hasFiredFallingEdge.get(), "The interrupt did not trigger on the rising edge"); + assertFalse(hasFiredRisingEdge.get(), "The interrupt triggered on the rising edge"); + } + } } From f7d0601b2e21f5932bf33e2e440e0a7d155d78bf Mon Sep 17 00:00:00 2001 From: Gold856 <117957790+Gold856@users.noreply.github.com> Date: Sun, 9 Jun 2024 15:39:17 -0400 Subject: [PATCH 2/2] Make tests consistent and make assertions non-fatal --- wpilibc/src/test/native/cpp/InterruptTest.cpp | 14 ++++----- .../edu/wpi/first/wpilibj/InterruptTest.java | 31 ++++++++++++------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/wpilibc/src/test/native/cpp/InterruptTest.cpp b/wpilibc/src/test/native/cpp/InterruptTest.cpp index 85fb2fe9fda..b21484c2c67 100644 --- a/wpilibc/src/test/native/cpp/InterruptTest.cpp +++ b/wpilibc/src/test/native/cpp/InterruptTest.cpp @@ -30,7 +30,7 @@ TEST(InterruptTest, AsynchronousInterrupt) { frc::Wait(0.5_s); DIOSim digitalSim{di}; digitalSim.SetValue(false); - frc::Wait(20_ms); + frc::Wait(10_ms); digitalSim.SetValue(true); frc::Wait(10_ms); @@ -59,7 +59,7 @@ TEST(InterruptTest, RisingEdge) { digitalSim.SetValue(false); frc::Wait(0.5_s); interrupt.Enable(); - frc::Wait(20_ms); + frc::Wait(10_ms); digitalSim.SetValue(true); frc::Wait(10_ms); @@ -69,8 +69,8 @@ TEST(InterruptTest, RisingEdge) { count++; ASSERT_TRUE(count < 1000); } - ASSERT_FALSE(hasFiredFallingEdge); - ASSERT_TRUE(hasFiredRisingEdge); + EXPECT_FALSE(hasFiredFallingEdge); + EXPECT_TRUE(hasFiredRisingEdge); } TEST(InterruptTest, FallingEdge) { @@ -89,7 +89,7 @@ TEST(InterruptTest, FallingEdge) { digitalSim.SetValue(true); frc::Wait(0.5_s); interrupt.Enable(); - frc::Wait(20_ms); + frc::Wait(10_ms); digitalSim.SetValue(false); frc::Wait(10_ms); @@ -99,7 +99,7 @@ TEST(InterruptTest, FallingEdge) { count++; ASSERT_TRUE(count < 1000); } - ASSERT_TRUE(hasFiredFallingEdge); - ASSERT_FALSE(hasFiredRisingEdge); + EXPECT_TRUE(hasFiredFallingEdge); + EXPECT_FALSE(hasFiredRisingEdge); } } // namespace frc diff --git a/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java b/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java index 320beadadd2..795f6291235 100644 --- a/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java +++ b/wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java @@ -4,6 +4,7 @@ package edu.wpi.first.wpilibj; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -23,7 +24,7 @@ void testAsynchronousInterrupt() { AsynchronousInterrupt interrupt = new AsynchronousInterrupt( di, - (a, b) -> { + (rising, falling) -> { counter.incrementAndGet(); hasFired.set(true); })) { @@ -54,9 +55,9 @@ void testRisingEdge() { AsynchronousInterrupt interrupt = new AsynchronousInterrupt( di, - (a, b) -> { - hasFiredFallingEdge.set(b); - hasFiredRisingEdge.set(a); + (rising, falling) -> { + hasFiredFallingEdge.set(falling); + hasFiredRisingEdge.set(rising); })) { interrupt.setInterruptEdges(true, true); DIOSim digitalSim = new DIOSim(di); @@ -73,8 +74,12 @@ void testRisingEdge() { count++; assertTrue(count < 1000); } - assertFalse(hasFiredFallingEdge.get(), "The interrupt triggered on the falling edge"); - assertTrue(hasFiredRisingEdge.get(), "The interrupt did not trigger on the rising edge"); + assertAll( + () -> + assertFalse(hasFiredFallingEdge.get(), "The interrupt triggered on the falling edge"), + () -> + assertTrue( + hasFiredRisingEdge.get(), "The interrupt did not trigger on the rising edge")); } } @@ -87,9 +92,9 @@ void testFallingEdge() { AsynchronousInterrupt interrupt = new AsynchronousInterrupt( di, - (a, b) -> { - hasFiredFallingEdge.set(b); - hasFiredRisingEdge.set(a); + (rising, falling) -> { + hasFiredFallingEdge.set(falling); + hasFiredRisingEdge.set(rising); })) { interrupt.setInterruptEdges(true, true); DIOSim digitalSim = new DIOSim(di); @@ -106,8 +111,12 @@ void testFallingEdge() { count++; assertTrue(count < 1000); } - assertTrue(hasFiredFallingEdge.get(), "The interrupt did not trigger on the rising edge"); - assertFalse(hasFiredRisingEdge.get(), "The interrupt triggered on the rising edge"); + assertAll( + () -> + assertTrue( + hasFiredFallingEdge.get(), "The interrupt did not trigger on the rising edge"), + () -> + assertFalse(hasFiredRisingEdge.get(), "The interrupt triggered on the rising edge")); } } }