Fix ffm-plugin: 5th+ digital inputs receive no state change events#623
Fix ffm-plugin: 5th+ digital inputs receive no state change events#623
Conversation
|
@copilot if possible extend ffm-plugin unit tests to proof this error existed in the current version and is fixed after your changes. |
Primary fix: Changed EventWatcher from virtual threads to platform daemon threads to avoid carrier thread pinning when blocking native poll() is called. Virtual threads pinned to the ForkJoinPool carrier threads (one per CPU core) prevented 5th+ pins from running on Raspberry Pi 4 (4 cores). Secondary fix: Corrected LineConfigAttribute debounce mask from `1L << bcm` to `1L` (index 0 in the offsets array), per kernel GPIO documentation. This fixes debounce being silently ignored on all BCM pins > 0. Added regression test: testMoreThanFourInputsReceiveEvents verifies that 5+ digital inputs all receive state change events. Co-authored-by: FDelporte <1415873+FDelporte@users.noreply.github.com> Agent-Logs-Url: https://github.com/Pi4J/pi4j/sessions/97e73dd8-a439-48fa-a3c1-c5758cb50f8c
…e the carrier-thread pinning bug The mock poll() now blocks each watcher thread (using a CountDownLatch barrier) until all numPins=5 watchers are simultaneously blocked, directly mimicking concurrent blocking native poll() syscalls. With the old virtual-thread code on a <=4-core machine the ForkJoinPool carrier pool is exhausted and the 5th watcher never enters poll(), causing allWatchersBlockingLatch.await() to time out — proving the bug existed. With the fixed platform-daemon-thread code every watcher has its own OS thread, all 5 block concurrently, and the test completes successfully. Co-authored-by: FDelporte <1415873+FDelporte@users.noreply.github.com> Agent-Logs-Url: https://github.com/Pi4J/pi4j/sessions/d2e6d6d7-e3eb-4563-aec4-1ca7da3e2a40
Done in commit With the old virtual-thread code on a ≤4-core machine the ForkJoinPool carrier pool is exhausted by the first 4 blocking watchers; the 5th watcher is never scheduled and Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| logger.trace("{}-{} - Adding new listener", deviceName, bcm); | ||
| if (threadFactory == null) { | ||
| this.threadFactory = Thread.ofVirtual().name(deviceName + "-event-detection-pin-", bcm) | ||
| this.threadFactory = Thread.ofPlatform().name(deviceName + "-event-detection-pin-", bcm) |
There was a problem hiding this comment.
@copilot can you explain this in more detail please? I thought the goal of virtual threads was exactly to step away from the platform thread limit? So how is this a problem that four virtual threads block the four threads on a four-core processor like the raspberry pi?
There was a problem hiding this comment.
Virtual threads are designed to avoid the OS-thread-per-task limit by unmounting from their carrier thread whenever they hit a standard Java blocking call (Object.wait(), LockSupport.park(), ReentrantLock, etc.) — the carrier thread is released and can run another virtual thread. That's the happy path.
There is one critical exception: a virtual thread that calls a native method (JNI or Foreign Function) while blocking cannot be unmounted. It stays pinned to its carrier thread for the entire duration of the native call. This is documented in the JEP-444 notes as "pinning".
Native.poll() here is a foreign-function call that ultimately issues the Linux poll(2) syscall. While the kernel is sitting inside poll() waiting for an edge event, the virtual thread cannot yield — it pins a carrier thread.
The ForkJoinPool that backs virtual threads defaults its carrier-thread count to Runtime.getRuntime().availableProcessors() — 4 on a Raspberry Pi 4. Once 4 EventWatcher loops are each blocked inside the native poll() call, all 4 carrier threads are pinned. Any 5th (or later) EventWatcher virtual thread that needs to start its loop can never be scheduled: there are no free carrier threads and it cannot steal one because none will be released until a GPIO edge event arrives.
Platform daemon threads each own a real OS thread. Blocking in a native call is normal for an OS thread — it only blocks that one thread, and the OS can schedule all others independently. That removes the 4-input ceiling entirely.
There was a problem hiding this comment.
Oh wow, yeah, didn't realize we were using virtual threads. Makes absolute sense as i read about the pinning before.
There was a problem hiding this comment.
Great to know, I did not know the pinning was related to native calls. I guess that would fix the issue.
eitch
left a comment
eitch
left a comment
There was a problem hiding this comment.
With the exception of the debounce, i agree with the fix.
| throw new InitializeException("Debounce value of " + debounce + " is too large"); | ||
| } | ||
| var debounceAttribute = new LineAttribute(LineAttributeId.GPIO_V2_LINE_ATTR_ID_DEBOUNCE.getValue(), 0, 0, (int) debounce * 1000); | ||
| attributes.add(new LineConfigAttribute(debounceAttribute, 1L << bcm)); |
There was a problem hiding this comment.
@IAmNickNack do you agree with copilot here? It says the following:
Secondary fix — debounce mask (FFMDigitalInput.initialize): LineConfigAttribute.mask is an index into offsets[], not a GPIO number. Each FFMDigitalInput owns a single-line request at index 0, so the mask must be 1L, not 1L << bcm. The previous code silently disabled hardware debounce for every BCM pin > 0.
// Before — wrong: uses BCM number as bit position
attributes.add(new LineConfigAttribute(debounceAttribute, 1L << bcm));// After — correct: bit 0 = index 0 in offsets[]
attributes.add(new LineConfigAttribute(debounceAttribute, 1L));
|
Extra smoke test shows the solution works! Will merge after some more tests. |
# Conflicts: # pi4j-test/src/main/java/com/pi4j/test/Main.java # pi4j-test/src/main/java/com/pi4j/test/smoketest/FiveDigitalInputsTestCase.java
|
Bloody awesome! |
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency limitation in the FFM GPIO digital input provider where event watcher threads could become unschedulable (observed as missing state-change events on the 5th+ input on 4-core devices), and corrects hardware debounce configuration targeting.
Changes:
- Switch
FFMDigitalInputevent-watcher execution from virtual threads to platform daemon threads to avoid carrier-thread pinning during blocking nativepoll()calls. - Fix debounce attribute mask application to target the requested line index (bit 0) rather than shifting by BCM number.
- Add a unit regression test that simulates concurrent blocking
poll()behavior across 5 digital inputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/pi4j-plugin-ffm/src/main/java/com/pi4j/plugin/ffm/providers/gpio/FFMDigitalInput.java | Updates debounce mask usage and changes the event-watcher thread/executor model. |
| plugins/pi4j-plugin-ffm/src/test/java/com/pi4j/plugin/ffm/unit/GPIOTest.java | Adds a regression test covering 5+ simultaneous digital input event watchers. |
| pi4j-test/src/main/java/com/pi4j/test/smoketest/FiveDigitalInputsTestCase.java | Removes a stray leading line artifact. |
Comments suppressed due to low confidence (2)
plugins/pi4j-plugin-ffm/src/main/java/com/pi4j/plugin/ffm/providers/gpio/FFMDigitalInput.java:145
- The
uncaughtExceptionHandler(...)configured on theThreadFactorywill not see exceptions thrown byEventWatcherbecause the task is submitted viaExecutorService.submit(...), which captures exceptions in the returnedFuture. If you want these failures logged, useexecute(...)forRunnabletasks or store/inspect theFutureand logExecutionException(or wrap the runnable to catch/log).
this.threadFactory = Thread.ofPlatform().name(deviceName + "-event-detection-pin-", bcm)
.daemon(true)
.uncaughtExceptionHandler(((_, e) -> logger.error(e.getMessage(), e)))
.factory();
this.eventTaskProcessor = Executors.newCachedThreadPool(threadFactory);
}
var watcher = new EventWatcher(chipFileDescriptor, PinEvent.BOTH, events -> {
for (DetectedEvent detectedEvent : events) {
var state = switch (detectedEvent.pinEvent()) {
case RISING -> DigitalState.HIGH;
case FALLING -> DigitalState.LOW;
default -> DigitalState.UNKNOWN;
};
this.dispatch(new DigitalStateChangeEvent<DigitalInput>(this, state));
}
});
watchers.add(watcher);
eventTaskProcessor.submit(watcher);
logger.trace("{}-{} - New listener added", deviceName, bcm);
plugins/pi4j-plugin-ffm/src/main/java/com/pi4j/plugin/ffm/providers/gpio/FFMDigitalInput.java:146
addListener(...)currently creates and submits a newEventWatcherevery time it is called. Multiple watcher threads polling/reading from the same line-event FD will race to consume events and can cause missed/duplicated events (and unnecessary extra threads). Consider starting exactly one watcher perFFMDigitalInput(e.g., only when the first listener/binding is added) and have it dispatch to the shared listener manager.
public DigitalInput addListener(DigitalStateChangeListener... listener) {
logger.trace("{}-{} - Adding new listener", deviceName, bcm);
if (threadFactory == null) {
this.threadFactory = Thread.ofPlatform().name(deviceName + "-event-detection-pin-", bcm)
.daemon(true)
.uncaughtExceptionHandler(((_, e) -> logger.error(e.getMessage(), e)))
.factory();
this.eventTaskProcessor = Executors.newCachedThreadPool(threadFactory);
}
var watcher = new EventWatcher(chipFileDescriptor, PinEvent.BOTH, events -> {
for (DetectedEvent detectedEvent : events) {
var state = switch (detectedEvent.pinEvent()) {
case RISING -> DigitalState.HIGH;
case FALLING -> DigitalState.LOW;
default -> DigitalState.UNKNOWN;
};
this.dispatch(new DigitalStateChangeEvent<DigitalInput>(this, state));
}
});
watchers.add(watcher);
eventTaskProcessor.submit(watcher);
logger.trace("{}-{} - New listener added", deviceName, bcm);
return super.addListener(listener);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Regression test for the carrier-thread-pinning bug: | ||
| // | ||
| // Before the fix, EventWatcher used virtual threads. Virtual threads that call a | ||
| // blocking native method (poll()) are *pinned* to a ForkJoinPool carrier thread | ||
| // for the duration of the call. The carrier pool size defaults to the number of | ||
| // available CPU cores (4 on a Raspberry Pi 4). Once 4 pins are listening, all | ||
| // carrier threads are pinned and the 5th+ EventWatcher can never be scheduled. | ||
| // | ||
| // This test makes each watcher's first poll() call block until ALL numPins | ||
| // watchers are simultaneously blocked, then releases them. That directly mirrors | ||
| // the real scenario: concurrent blocking native poll() calls. | ||
| // | ||
| // With the old virtual-thread code on a <=4-core machine the test would time out | ||
| // at allWatchersBlockingLatch because the 5th watcher can never enter poll(). | ||
| // With the fixed platform-daemon-thread code every watcher runs on its own OS | ||
| // thread, all 5 block simultaneously, and the test completes. |
There was a problem hiding this comment.
This regression test only fails with the old virtual-thread implementation on machines where the virtual-thread scheduler parallelism is <= 4 (e.g., a 4-core Raspberry Pi). On CI/dev machines with >4 cores, the old buggy code may still pass, so the test may not reliably prevent regressions. Consider forcing the processor count/scheduler parallelism for the test JVM (e.g., Surefire -XX:ActiveProcessorCount=4 or jdk.virtualThreadScheduler.parallelism=4 set before any virtual threads are created), or explicitly skip/assert based on availableProcessors() so the test outcome is deterministic.
| // check conversion from ms to ns | ||
| if (debounce * 1000 > Integer.MAX_VALUE) { | ||
| throw new InitializeException("Debounce value of " + debounce + " is too large"); | ||
| } | ||
| var debounceAttribute = new LineAttribute(LineAttributeId.GPIO_V2_LINE_ATTR_ID_DEBOUNCE.getValue(), 0, 0, (int) debounce * 1000); | ||
| attributes.add(new LineConfigAttribute(debounceAttribute, 1L << bcm)); | ||
| attributes.add(new LineConfigAttribute(debounceAttribute, 1L)); | ||
| } |
There was a problem hiding this comment.
DigitalInputConfigBuilder.debounce(...) stores debounce in microseconds, and LineAttribute expects debounce_period_us, but this code multiplies debounce by 1000 before passing it to the kernel attribute. That makes the configured hardware debounce 1000× larger than the Pi4J config value and also makes the overflow check misleading. Align the units (either pass debounce directly as microseconds, or clearly convert from milliseconds at the config boundary and update naming/docs accordingly).
There was a problem hiding this comment.
@IAmNickNack @FDelporte @stefanhaustein perhaps here is our issue with debounce?
Virtual threads in
EventWatcherpin to ForkJoinPool carrier threads when making blocking nativepoll()calls. On a Raspberry Pi 4 (4 cores = 4 carrier threads), once 4 pins are registered, subsequent pins can never be scheduled.Changes
FFMDigitalInput.addListener): ReplaceThread.ofVirtual()+Executors.newThreadPerTaskExecutor()withThread.ofPlatform().daemon(true)+Executors.newCachedThreadPool(). Platform threads have dedicated OS threads and cannot be pinned, removing the 4-input ceiling.FFMDigitalInput.initialize):LineConfigAttribute.maskis an index intooffsets[], not a GPIO number. EachFFMDigitalInputowns a single-line request at index 0, so the mask must be1L, not1L << bcm. The previous code silently disabled hardware debounce for every BCM pin > 0.testMoreThanFourInputsReceiveEvents): The test uses aCountDownLatchbarrier to make each watcher's firstpoll()call block until all 5 watchers are simultaneously blocked — directly mimicking concurrent blocking nativepoll()syscalls. With the old virtual-thread code on a ≤4-core machine the ForkJoinPool carrier pool is exhausted by the first 4 blocking watchers; the 5th watcher is never scheduled and the test times out, proving the bug existed. With the fixed platform-daemon-thread code all 5 run on dedicated OS threads, block concurrently, and the test passes.Original prompt
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.