feat: 3x4 matrix keypad support with Moonraker ASSIGN_SPOOL integration#46
Merged
Conversation
- InputManager: polls 3x4 keypad, sends KEYPAD_DIGIT/CONFIRM/CANCEL
messages to ApplicationManager
- ApplicationManager: accumulates digits, shows "Assign to: T{n}" on LCD,
POSTs ASSIGN_SPOOL TOOL=T{n} to Moonraker on # confirm, * clears
- ConfigurationManager: moonraker_url and keypad_on NVS fields with
runtime accessors (isKeypadEnabled, getMoonrakerURL)
- DeviceConfig: keypad_enabled added to PeripheralConfig
- Config web UI: Klipper/Moonraker URL section, keypad toggle in Hardware,
updated hint text for NVS-based hardware control
- WebServerManager: moonraker_url and keypad_enabled in GET/POST config API
- ApplicationManager.h: removed #if ENABLE_KEYPAD compile-time guards,
keypad message types always compiled in (NVS runtime model)
📝 WalkthroughWalkthroughAdds optional 3x4 keypad input, message handlers, and UI/config support to assign spools via Moonraker: keypad events feed ApplicationManager, which validates state (NFC when present) and posts ASSIGN_SPOOL G-code to Moonraker; keypad enablement and Moonraker URL are persisted and exposed in the web UI/API. Changes
Sequence Diagram(s)sequenceDiagram
participant Keypad as Matrix Keypad
participant InputMgr as InputManager
participant AppMgr as ApplicationManager
participant NFC as NFCManager
participant LCD as LCD
participant Moonraker as Moonraker API
Keypad->>InputMgr: key '5' pressed
InputMgr->>AppMgr: sendMessage(KEYPAD_DIGIT, '5')
AppMgr->>AppMgr: handleKeypadDigit('5') -> append buffer
AppMgr->>LCD: update "Assign to: T5"
Keypad->>InputMgr: '#' pressed (confirm)
InputMgr->>AppMgr: sendMessage(KEYPAD_CONFIRM)
AppMgr->>AppMgr: handleKeypadConfirm()
AppMgr->>AppMgr: if buffer empty -> LCD error
AppMgr->>NFC: check spool present (non-native)
alt Spool present or native
AppMgr->>AppMgr: sendAssignSpool("5")
AppMgr->>AppMgr: acquire g_httpMutex (5s)
AppMgr->>Moonraker: POST /printer/gcode/script {"script":"ASSIGN_SPOOL TOOL=T5"}
Moonraker-->>AppMgr: 200 OK
AppMgr->>LCD: display "Assigned"
AppMgr->>AppMgr: clear buffer
else No spool
AppMgr->>LCD: display error, clear buffer
end
Keypad->>InputMgr: '*' pressed (cancel)
InputMgr->>AppMgr: sendMessage(KEYPAD_CANCEL)
AppMgr->>AppMgr: handleKeypadCancel() -> clear buffer
AppMgr->>LCD: display "Cleared"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ApplicationManager.cpp`:
- Around line 924-960: The keypad buffer is not tied to a specific scanned spool
so entries can be applied to the wrong tag; fix by binding entry to the current
spool when typing starts and validating on confirm: in handleKeypadDigit, when
keypadBufferLen_ == 0 capture the current spool ID via
NFCManager::getInstance().getCurrentSpoolState(...) into a new member (e.g.
currentSpoolId_) and set a flag (e.g. keypadBoundSpool_), and if no spool
present refuse to start entry; in handleKeypadConfirm, call
NFCManager::getInstance().getCurrentSpoolState(...) again and compare the
current spool ID to currentSpoolId_ and if they differ clear
keypadBuffer_/keypadBufferLen_, reset keypadBoundSpool_, update the LCD with a
"spool changed" message and abort; also ensure any NFC spool-state change
handler (where CurrentSpoolState is processed) clears
keypadBuffer_/keypadBufferLen_ and keypadBoundSpool_ when the tag is removed or
a different tag is detected so the buffer cannot be reused for another spool
before confirm, then call sendAssignSpool only when the spool IDs match.
- Around line 960-969: sendAssignSpool currently may return early on error but
the caller always shows "Assigned..." and clears the entry; change
sendAssignSpool to return a status (bool or enum) indicating success or failure,
then update the caller in ApplicationManager.cpp to check that return value
before updating lcdManager->updateScreen(...) and before clearing keypadBuffer_
and keypadBufferLen_; use the symbols sendAssignSpool, lcdManager,
keypadBuffer_, and keypadBufferLen_ to locate the code and ensure only on
success you show the Assigned message and clear the buffer, otherwise leave the
buffer intact and show an error/failure message.
- Around line 991-1013: handleKeypadConfirm() performs a blocking HTTP POST
inside ApplicationManager::processMessages() which can freeze the event loop;
add explicit HTTPClient timeouts (call http.setConnectTimeout(...) and
http.setTimeout(...) on the HTTPClient instance before http.begin()/http.POST())
to bound connect/read waits, and then consider moving the POST logic into a
dedicated task (similar to otaDownloadTask in WebServerManager) that performs
the HTTP work and posts the result back to the main loop via an AppMessage;
update references in ApplicationManager (handleKeypadConfirm(), g_httpMutex
usage) to use the new task or at minimum ensure timeouts are set before calling
http.POST().
In `@src/ConfigHTML.h`:
- Around line 123-130: The moonraker_url text input lacks a maxlength and can
accept strings longer than the server-side buffer ConfigUpdate::moonraker_url
(128 bytes), causing silent truncation; update the input element with
id="moonraker_url" to include maxlength="127" (client-side enforcement) and
ensure the placeholder/label remain unchanged so users get immediate feedback
when typing long URLs.
In `@src/WebServerManager.cpp`:
- Line 503: The JSON buffer for parsing is likely undersized: increase the
StaticJsonDocument capacity used in the parsing code (the
StaticJsonDocument<768> instance named doc) to a larger size (e.g.,
StaticJsonDocument<1024>) so deserializeJson has enough headroom for
ConfigUpdate’s string fields and JSON overhead; update any related comments and
re-run deserializeJson error checks to ensure failures no longer occur when
ConfigUpdate fields approach their max lengths.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 01e8df1a-fcb0-4500-aef3-62e226649e75
📒 Files selected for processing (11)
src/ApplicationManager.cppsrc/ApplicationManager.hsrc/ConfigHTML.hsrc/ConfigurationManager.cppsrc/ConfigurationManager.hsrc/DeviceConfig.cppsrc/DeviceConfig.hsrc/InputManager.cppsrc/InputManager.hsrc/WebServerManager.cppsrc/main.cpp
Comment on lines
+924
to
+960
| void ApplicationManager::handleKeypadDigit(const AppMessage& msg) { | ||
| char digit = msg.payload.keypadDigit.digit; | ||
| Serial.printf("EVENT: KeypadDigit - '%c'\n", digit); | ||
|
|
||
| if (keypadBufferLen_ < sizeof(keypadBuffer_) - 1) { | ||
| keypadBuffer_[keypadBufferLen_++] = digit; | ||
| keypadBuffer_[keypadBufferLen_] = '\0'; | ||
| } | ||
|
|
||
| if (lcdManager) { | ||
| char line[17]; | ||
| snprintf(line, sizeof(line), "Assign to: T%s", keypadBuffer_); | ||
| lcdManager->updateScreen(line, "# Confirm * Clr"); | ||
| } | ||
| } | ||
|
|
||
| void ApplicationManager::handleKeypadConfirm() { | ||
| Serial.println("EVENT: KeypadConfirm"); | ||
|
|
||
| if (keypadBufferLen_ == 0) { | ||
| if (lcdManager) lcdManager->updateScreen("No tool entered", "Type number + #"); | ||
| return; | ||
| } | ||
|
|
||
| #ifndef NATIVE_TEST | ||
| // Check if a spool is currently detected | ||
| CurrentSpoolState state; | ||
| bool spoolPresent = NFCManager::getInstance().getCurrentSpoolState(state) && state.present; | ||
| if (!spoolPresent) { | ||
| if (lcdManager) lcdManager->updateScreen("No spool scanned", "Scan tag first"); | ||
| keypadBuffer_[0] = '\0'; | ||
| keypadBufferLen_ = 0; | ||
| return; | ||
| } | ||
| #endif | ||
|
|
||
| sendAssignSpool(keypadBuffer_); |
There was a problem hiding this comment.
Bind the buffered tool entry to the current scan.
This flow only checks that some spool is present when # is pressed. Digits entered before a scan, or for a previous scan, will be applied to whichever tag happens to be present now. Clear the buffer whenever the tag context changes, or capture the current spool ID when entry starts and validate it here before posting.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 933-933: implicit conversion 'LCDManager *' -> bool
(readability-implicit-bool-conversion)
[warning] 934-934: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 944-944: implicit conversion 'LCDManager *' -> bool
(readability-implicit-bool-conversion)
[warning] 944-944: statement should be inside braces
(readability-braces-around-statements)
[warning] 953-953: implicit conversion 'LCDManager *' -> bool
(readability-implicit-bool-conversion)
[warning] 953-953: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 924 - 960, The keypad buffer is not
tied to a specific scanned spool so entries can be applied to the wrong tag; fix
by binding entry to the current spool when typing starts and validating on
confirm: in handleKeypadDigit, when keypadBufferLen_ == 0 capture the current
spool ID via NFCManager::getInstance().getCurrentSpoolState(...) into a new
member (e.g. currentSpoolId_) and set a flag (e.g. keypadBoundSpool_), and if no
spool present refuse to start entry; in handleKeypadConfirm, call
NFCManager::getInstance().getCurrentSpoolState(...) again and compare the
current spool ID to currentSpoolId_ and if they differ clear
keypadBuffer_/keypadBufferLen_, reset keypadBoundSpool_, update the LCD with a
"spool changed" message and abort; also ensure any NFC spool-state change
handler (where CurrentSpoolState is processed) clears
keypadBuffer_/keypadBufferLen_ and keypadBoundSpool_ when the tag is removed or
a different tag is detected so the buffer cannot be reused for another spool
before confirm, then call sendAssignSpool only when the spool IDs match.
Comment on lines
+960
to
+969
| sendAssignSpool(keypadBuffer_); | ||
|
|
||
| if (lcdManager) { | ||
| char line[17]; | ||
| snprintf(line, sizeof(line), "Assigned T%s", keypadBuffer_); | ||
| lcdManager->updateScreen(line, "OK"); | ||
| } | ||
|
|
||
| keypadBuffer_[0] = '\0'; | ||
| keypadBufferLen_ = 0; |
There was a problem hiding this comment.
Don't overwrite assignment failures with a success screen.
sendAssignSpool() can return early for a missing URL, HTTP-mutex timeout, or non-200 response, but this block still shows Assigned... and clears the entry. That gives a false-positive UI and forces the user to retype the tool number for retry.
Possible fix
- sendAssignSpool(keypadBuffer_);
-
- if (lcdManager) {
- char line[17];
- snprintf(line, sizeof(line), "Assigned T%s", keypadBuffer_);
- lcdManager->updateScreen(line, "OK");
- }
-
- keypadBuffer_[0] = '\0';
- keypadBufferLen_ = 0;
+ if (sendAssignSpool(keypadBuffer_)) {
+ if (lcdManager) {
+ char line[17];
+ snprintf(line, sizeof(line), "Assigned T%s", keypadBuffer_);
+ lcdManager->updateScreen(line, "OK");
+ }
+ keypadBuffer_[0] = '\0';
+ keypadBufferLen_ = 0;
+ }This needs sendAssignSpool() to return a status/result.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 962-962: implicit conversion 'LCDManager *' -> bool
(readability-implicit-bool-conversion)
[warning] 963-963: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 960 - 969, sendAssignSpool currently
may return early on error but the caller always shows "Assigned..." and clears
the entry; change sendAssignSpool to return a status (bool or enum) indicating
success or failure, then update the caller in ApplicationManager.cpp to check
that return value before updating lcdManager->updateScreen(...) and before
clearing keypadBuffer_ and keypadBufferLen_; use the symbols sendAssignSpool,
lcdManager, keypadBuffer_, and keypadBufferLen_ to locate the code and ensure
only on success you show the Assigned message and clear the buffer, otherwise
leave the buffer intact and show an error/failure message.
4 tasks
- sendAssignSpool returns bool; caller only shows "Assigned OK" on success - HTTP connect timeout (3s) and read timeout (5s) on Moonraker POST to prevent blocking the event loop if Moonraker is unreachable - maxlength="127" on moonraker_url input to match server buffer size - JSON parse buffer bumped from 768 to 1024 for config POST handler - Replaced personal IP in placeholder with generic printer.local
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ApplicationManager.cpp (1)
924-938:⚠️ Potential issue | 🟠 MajorKeypad buffer not bound to scanned spool — risk of misassignment.
The digit buffer accumulates input without tracking which spool was present when typing started. A user could type digits, swap the NFC tag, then press
#— assigning to the wrong spool.Consider capturing the spool UID when the first digit is entered (when
keypadBufferLen_ == 0) and validating it matches at confirm time. Additionally, clear the buffer inhandleTagRemoved()so stale input cannot be applied to a different tag.🛡️ Sketch of proposed fix
Add a member to track the spool being assigned:
// In ApplicationManager.h, add: char keypadBoundSpoolId_[17] = {0};Then in
handleKeypadDigit:void ApplicationManager::handleKeypadDigit(const AppMessage& msg) { char digit = msg.payload.keypadDigit.digit; Serial.printf("EVENT: KeypadDigit - '%c'\n", digit); +#ifndef NATIVE_TEST + // Bind to current spool when first digit is entered + if (keypadBufferLen_ == 0) { + CurrentSpoolState state; + if (NFCManager::getInstance().getCurrentSpoolState(state) && state.present) { + strncpy(keypadBoundSpoolId_, state.spool_id, sizeof(keypadBoundSpoolId_) - 1); + } else { + keypadBoundSpoolId_[0] = '\0'; + } + } +#endif + if (keypadBufferLen_ < sizeof(keypadBuffer_) - 1) {And in
handleKeypadConfirm, validate the spool matches:bool spoolPresent = NFCManager::getInstance().getCurrentSpoolState(state) && state.present; - if (!spoolPresent) { + if (!spoolPresent || strcmp(state.spool_id, keypadBoundSpoolId_) != 0) { + if (lcdManager) lcdManager->updateScreen("Spool changed", "Re-enter tool #");Clear buffer on tag removal in
handleTagRemoved:void ApplicationManager::handleTagRemoved(const AppMessage& msg) { // ... existing code ... + keypadBuffer_[0] = '\0'; + keypadBufferLen_ = 0; + keypadBoundSpoolId_[0] = '\0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ApplicationManager.cpp` around lines 924 - 938, The keypad buffer must be bound to the NFC spool UID to prevent cross-tag assignment: add a member (e.g., keypadBoundSpoolId_ char[17]) to ApplicationManager, on the first digit input in handleKeypadDigit (when keypadBufferLen_ == 0) capture the current tag UID into keypadBoundSpoolId_, and on confirm in handleKeypadConfirm verify the stored keypadBoundSpoolId_ matches the currently-present tag UID before applying the assignment (reject/clear if it doesn't match); also clear keypadBuffer_ and keypadBoundSpoolId_ in handleTagRemoved (and when user presses clear or buffer is emptied) so stale input cannot be used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ApplicationManager.cpp`:
- Around line 924-938: The keypad buffer must be bound to the NFC spool UID to
prevent cross-tag assignment: add a member (e.g., keypadBoundSpoolId_ char[17])
to ApplicationManager, on the first digit input in handleKeypadDigit (when
keypadBufferLen_ == 0) capture the current tag UID into keypadBoundSpoolId_, and
on confirm in handleKeypadConfirm verify the stored keypadBoundSpoolId_ matches
the currently-present tag UID before applying the assignment (reject/clear if it
doesn't match); also clear keypadBuffer_ and keypadBoundSpoolId_ in
handleTagRemoved (and when user presses clear or buffer is emptied) so stale
input cannot be used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61a3967f-ebbc-41ad-887d-d20e9ea7d36e
📒 Files selected for processing (4)
src/ApplicationManager.cppsrc/ApplicationManager.hsrc/ConfigHTML.hsrc/WebServerManager.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isKeypadEnabled()NVS flag.12), press#to confirm. Scanner POSTsASSIGN_SPOOL TOOL=T{n}directly to Moonraker.*clears entered digits. LCD showsAssign to: T{n}as digits are typed.moonraker_urlfield in ConfigurationManager, NVS, web config UI, and installer. Full URL format (e.g.http://192.168.1.72:7125).#if ENABLE_KEYPADcompile-time guards removed from ApplicationManager.h. Keypad message types always compiled in; hardware init gated by NVSkeypad_onflag at runtime, matching the LCD/LED pattern.Addresses #34.
Test plan
Summary by CodeRabbit