Optimize PA01's matrix keyboard scanning#7394
Conversation
📝 WalkthroughWalkthroughThe change adds two early return points to ChangesEarly Exit Conditions in Key Polling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
radio/src/targets/pa01/key_driver.cpp (2)
109-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-existing concurrency guard bug: missing early return.
When
ui8ReadInProgress != 0, another thread is already scanning the matrix. The code setskeyStatebut continues to line 114 and increments the counter, allowing concurrent access tosyncelem.ui8ReadInProgress. According to the cross-target pattern (pl18 in context snippet 3), this check should return immediately.While the second check at line 117 (with the new return at line 119) mitigates this, the first check should also return to fully prevent concurrent access.
🔒 Proposed fix: add early return
if (syncelem.ui8ReadInProgress != 0) { keyState = syncelem.oldResult; + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/pa01/key_driver.cpp` around lines 109 - 111, When syncelem.ui8ReadInProgress != 0 the code currently sets keyState = syncelem.oldResult but continues executing and increments the concurrency counter; change this by making the branch return immediately after setting keyState to syncelem.oldResult to avoid touching syncelem.ui8ReadInProgress (i.e., add an early return in the same if that checks syncelem.ui8ReadInProgress). Locate the check using syncelem.ui8ReadInProgress and keyState in key_driver.cpp and ensure the function exits right after assigning syncelem.oldResult so no further access (like the counter increment) occurs.
86-87:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: OR operation causes sticky key state.
The
|=operator sets the ENT bit when pressed but never clears it when released during early-return cycles. SincekeyStateis static (persists across calls), previously-pressed keys remain set even after release until the next full matrix scan. This causes keys to appear stuck.When taking the early-return path, either properly update the ENT bit (set if pressed, clear if released) or leave
keyStateunmodified and rely on the cached state.🔧 Proposed fix: properly update ENT bit
- if (gpio_read(KEYS_GPIO_ENTER) == 0) - keyState |= 1<<ENT; + if (gpio_read(KEYS_GPIO_ENTER) == 0) + keyState |= 1<<ENT; + else + keyState &= ~(1<<ENT);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/pa01/key_driver.cpp` around lines 86 - 87, The early-return branch currently uses "keyState |= 1<<ENT" which sets the ENT bit but never clears it, causing sticky keys because keyState is static; change that logic so the ENT bit is explicitly updated based on gpio_read(KEYS_GPIO_ENTER): if gpio_read(KEYS_GPIO_ENTER) == 0 then set the bit (keyState |= 1<<ENT) else clear the bit (keyState &= ~(1<<ENT)); alternatively, if the early-return should not touch the cached value, leave keyState unchanged—locate the use of keyState, KEYS_GPIO_ENTER, and ENT and replace the single |= update with the conditional set/clear using the 1<<ENT mask.
🧹 Nitpick comments (1)
radio/src/targets/pa01/key_driver.cpp (1)
90-90: 💤 Low valueFix comment spacing.
Missing space after comma.
✏️ Proposed fix
- //If no interrupt occurs, it means no key is pressed, so no scanning is needed this time,exit. + //If no interrupt occurs, it means no key is pressed, so no scanning is needed this time, exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/pa01/key_driver.cpp` at line 90, Update the inline comment that currently reads "//If no interrupt occurs, it means no key is pressed, so no scanning is needed this time,exit." to fix spacing after the comma by adding a space before "exit."—i.e., change ",exit." to ", exit."; locate this comment in key_driver.cpp (the comment text itself) and apply the single-character spacing fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@radio/src/targets/pa01/key_driver.cpp`:
- Around line 109-111: When syncelem.ui8ReadInProgress != 0 the code currently
sets keyState = syncelem.oldResult but continues executing and increments the
concurrency counter; change this by making the branch return immediately after
setting keyState to syncelem.oldResult to avoid touching
syncelem.ui8ReadInProgress (i.e., add an early return in the same if that checks
syncelem.ui8ReadInProgress). Locate the check using syncelem.ui8ReadInProgress
and keyState in key_driver.cpp and ensure the function exits right after
assigning syncelem.oldResult so no further access (like the counter increment)
occurs.
- Around line 86-87: The early-return branch currently uses "keyState |= 1<<ENT"
which sets the ENT bit but never clears it, causing sticky keys because keyState
is static; change that logic so the ENT bit is explicitly updated based on
gpio_read(KEYS_GPIO_ENTER): if gpio_read(KEYS_GPIO_ENTER) == 0 then set the bit
(keyState |= 1<<ENT) else clear the bit (keyState &= ~(1<<ENT)); alternatively,
if the early-return should not touch the cached value, leave keyState
unchanged—locate the use of keyState, KEYS_GPIO_ENTER, and ENT and replace the
single |= update with the conditional set/clear using the 1<<ENT mask.
---
Nitpick comments:
In `@radio/src/targets/pa01/key_driver.cpp`:
- Line 90: Update the inline comment that currently reads "//If no interrupt
occurs, it means no key is pressed, so no scanning is needed this time,exit." to
fix spacing after the comma by adding a space before "exit."—i.e., change
",exit." to ", exit."; locate this comment in key_driver.cpp (the comment text
itself) and apply the single-character spacing fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a33761a7-c827-42c9-95c9-f57755c7387e
📒 Files selected for processing (1)
radio/src/targets/pa01/key_driver.cpp
|
Seems not correct, reverted many changes in #7315, let me rework this using AI coder |
|
Replaced by #7405 |
2 participants
1.Resubmit program #7315
2.Exit key scanning when no interrupt occurs or a concurrent task is invoked.However. But，I'm not sure if this function is intended to be called concurrently.
Summary by CodeRabbit