Make the interrupt poll thread persistent: park on detach instead of exiting#11
Draft
Nursedude wants to merge 2 commits into
Draft
Make the interrupt poll thread persistent: park on detach instead of exiting#11Nursedude wants to merge 2 commits into
Nursedude wants to merge 2 commits into
Conversation
…exiting Deeper fix for the thread-stack leak class (see PR pine64#10 for the minimal one-liner). Creating and destroying a pthread per attach/detach cycle is churn no embedded target should pay — on 32-bit router-class platforms (OpenWrt mips/arm) the strand variant exhausts the address space in hours, and even leak-free churn costs a stack mmap + clone per radio interrupt. The poll thread is now created once per pinedio_init() and PARKS on a condition variable when the last interrupt detaches, resuming on the next attach. There is never a mid-life thread to join, so detaching from within the interrupt callback (which runs on this very thread) is inherently safe — the self-join hazard af9bc27 guarded against is gone by construction. The thread terminates exactly once, in pinedio_deinit() (join, or detach-self in the pathological deinit-from-callback case). Behavior change, deliberate: a pinedio_get_input() failure now latches in_error and keeps polling at the normal cadence (self-healing on a later good read) instead of silently killing the poll thread until the next full deinit/init.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deeper follow-up to #10 (the minimal one-line leak fix) — this eliminates the leak class rather than the instance. Maintainers' choice: merge #10 alone, or this instead (it supersedes #10); both fix meshtastic/firmware#10468.
Why go further than #10
Creating and destroying a pthread per attach/detach cycle is churn embedded targets shouldn't pay: consumers that detach/re-attach around every radio interrupt (meshtastic firmware does) spawn ~9–12 threads per minute — a fresh 8 MB stack mmap + clone3 each time even once #10 makes them reclaimable. On 32-bit router-class platforms (OpenWrt mips/arm, ~2 GB usable VA) the stranded variant exhausts address space in hours, and
vm.max_map_count(65530 default) is its own ceiling.Design
pinedio_init()(on first attach) and parks on a condition variable when the last interrupt detaches; the next attach signals it awake. Attach/detach cycles toggle a flag — zero thread churn.pinedio_deinit()(join;pthread_detach(self)in the pathological deinit-from-callback case).usb_access_mutex→poll_park_mutex); the poll thread never holds the park mutex while taking the USB mutex.Deliberate behavior change: a
pinedio_get_input()failure now latchesin_errorand keeps polling at the normal cadence — self-healing when a later read succeeds — instead of silently killing the poll thread until the next full deinit/init (the old failure mode left interrupt delivery dead with no recovery path). If you prefer the old semantics I can park on error instead.Testing status
Compile-tested against the full meshtastic firmware native build (2.7.24/472b14c, aarch64). Not yet soak-tested on hardware — my three CH341 field units are currently validating #10 (numbers posted there: leak eliminated). I'll run this branch through the same live-hardware soak on request, or once there's maintainer appetite for the deeper change.