fix(pa01): Optimize matrix keyboard scanning#7315
Conversation
|
Please cleanup (remove commented out block, dead code, etc) and document properly in the code. From what I understand, it seems that different groups of keys are polled in each |
Yes, testing has shown that it significantly reduces the probability of I2C errors. However, the original method of scanning only once during boot must be retained. Since it's a matrix keyboard, we don't actually need interrupts at all. |
raphaelcoeffic
left a comment
raphaelcoeffic
left a comment
There was a problem hiding this comment.
Overview
The PR reworks pollKeys() for the PA01 target so that instead of driving all four matrix columns and reading all rows inside a single pollKeys() call (1× heavy I²C burst every 10 ms), the scan is split into a 4-step state machine — each invocation drives one column and reads one set of rows. The interrupt-driven shouldReadKeys path is removed. Per the author this eliminates I²C errors seen under contention (e.g. audio playback during model switching).
Correctness issues
extern volatile bool errorOccurs;won't link cleanly. Inbsp_io.cpp:59,errorOccursisstatic(internal linkage), so the newexterninkey_driver.cppcan't reference it. It only compiles today because nothing actually reads the variable inkey_driver.cpp— please delete theextern(dead code), or dropstaticinbsp_io.cppif the intent is really to share it.- Stale header declaration.
radio/src/targets/pa01/bsp_io.h:48still declaresbool bsp_get_shouldReadKeys();even though the definition, internal flag, and ISR have all been removed. Remove it to keep the header consistent. - IRQ wiring fully removed? The diff drops
_io_int_handlerand thegpio_init_int(IO_INT_GPIO, ...)call. Double-check that no other path (sleep/wake, bootloader) still expects the AW9523B INT line to drive an interrupt, and consider removing theIO_INT_GPIOdefine if now unused. - State machine never resets on suspend. If
suspendI2CTaskstoggles mid-cycle,pollkey_stepand the staticresultaccumulator are frozen; when I²C resumes, the remaining 1–3 columns get OR'd into a partially-staleresult, yielding one spurious combined frame before recovery. Aresult = 0; pollkey_step = 0;reset on re-enable (or at entry to step 0) would be safer. - Uninitialized first cycle.
pollkey_stepstarts at 0, so the firstpollKeys()only drives~OUT1and returns; the actual column-1 read happens next tick. Not a bug, but it meanskeyStateis zero for ~40 ms after boot. - Key latency goes from 10 ms to 40 ms. With
keysPollingCycle()called from the 10 ms timer (edgetx.cpp:211) and the full matrix now taking 4 ticks,keyStateupdates at 25 Hz instead of 100 Hz. Probably imperceptible for short presses, but repeat-rate / long-press debouncing inkeys.cppwas tuned around the old cadence — worth testing repeat and combo keys explicitly.
Code quality
Following up on the earlier cleanup request:
- Duplicated function body. The
#if !defined(BOOT) … #else … #endifgives two fullpollKeys()implementations. Consider keeping a single implementation and factoring the per-column read into a helper, instead of duplicating the scan code. - Magic hex step values.
0x01 / 0x03 / 0x07 / 0x0Flook bitmask-like but are just sequence markers. Anenum(STEP_COL1 … STEP_COL4) or plain0..3+ aswitchwould be much clearer. Yoda comparisons (0x03==pollkey_step) also don't match the project style. volatile staticonresult/pollkey_step.volatileisn't buying anything ifpollKeys()is only invoked from the 10 ms timer task — it just inhibits optimization. If you do want to defend against concurrent callers, use a proper atomic/critical section; otherwise dropvolatile.- Concurrency guard removed without a note. The old
syncelem.ui8ReadInProgressblock is gone. That's probably fine under a single-caller assumption, but please add a brief comment stating that assumption so the guard isn't reintroduced later "just in case." READ_ENTER_KEYmacro. Astatic inlinehelper would be clearer and avoid macro scoping pitfalls. It's also repeated in every step branch — one call perpollKeys()would do.- Formatting. Missing spaces around operators (
if(!pollkey_step),1<<TR1U,?true:false), trailing blank lines, inconsistent indent — please run the formatter used elsewhere in this tree. - Comments. Add a short block at the top of
pollKeys()describing the state machine (column order, why it was split, effective scan rate) per the prior review request.
Suggestions
- Delete the now-unused
bsp_get_shouldReadKeys,shouldReadKeys,_io_int_handler, andIO_INT_GPIOreferences (header + source). - Remove
extern volatile bool errorOccurs;inkey_driver.cpp(unused), or make it a real shared symbol with a consumer. - Collapse the
#if !defined(BOOT)/#elseduplication into a single implementation with a helper. - Replace hex step codes with an
enum+switch. - Drop
volatileon the statics; document the single-caller assumption. - Add a one-line comment where the old
delay_us(BSP_READ_AFTER_WRITE_DELAY)used to be, explaining it's no longer needed because the 10 ms tick provides settle time between column drive and read — so nobody "fixes" it back in. - Quick manual test of repeat / long-press / trim-hold behaviors to confirm the 40 ms scan period doesn't regress UX.
Risk
Low blast radius — PA01-only, no shared-driver touches. Main residual risks are the linker/stale-header hygiene items and the increased matrix latency. Functional regressions would show up as lost repeat events or laggy trims.
|
I suggested this change, polling matrix with I2C added a lot of waits in between each write and read. Pipeline reads and writes with 10ms timer task will be more efficient and less error prone. |
|
@raphaelcoeffic You used claude code to do code review? |
richardclli
changed the title
richardclli
added
backport/2.11
|
@sneone This PR has conflicts with 2.11 branch, you better make a 2.11 version for fixing 2.11 branch |
|
@sneone Why you removing the interrupt handling? I remember this is necessary otherwise the MCU load can be much higher. Because I have once disabled the interrupt and @gagarinlg spot the problem and add back the interrupt. |
|
If only one row (or column) is scanned per cycle, interrupts must be disabled, since it is impossible to determine which specific row (or column) the key press originates from. If interrupts are enabled, a full scan of all rows and columns is mandatory. If the MCU load is too high, it is still recommended to keep the original approach of scanning the entire matrix in one cycle. What do you think? @richardclli |
|
I think interrupt is useful, it can save you from one read. Only need to store the latest read, so after you change the output and input changes, then it will interrupt and change the dirty flag, and when you see the dirty flag, you do a read. And because this polling will be called every 10ms, so interrupt will occurs before next read. And if not interrupt occurs, then you will save one read. And this should happens 9x% of the time because usually user will not press any buttons for most cases.
|
📝 WalkthroughWalkthroughThe PR adds two early-exit guards to the ChangesKey polling early-exit conditions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (1)
radio/src/targets/pa01/key_driver.cpp (1)
108-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
ui8ReadInProgressfrom getting stuck non-zero on concurrent calls.Line 118 can deadlock scanning under a race: two callers may both hit the
> 1return path, and neither reaches Line 174 to clear the flag. That leavespollKeys()permanently returning cached state.Proposed fix
- if (syncelem.ui8ReadInProgress != 0) { - keyState = syncelem.oldResult; - } - - // ui8ReadInProgress was 0, increment it - syncelem.ui8ReadInProgress++; - // Double check before continuing, as non-atomic, non-blocking so far - // If ui8ReadInProgress is above 1, then there was concurrent task calling it, exit - if (syncelem.ui8ReadInProgress > 1) { - keyState = syncelem.oldResult; - return; - } + if (syncelem.ui8ReadInProgress != 0) { + keyState = syncelem.oldResult; + return; + } + + // Claim scan ownership + syncelem.ui8ReadInProgress = 1;🤖 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 108 - 119, A race can leave syncelem.ui8ReadInProgress stuck >0 because both callers take the early-return path and never clear the flag; modify the concurrent-path in the pollKeys() logic so that when you detect syncelem.ui8ReadInProgress > 1 you restore keyState = syncelem.oldResult and then decrement/clear syncelem.ui8ReadInProgress (or use an atomic compare-and-swap clear) before returning so the flag is never left non-zero; ensure every exit path from the read section (including the >1 branch) clears or decrements syncelem.ui8ReadInProgress to avoid permanent stuck state.
🤖 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 108-119: A race can leave syncelem.ui8ReadInProgress stuck >0
because both callers take the early-return path and never clear the flag; modify
the concurrent-path in the pollKeys() logic so that when you detect
syncelem.ui8ReadInProgress > 1 you restore keyState = syncelem.oldResult and
then decrement/clear syncelem.ui8ReadInProgress (or use an atomic
compare-and-swap clear) before returning so the flag is never left non-zero;
ensure every exit path from the read section (including the >1 branch) clears or
decrements syncelem.ui8ReadInProgress to avoid permanent stuck state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09da2ff2-90c9-4293-8c1a-90791a104001
📒 Files selected for processing (1)
radio/src/targets/pa01/key_driver.cpp
3 participants
1.I2C errors can easily occur in certain situations, such as when sound plays during model switching. Switching to a four-cycle step-by-step scan resolved the issue (tested hundreds of times without errors).
Summary by CodeRabbit