[hal] Fix interrupt edges being flipped in sim (#6720)

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);
   }
 }
 

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);
 
@@ -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(10_ms);
+  digitalSim.SetValue(true);
+  frc::Wait(10_ms);
+
+  int count = 0;
+  while (!hasFiredRisingEdge) {
+    frc::Wait(5_ms);
+    count++;
+    ASSERT_TRUE(count < 1000);
+  }
+  EXPECT_FALSE(hasFiredFallingEdge);
+  EXPECT_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(10_ms);
+  digitalSim.SetValue(false);
+  frc::Wait(10_ms);
+
+  int count = 0;
+  while (!hasFiredFallingEdge) {
+    frc::Wait(5_ms);
+    count++;
+    ASSERT_TRUE(count < 1000);
+  }
+  EXPECT_TRUE(hasFiredFallingEdge);
+  EXPECT_FALSE(hasFiredRisingEdge);
+}
 }  // namespace frc

wpilibj/src/test/java/edu/wpi/first/wpilibj/InterruptTest.java

@@ -4,7 +4,9 @@
 
 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;
 
 import edu.wpi.first.wpilibj.simulation.DIOSim;
@@ -22,7 +24,7 @@ class InterruptTest {
         AsynchronousInterrupt interrupt =
             new AsynchronousInterrupt(
                 di,
-                (a, b) -> {
+                (rising, falling) -> {
                   counter.incrementAndGet();
                   hasFired.set(true);
                 })) {
@@ -43,4 +45,78 @@ class InterruptTest {
       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,
+                (rising, falling) -> {
+                  hasFiredFallingEdge.set(falling);
+                  hasFiredRisingEdge.set(rising);
+                })) {
+      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);
+      }
+      assertAll(
+          () ->
+              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,
+                (rising, falling) -> {
+                  hasFiredFallingEdge.set(falling);
+                  hasFiredRisingEdge.set(rising);
+                })) {
+      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);
+      }
+      assertAll(
+          () ->
+              assertTrue(
+                  hasFiredFallingEdge.get(), "The interrupt did not trigger on the rising edge"),
+          () ->
+              assertFalse(hasFiredRisingEdge.get(), "The interrupt triggered on the rising edge"));
+    }
+  }
 }