-
Notifications
You must be signed in to change notification settings - Fork 7
Wifi v4 #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…es cold boot race conditions, reduces connection time to 8-20s and failure detection to 10-20s
…ility, Single Network Mode, and event-driven interface monitoring
…reconnection blocking in SNM
feat(wifi): Reworked single network mode and handling
fix(wireless): Fix hotspot not starting on fresh boot without ethernet
fix(wireless): Rolling back previous commit
fix(wireless): Fix hotspot startup race condition
fix(wireless): Fix hotspot on cold interface
fix(wireless): Add interface readiness check for first boot hotspot
fix(wireless): Add interface readiness check for first boot hotspot
|
Thank you for the thorough review. I appreciate the attention to detail on such a critical component. What was delivered: The v4.0 refactoring addresses the core requirements we set out to solve: SNM compliance for AP2 licensing, deadlock elimination, USB WiFi adapter timing, DHCP lease management, and emergency recovery modes. The code has been tested against the agreed test cases and functions correctly across known to me scenarios. Regarding your review items: Items 1-4 (notifyWirelessReady timing, regdomain behavior, variable declaration order, WiFi enable/disable in dual mode): These are valid observations. However, they require decisions about backend integration and system-wide behavior that the core team is better positioned to make. You have visibility into dependencies I do not. Item 5 (12-hour reconnection): This is wpa_supplicant's responsibility. wireless.js monitors state changes but does not control the underlying reconnection behavior. Item 6 (Hotspot with background reconnection): This is a feature request, not part of the original scope. Style feedback: Noted for any future work. Going forward: I have completed the wireless.js prototype I committed myself to. The code is functional and documented. The atomic commit history provides clear traceability for review. Further development decisions - what to merge, what to modify, what to prioritize - belong with the core team. I lack visibility into backend systems, product roadmap, and integration requirements to make those calls. I remain available to assist with specific, well-defined tasks if needed. But ownership of wireless.js now sits with Volumio. |
|
Pi 3B+ CPU Consumption Report An additional issue has been reported: wireless.js consuming 99% CPU on Pi 3B+, causing system-wide performance degradation including audio xruns and network latency. Evidence:
Likely causes:
My position: The prototype code uses the same callback patterns and timing constants established in the original implementation. If there is a runaway loop, it may be triggered by a specific hardware or network condition not present in my test environment. This is noted for core team investigation. A defensive fix would be to add loop counters and maximum iteration guards to any polling or retry mechanism, but identifying the specific offending code path requires logs from the affected device |
|
I think we are slightly misaligned on expectations, so I want to clarify this clearly. You did a great job pushing hard on this and driving the refactor forward, and that effort is appreciated. That said, in this case the push may have had the opposite effect. With large refactors of core systems like wireless.js, speed alone is not the goal. What we strive for is landing changes that work 100% of the time across hardware and real-world conditions. For small PRs, it is fine to deliver a working change and let maintainers take it from there. For large core refactors, responsibility is shared until the PR is either merged or withdrawn, including working through review feedback and fixing regressions found during testing (and that's why I tried to discourage this approach...). A “thanks for QA, good luck” handoff is not appropriate for a change of this scope, and it ends up wasting time on both sides. Right now we have users affected by performance issues, and this is exactly where your competence can be best used. We can fix this together by aligning on the blocking issues and iterating until it is solid and safe to merge. If you are willing to continue, let us define a short list of fixes and move forward together. If not, we can pause or withdraw the PR and revisit parts of it later under core-team ownership. Let me know how you would like to proceed. |
|
I tried to have a look to the PR code, but unfortuately it's way beyond my skills and knowledge of the system. The work looks solid, and initial feedback on the forum from the people testing it is very positive, this is a potential big step forward for the system. If we want to be pragmatic and speed things up, my proposal is to identify the potential regression and fix them, I think @foonerd is the best person to take care of the fixing part, being this rework one of his creations. Of course we can support with extensive testing and QA. |
This alone unfolds knowledge gap. If overdue wireless overhaul is beyond grasp I have serious concerns about bookworm as such where technological change is continuously underestimated. Perhaps complete move to Bookworm should be canned. |
|
I am not the one blocking progress and nothing has changed. My time is limited. |
|
Thanks. I updated the original description adding the new bug which is high CPU usage. IMHO the priorities are (from urgent to low): 5 High CPU Usage of wireless which creates buffer underruns If there's one where you feel I will be better suited, feel free to tell and we can split the tasks |
|
Startup race condition On boot with ethernet connected, wireless.js starts two parallel flows. The variable The sequence:
Result: Hotspot running when system should be in scan mode. Fix: Added function refreshEthernetState() {
try {
var carrier = fs.readFileSync('/sys/class/net/eth0/carrier', 'utf8').trim();
var newState = (carrier === '1');
if (newState !== isWiredNetworkActive) {
loggerInfo('refreshEthernetState: Corrected ethernet state: ' +
(newState ? 'connected' : 'disconnected'));
isWiredNetworkActive = newState;
}
} catch (e) {
// eth0 doesn't exist or carrier not readable
}
}Called in The async monitor remains for runtime cable plug/unplug events. The sync check handles startup timing only. |
|
Commits to date should handle priorities from 1 to 4 included. As such are ready for review. |
|
Priority 5 - High CPU Usage of wireless which creates buffer underruns @volumio - this is serious blame :44c2573 Issue: Duplicate logging causing excessive sync I/O Current implementation writes every log message twice:
Measured impact: Log statements in code: 315 total
During 30-second connection phase with 1-second polling:
Each sync write:
Estimated I/O time per connection attempt:
With debug enabled (adds ~115 more paths):
Options: Option A: Journal only (remove file logging)
Option B: File only (remove console.log)
Option C: File only when debug=true, journal always
Option D: Journal only, file only when debug=true
Recommendation: Option D Production (debug=false):
Debug (debug=true):
This halves sync I/O in production and provides full logging when investigating issues. |
|
Started testing |
|
@volumio - triage needed as per my previous comment: Priority 5 - High CPU Usage of wireless which creates buffer underruns |
|
@foonerd did quite some extensive testing and all looks nominal now. Re your performance remark about double logging. I agree with you that option D is the best one However I would suggest that we keep like it is for now until we have verified the behavour 100%, as this might get us useful debug informations. We can revist in 1 month from now by implementing option D. FYI, on this regard I change the use of append from sync to async, this alone will improve performance. and considering the logging frequency we should not have problem with effective sequence of logs. As a general rule, given how nodejs works we should limit sync operations as much as possible, as they block nodejs event loop quite substantially and impact severely the performance. I think we can merge now and keep on monitoring the behaviour and keep on improving this excellent code now. Thanks. Merging, you can do a build at your convenience ! |
|
@foonerd I need your approval before merging, or anyone else... |
foonerd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting to my todo list for end of January 2026 review of the logger options - namely selected option D.
No description provided.