Ethernet enabled breaks gpio button short/long/double click logic#1038
Ethernet enabled breaks gpio button short/long/double click logic#1038penfold42 wants to merge 3 commits into
Conversation
checking link status takes 12ms -> a killer in the main loop. only do it once a second I also think its ultimately doing it twice. even with this, the main loop runs much slower. button logic is tweaked to use the # of 100uS that passed. it assumed it was called every 100uS
m_pNet->IsRunning(); also calls IsLinkUP()
Reviewer's GuideIntroduces timer-based throttling for network updates and makes button click/long/double-press detection interval-aware so UI timing remains correct when Ethernet/network activity is present; also cleans up network readiness logic and a minor USB initialization style issue. Sequence diagram for throttled network updates in CMiniDexed::ProcesssequenceDiagram
participant CMiniDexed
participant CTimer
participant Net
CMiniDexed->>CTimer: GetClockTicks()
CTimer-->>CMiniDexed: currentTick
CMiniDexed->>CMiniDexed: [currentTick - m_lastNetworkUpdate > NETWORK_UPDATE_NUM_TICKS]
alt [currentTick - m_lastNetworkUpdate > NETWORK_UPDATE_NUM_TICKS]
CMiniDexed->>CMiniDexed: m_lastNetworkUpdate = currentTick
CMiniDexed->>Net: UpdateNetwork()
else [interval not reached]
CMiniDexed->>CMiniDexed: skip UpdateNetwork
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new tick-based throttling in
Processand the interval logic inCUIButtons::Updateboth rely on unsignedGetClockTicks()subtraction without any wraparound handling; consider explicitly handling timer overflow so long-running devices don’t produce negative/huge effective intervals. - In
CUIButtons::Update,m_intervalcan become 0 whencurrentTick - m_lastTick < BUTTONS_UPDATE_NUM_TICKS, which preventsm_timerfrom progressing for short periods; you may want to enforce a minimum interval of 1 or avoid the division so button timing remains robust at higher update rates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new tick-based throttling in `Process` and the interval logic in `CUIButtons::Update` both rely on unsigned `GetClockTicks()` subtraction without any wraparound handling; consider explicitly handling timer overflow so long-running devices don’t produce negative/huge effective intervals.
- In `CUIButtons::Update`, `m_interval` can become 0 when `currentTick - m_lastTick < BUTTONS_UPDATE_NUM_TICKS`, which prevents `m_timer` from progressing for short periods; you may want to enforce a minimum interval of 1 or avoid the division so button timing remains robust at higher update rates.
## Individual Comments
### Comment 1
<location path="src/minidexed.cpp" line_range="2275-2276" />
<code_context>
- bNetIsRunning &= m_pNetDevice->IsLinkUp();
- else if (m_pNetDevice->GetType() == NetDeviceTypeWLAN)
+
+ if (m_pNetDevice->GetType() == NetDeviceTypeWLAN)
bNetIsRunning &= (m_WPASupplicant && m_WPASupplicant->IsConnected());
if (!m_bNetworkInit && bNetIsRunning)
</code_context>
<issue_to_address>
**question:** Re-check whether Ethernet link state should still gate bNetIsRunning.
This change removes the Ethernet `IsLinkUp()` check from `bNetIsRunning`, so an unplugged Ethernet cable may now be seen as a running network, unlike WLAN which still requires a supplicant connection. If this behavior is intentional, consider whether any minimal link-state check is still needed (e.g., platform-specific) to avoid cases where the stack is up but the physical link is down.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Build for testing: |
As the code stands its unnecessary and I suspect the compiler optimises it away. It can't hurt and might save someone in the future.
|
Build for testing: |
|
My testing with the generated builds with Pi2 and Pi3 is complete. |
|
Thank you very much @penfold42. Do you have a chance to test on more models? |
I also tested on an old Pi1 (with my build to enable ethernet). I dont have a pi4 or pi5. |
|
Would be great if someone could test 4/5. Anyone? |
I've tested the Pi3 |
Fixes #1012
Partially tested with my build on Pi2, Pi3 (and Pi1 with network enabled).
Will do more testing with generated build.
Summary by Sourcery
Throttle network updates and make button handling time-based on actual elapsed ticks to fix input timing issues when Ethernet is enabled.
Bug Fixes:
Enhancements: