feat: add driver for MCP23017#11015
Conversation
|
|
@myembeddedstuff, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
📝 WalkthroughWalkthroughThe change adds MCP23017 I2C keyboard detection, a complete keyboard driver with tap and long-press handling, and integration that maps generated key events into existing input events. ChangesMCP23017 keyboard support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant I2CScanner
participant MCP23017Keyboard
participant KbI2cBase
participant InputObserver
I2CScanner->>I2CScanner: detect MCP23017
I2CScanner->>KbI2cBase: select keyboard model 0x20
KbI2cBase->>MCP23017Keyboard: initialize on selected TwoWire bus
KbI2cBase->>MCP23017Keyboard: trigger()
MCP23017Keyboard-->>KbI2cBase: queued character events
KbI2cBase->>InputObserver: notify mapped InputEvent
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/input/cardKbI2cImpl.cpp (1)
15-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winArray size mismatch prevents scanning of all keyboards.
The array
i2caddr_scannow contains 6 elements sinceMCP23017_KB_ADDRwas added. However, theelsebranch (whenT_LORA_PAGERis not defined) still hardcodesi2caddr_asizeto5. This will truncate the scan, causing the last device in the array (e.g.,TCA8418_KB_ADDR) to be ignored entirely.To prevent this class of bugs entirely, compute the size dynamically for all cases instead of relying on a hardcoded number.
🐛 Proposed fix
- uint8_t i2caddr_scan[] = {CARDKB_ADDR, TDECK_KB_ADDR, BBQ10_KB_ADDR, MCP23017_KB_ADDR, MPR121_KB_ADDR, TCA8418_KB_ADDR}; -#if defined(T_LORA_PAGER) - uint8_t i2caddr_asize = sizeof(i2caddr_scan) / sizeof(i2caddr_scan[0]); -#else - uint8_t i2caddr_asize = 5; -#endif + uint8_t i2caddr_scan[] = {CARDKB_ADDR, TDECK_KB_ADDR, BBQ10_KB_ADDR, MCP23017_KB_ADDR, MPR121_KB_ADDR, TCA8418_KB_ADDR}; + uint8_t i2caddr_asize = std::size(i2caddr_scan);🤖 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 `@src/input/cardKbI2cImpl.cpp` around lines 15 - 20, Update i2caddr_asize alongside i2caddr_scan so both preprocessor branches compute the array length dynamically using sizeof, removing the hardcoded value 5 and ensuring every address, including TCA8418_KB_ADDR, is scanned.
🧹 Nitpick comments (3)
src/detect/ScanI2C.cpp (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding the array size.
Using a hardcoded number
7can lead to an out-of-bounds read or missed device type if the array is modified again in the future without updating the count. Since this project uses C++17, prefer usingstd::size(types).♻️ Proposed refactor
- ScanI2C::DeviceType types[] = {CARDKB, TDECKKB, BBQ10KB, RAK14004, MCP23017, MPR121KB, TCA8418KB}; - return firstOfOrNONE(7, types); + ScanI2C::DeviceType types[] = {CARDKB, TDECKKB, BBQ10KB, RAK14004, MCP23017, MPR121KB, TCA8418KB}; + return firstOfOrNONE(std::size(types), types);🤖 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 `@src/detect/ScanI2C.cpp` around lines 34 - 35, Update the firstOfOrNONE call in ScanI2C’s device-type selection to derive the count from the local types array using std::size(types) instead of the hardcoded 7, preserving the existing array contents and return behavior.src/input/MCP23017Keyboard.cpp (2)
167-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove excessive newlines from logs and fix typo.
Standard logging macros usually append their own newline. The triple
\n\n\nadds unnecessary noise. Also, "deattached" should be "detached".🧹 Proposed cleanup
void MCP23017Keyboard::attachInterrupt(uint8_t pin, void (*func)(void)) const { - LOG_DEBUG("MCP23017 attached interrupt\n\n\n"); + LOG_DEBUG("MCP23017 attached interrupt"); pinMode(pin, INPUT_PULLUP); ::attachInterrupt(digitalPinToInterrupt(pin), func, FALLING); } void MCP23017Keyboard::detachInterrupt(uint8_t pin) const { - LOG_DEBUG("MCP23017 deattached interrupt\n\n\n"); + LOG_DEBUG("MCP23017 detached interrupt"); ::detachInterrupt(pin); }🤖 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 `@src/input/MCP23017Keyboard.cpp` around lines 167 - 176, Update MCP23017Keyboard::attachInterrupt and MCP23017Keyboard::detachInterrupt to remove the embedded triple newline characters from both LOG_DEBUG messages, relying on the logging macro’s normal line termination. Correct the detach message spelling from “deattached” to “detached”.
249-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove empty
elseblock.🧹 Proposed cleanup
- if (keyCount(keyRegister) != 1) { - return; - } else { - } + if (keyCount(keyRegister) != 1) { + 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 `@src/input/MCP23017Keyboard.cpp` around lines 249 - 252, Remove the empty else block following the keyCount(keyRegister) check, leaving the early return for counts not equal to one and allowing normal execution to continue afterward.
🤖 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.
Inline comments:
In `@src/detect/ScanI2CTwoWire.cpp`:
- Around line 651-667: Verify the TCA9535 behavior when reading register 0x0A,
including wrapped-command and NACK outcomes, and update the MCP23017/TCA9535
detection in the surrounding switch case so failed or wrapped reads cannot
misclassify the device; preserve RAK12035 detection. Also complete the missing
closing parenthesis in the TCA9535/MCP23017 comment and correct the indentation
of the nested register-read line to match trunk fmt.
In `@src/input/kbI2cBase.cpp`:
- Around line 394-396: Reorder the switch cases in the input handler so CARDKB’s
`case 0x00` no longer falls through into the MCP23017 block. Move `case 0x00` to
immediately precede `case 0x10`, preserving the shared CARDKB/T-DECK request
flow while leaving the `case 0x20` MCP23017 handler separate.
- Around line 82-84: Update the ScanI2C::WIRE1 case to initialize MCPkeyboard
when cardkb_found.address equals MCP23017_KB_ADDR, passing &Wire1 to
MCPkeyboard.begin; keep the existing Wire initialization in the other bus case
unchanged.
In `@src/input/MCP23017Keyboard.cpp`:
- Around line 264-274: Replace the raw millis arithmetic in
src/input/MCP23017Keyboard.cpp lines 264-274 with
Throttle::isWithinTimespanMs(last_tap, MULTI_TAP_THRESHOLD), removing
tap_interval and its negative-wraparound branch while preserving the surrounding
key-state behavior. Also update lines 301-311 to remove held_interval and its
negative-wraparound logic, using !Throttle::isWithinTimespanMs(last_tap,
LONG_PRESS_THRESHOLD) for the long-press timeout check.
---
Outside diff comments:
In `@src/input/cardKbI2cImpl.cpp`:
- Around line 15-20: Update i2caddr_asize alongside i2caddr_scan so both
preprocessor branches compute the array length dynamically using sizeof,
removing the hardcoded value 5 and ensuring every address, including
TCA8418_KB_ADDR, is scanned.
---
Nitpick comments:
In `@src/detect/ScanI2C.cpp`:
- Around line 34-35: Update the firstOfOrNONE call in ScanI2C’s device-type
selection to derive the count from the local types array using std::size(types)
instead of the hardcoded 7, preserving the existing array contents and return
behavior.
In `@src/input/MCP23017Keyboard.cpp`:
- Around line 167-176: Update MCP23017Keyboard::attachInterrupt and
MCP23017Keyboard::detachInterrupt to remove the embedded triple newline
characters from both LOG_DEBUG messages, relying on the logging macro’s normal
line termination. Correct the detach message spelling from “deattached” to
“detached”.
- Around line 249-252: Remove the empty else block following the
keyCount(keyRegister) check, leaving the early return for counts not equal to
one and allowing normal execution to continue afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a02defa-cc8e-4dc7-a0c1-65eac4c8198f
📒 Files selected for processing (9)
src/detect/ScanI2C.cppsrc/detect/ScanI2C.hsrc/detect/ScanI2CTwoWire.cppsrc/input/MCP23017Keyboard.cppsrc/input/MCP23017Keyboard.hsrc/input/cardKbI2cImpl.cppsrc/input/kbI2cBase.cppsrc/input/kbI2cBase.hsrc/main.cpp
| case TCA9535_ADDR: // this can also be MCP23017_ADDR (both 0x20 | ||
| case RAK120352_ADDR: | ||
| case RAK120353_ADDR: | ||
| registerValue = getRegisterValue(ScanI2CTwoWire::RegisterLocation(addr, 0x02), 1); | ||
| if (registerValue == addr.address) { // RAK12035 returns its I2C address at 0x02 (eg 0x20) | ||
| type = RAK12035; | ||
| logFoundDevice("RAK12035", (uint8_t)addr.address); | ||
| } else { | ||
| type = TCA9535; | ||
| logFoundDevice("TCA9535", (uint8_t)addr.address); | ||
| // TCA9535 only has registers 0x00-0x07; MCP23017 has IOCON at 0x0A | ||
| registerValue = getRegisterValue(ScanI2CTwoWire::RegisterLocation(addr, 0x0A), 1); | ||
| if (registerValue != 0xFF) { | ||
| type = MCP23017; | ||
| logFoundDevice("MCP23017", (uint8_t)addr.address); | ||
| } else { | ||
| type = TCA9535; | ||
| logFoundDevice("TCA9535", (uint8_t)addr.address); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify register 0x0A read behavior for TCA9535.
The differentiation between MCP23017 and TCA9535 relies on reading register 0x0A. The code assumes registerValue will be 0xFF for a TCA9535. If the TCA9535 wraps the command byte (e.g., 0x0A % 8 = 0x02), register 0x02 (Output Port 0) must default to 0xFF for this logic to hold. If the device NACKs the address instead, getRegisterValue() might return 0x00 (since available() would be 0), causing it to be incorrectly identified as an MCP23017 (0x00 != 0xFF).
Also, fix the missing closing parenthesis in the comment on line 651, and the off-by-one indentation on line 659 to comply with trunk fmt as per coding guidelines.
💻 Proposed fixes (assuming register logic is verified to work)
- case TCA9535_ADDR: // this can also be MCP23017_ADDR (both 0x20
+ case TCA9535_ADDR: // this can also be MCP23017_ADDR (both 0x20)
case RAK120352_ADDR:
case RAK120353_ADDR:
registerValue = getRegisterValue(ScanI2CTwoWire::RegisterLocation(addr, 0x02), 1);
if (registerValue == addr.address) { // RAK12035 returns its I2C address at 0x02 (eg 0x20)
type = RAK12035;
logFoundDevice("RAK12035", (uint8_t)addr.address);
} else {
- // TCA9535 only has registers 0x00-0x07; MCP23017 has IOCON at 0x0A
+ // TCA9535 only has registers 0x00-0x07; MCP23017 has IOCON at 0x0A
registerValue = getRegisterValue(ScanI2CTwoWire::RegisterLocation(addr, 0x0A), 1);
if (registerValue != 0xFF) {
type = MCP23017;🤖 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 `@src/detect/ScanI2CTwoWire.cpp` around lines 651 - 667, Verify the TCA9535
behavior when reading register 0x0A, including wrapped-command and NACK
outcomes, and update the MCP23017/TCA9535 detection in the surrounding switch
case so failed or wrapped reads cannot misclassify the device; preserve RAK12035
detection. Also complete the missing closing parenthesis in the TCA9535/MCP23017
comment and correct the indentation of the nested register-read line to match
trunk fmt.
Source: Coding guidelines
| if (cardkb_found.address == MCP23017_KB_ADDR) { | ||
| MCPkeyboard.begin(MCP23017_KB_ADDR, &Wire); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize MCP23017 for the secondary I2C bus (Wire1).
You have added initialization for Wire (bus 0), but the equivalent logic is missing for the Wire1 case block above this (around line 70). Without it, the MCP23017 keyboard will fail to initialize on boards that connect it to the secondary I2C interface.
Add the following initialization to the ScanI2C::WIRE1 case:
if (cardkb_found.address == MCP23017_KB_ADDR) {
MCPkeyboard.begin(MCP23017_KB_ADDR, &Wire1);
}🤖 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 `@src/input/kbI2cBase.cpp` around lines 82 - 84, Update the ScanI2C::WIRE1 case
to initialize MCPkeyboard when cardkb_found.address equals MCP23017_KB_ADDR,
passing &Wire1 to MCPkeyboard.begin; keep the existing Wire initialization in
the other bus case unchanged.
| case 0x20: { // MCP23017 (Dirección I2C por defecto) | ||
| MCPkeyboard.trigger(); | ||
| InputEvent e = {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix hijacked CARDKB fallthrough.
By inserting case 0x20: directly after case 0x00: (line 393), the CARDKB logic now falls through into the MCP23017 handler instead of its original destination. This completely breaks CARDKB support because it will attempt to read MCP23017-specific registers.
To restore correctness, move case 0x00: to the end of the case 0x20: block so that it correctly falls through to case 0x10: (T-DECK), as it did prior to this PR.
// Move the CARDKB case from line 393 to immediately precede the T-DECK case:
case 0x00: // CARDKB
case 0x10: { // T-DECK
i2cBus->requestFrom((int)cardkb_found.address, 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 `@src/input/kbI2cBase.cpp` around lines 394 - 396, Reorder the switch cases in
the input handler so CARDKB’s `case 0x00` no longer falls through into the
MCP23017 block. Move `case 0x00` to immediately precede `case 0x10`, preserving
the shared CARDKB/T-DECK request flow while leaving the `case 0x20` MCP23017
handler separate.
| uint32_t now = millis(); | ||
| int32_t tap_interval = now - last_tap; | ||
|
|
||
| if (tap_interval < 0) { | ||
| // long running, millis has overflowed. | ||
| last_tap = 0; | ||
| state = Busy; | ||
| return; | ||
| } | ||
|
|
||
| if (next_key != last_key || tap_interval > MULTI_TAP_THRESHOLD) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use Throttle for time-based checks instead of raw millis() arithmetic.
As per coding guidelines, time-elapsed checks must use the Throttle helper rather than manual millis() arithmetic. The current logic subtracts and converts to a signed integer (int32_t), which fails to properly handle unsigned millis() wraparounds and causes bugs if the device runs for extended periods.
src/input/MCP23017Keyboard.cpp#L264-L274: Remove thetap_intervaland< 0logic. Replace the timeout check with!Throttle::isWithinTimespanMs(last_tap, MULTI_TAP_THRESHOLD).src/input/MCP23017Keyboard.cpp#L301-L311: Remove theheld_intervaland< 0logic. Replace the timeout check with!Throttle::isWithinTimespanMs(last_tap, LONG_PRESS_THRESHOLD).
📍 Affects 1 file
src/input/MCP23017Keyboard.cpp#L264-L274(this comment)src/input/MCP23017Keyboard.cpp#L301-L311
🤖 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 `@src/input/MCP23017Keyboard.cpp` around lines 264 - 274, Replace the raw
millis arithmetic in src/input/MCP23017Keyboard.cpp lines 264-274 with
Throttle::isWithinTimespanMs(last_tap, MULTI_TAP_THRESHOLD), removing
tap_interval and its negative-wraparound branch while preserving the surrounding
key-state behavior. Also update lines 301-311 to remove held_interval and its
negative-wraparound logic, using !Throttle::isWithinTimespanMs(last_tap,
LONG_PRESS_THRESHOLD) for the long-press timeout check.
Source: Coding guidelines
2 participants
Description
This PR adds a driver for the MCP23017. It is a simple implementation, following a similar structure to the MPR121 driver.
I have added the option to define a custom keymap connection using the
CUSTOM_MCP23017_MAPdefinition.Please note that I am unable to test this code on the specific devices listed below, as I do not have access to them.
🤝 Attestations
Summary by CodeRabbit