Add troubleshooting page with connectivity diagnostics#9
Conversation
- Add /troubleshooting page with WiFi, MQTT, Spoolman, NFC, and memory checks - Add /api/diagnostics endpoint returning JSON health data - Display scanner Device ID prominently for middleware configuration - Add getPN5180FirmwareVersion() to NFCManager via HardwareNFCConnection - Rename TagWriterHTML.h → OpenPrintTagWriterHTML.h for consistency - Add /troubleshooting nav link to all pages Closes #3
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a troubleshooting page at Changes
Sequence DiagramsequenceDiagram
participant Browser
participant WebServer as WebServerManager
participant WiFiMgr as WiFi Manager
participant MQTTMgr as MQTT Manager
participant Spoolman as External API
participant NFCMgr as NFC Manager
participant System as System Metrics
Browser->>WebServer: GET /troubleshooting
WebServer-->>Browser: TROUBLESHOOTING_HTML (page with UI)
Browser->>WebServer: fetch /api/diagnostics
WebServer->>WiFiMgr: Get status, RSSI, IP
WiFiMgr-->>WebServer: Connection details
WebServer->>MQTTMgr: Get enabled, broker, connection
MQTTMgr-->>WebServer: MQTT status
WebServer->>Spoolman: GET /api/v1/info
Spoolman-->>WebServer: {version: "..."} or timeout
WebServer->>NFCMgr: getPN5180FirmwareVersion()
NFCMgr-->>WebServer: fw[0], fw[1]
WebServer->>System: Get free heap, uptime
System-->>WebServer: Memory metrics
WebServer-->>Browser: {device_id, wifi, mqtt, spoolman, nfc, memory}
Browser->>Browser: runChecks(): parse JSON, update card states
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Poem
🚥 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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/WebServerManager.h (1)
54-55: Consider groupinghandleTroubleshootingPage()with other page handlers.
handleTroubleshootingPage()is a page handler but is declared after the API handlers section. For consistency with the existing organization, it should be grouped with other page handlers (lines 29-39).📁 Suggested reorganization
// Page handlers void handleLanding(); void handleReader(); void handleOpenPrintTagWriter(); void handleTigerTagWriter(); void handleOpenTag3DWriter(); void handleSharedCSS(); void handleSharedJS(); void handleOpenPrintTagLogo(); void handleTigerTagLogo(); void handleUpdatePage(); void handleConfigPage(); + void handleTroubleshootingPage(); // API handlers void handleApiStatus(); // ... other API handlers ... void handleApiPostConfig(); void handleApiDiagnostics(); - void handleTroubleshootingPage();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.h` around lines 54 - 55, Move the declaration of handleTroubleshootingPage() out of the API handlers section and place it with the other page handler declarations (the block containing the other handle*Page functions) so it is grouped consistently with page handlers; ensure only the declaration is relocated and keep handleApiDiagnostics() with the API handlers.src/TroubleshootingHTML.h (1)
63-73: Navigation link order differs from other pages.The navigation here has "Config" before "Update" (lines 70-71), while
OpenPrintTagWriterHTML.hhas "Update" before "Troubleshooting" before "Config" (lines 28-30). Consider keeping consistent navigation order across all pages for better UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TroubleshootingHTML.h` around lines 63 - 73, Reorder the navigation links in the <nav> block so the link order matches other pages (e.g., ensure the "Update" anchor appears before the "Troubleshooting" anchor and "Config" comes after), adjusting the anchors with href="/update", href="/troubleshooting", and href="/config" in TroubleshootingHTML.h to match the ordering used in OpenPrintTagWriterHTML.h; preserve existing classes like class="active" on the Troubleshooting link.src/NFCManager.cpp (1)
115-125: Unsafestatic_castviolates interface abstraction.The cast
static_cast<HardwareNFCConnection*>(connection_)assumes the connection is alwaysHardwareNFCConnection, butconnection_is declared asNFCConnectionI*. If a different implementation were injected viasetConnection(), this would cause undefined behavior.Consider one of these approaches:
- Add
getPN5180FirmwareVersion()andisPN5180Ready()to theNFCConnectionIinterface (with default no-op implementations).- Use
dynamic_castwith a null check (though RTTI may be disabled on ESP32).- Store a separate
HardwareNFCConnection*when the connection is created internally.Current production code always uses
HardwareNFCConnection, so this works in practice, but it's fragile for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NFCManager.cpp` around lines 115 - 125, The current NFCManager::getPN5180FirmwareVersion uses unsafe static_cast to HardwareNFCConnection*, breaking the NFCConnectionI abstraction; change the design so NFCManager does not assume concrete type: either (A) add isPN5180Ready() and getPN5180FirmwareVersion(uint8_t[2]) to the NFCConnectionI interface (with default no-op/false implementations) and call those methods on connection_, or (B) if you must keep the methods only on HardwareNFCConnection, ensure setConnection stores a separate HardwareNFCConnection* when a HardwareNFCConnection is provided and check that pointer here before calling, avoiding static_cast; update NFCManager::getPN5180FirmwareVersion, NFCConnectionI, and setConnection accordingly.src/WebServerManager.cpp (1)
227-244: Synchronous HTTP call blocks web server for up to 3 seconds.The Spoolman reachability check performs a blocking HTTP GET with a 3-second timeout. This blocks the web server's single-threaded request handling, potentially causing other requests to queue or timeout.
For a diagnostics endpoint that's called infrequently, this is acceptable, but consider:
- Reducing the timeout (e.g., 1-2 seconds) since this is just a connectivity check.
- Documenting this latency expectation for the frontend.
The current implementation is functional for the troubleshooting use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 227 - 244, The synchronous Spoolman reachability check in the spoolmanEnabled block of WebServerManager.cpp uses HTTPClient http with http.setTimeout(3000), which can block the single-threaded web server for up to 3s; reduce that timeout to 1000–2000 ms (e.g., 1500) by changing the http.setTimeout call and keep the rest of the logic (snprintf(infoUrl...), http.GET(), deserializeJson(info...)) intact; additionally, add a short comment near spoolman["reachable"] or the diagnostics endpoint to document the expected latency for the frontend.
🤖 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/HardwareNFCConnection.cpp`:
- Around line 115-117: The code currently ignores the boolean return of
readEEprom() before assigning fw_ and setting pn5180Ready_, which can mark the
device ready with invalid firmware data; change the logic in the block that
calls readEEprom() so you capture its return value and only assign fw_[0],
fw_[1] and set pn5180Ready_ = true when readEEprom() returns true, and handle
the failure path consistently (e.g., log/error and return false or leave
pn5180Ready_ false), mirroring how setupRF() is checked.
In `@src/TroubleshootingHTML.h`:
- Around line 250-254: The catch block is passing raw service IDs
('wifi','mqtt','spoolman','nfc','heap') into setCheck, which shows raw IDs
instead of user-facing names; change the catch to map each id to its display
name (e.g. via an existing helper or a new lookup like getDisplayName(id) or a
DISPLAY_NAME map) and pass that display name as the third argument to setCheck
(keep setCheck(id, 'fail', <displayName>, 'Error fetching diagnostics')). Ensure
you reference the same ids and the setCheck function when implementing the
lookup.
---
Nitpick comments:
In `@src/NFCManager.cpp`:
- Around line 115-125: The current NFCManager::getPN5180FirmwareVersion uses
unsafe static_cast to HardwareNFCConnection*, breaking the NFCConnectionI
abstraction; change the design so NFCManager does not assume concrete type:
either (A) add isPN5180Ready() and getPN5180FirmwareVersion(uint8_t[2]) to the
NFCConnectionI interface (with default no-op/false implementations) and call
those methods on connection_, or (B) if you must keep the methods only on
HardwareNFCConnection, ensure setConnection stores a separate
HardwareNFCConnection* when a HardwareNFCConnection is provided and check that
pointer here before calling, avoiding static_cast; update
NFCManager::getPN5180FirmwareVersion, NFCConnectionI, and setConnection
accordingly.
In `@src/TroubleshootingHTML.h`:
- Around line 63-73: Reorder the navigation links in the <nav> block so the link
order matches other pages (e.g., ensure the "Update" anchor appears before the
"Troubleshooting" anchor and "Config" comes after), adjusting the anchors with
href="/update", href="/troubleshooting", and href="/config" in
TroubleshootingHTML.h to match the ordering used in OpenPrintTagWriterHTML.h;
preserve existing classes like class="active" on the Troubleshooting link.
In `@src/WebServerManager.cpp`:
- Around line 227-244: The synchronous Spoolman reachability check in the
spoolmanEnabled block of WebServerManager.cpp uses HTTPClient http with
http.setTimeout(3000), which can block the single-threaded web server for up to
3s; reduce that timeout to 1000–2000 ms (e.g., 1500) by changing the
http.setTimeout call and keep the rest of the logic (snprintf(infoUrl...),
http.GET(), deserializeJson(info...)) intact; additionally, add a short comment
near spoolman["reachable"] or the diagnostics endpoint to document the expected
latency for the frontend.
In `@src/WebServerManager.h`:
- Around line 54-55: Move the declaration of handleTroubleshootingPage() out of
the API handlers section and place it with the other page handler declarations
(the block containing the other handle*Page functions) so it is grouped
consistently with page handlers; ensure only the declaration is relocated and
keep handleApiDiagnostics() with the API handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eea4fda-f168-4072-b9f4-26a40d4d3a0f
📒 Files selected for processing (14)
src/ConfigHTML.hsrc/HardwareNFCConnection.cppsrc/HardwareNFCConnection.hsrc/LandingHTML.hsrc/NFCManager.cppsrc/NFCManager.hsrc/OpenPrintTagWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/ReaderHTML.hsrc/TigerTagWriterHTML.hsrc/TroubleshootingHTML.hsrc/UpdateHTML.hsrc/WebServerManager.cppsrc/WebServerManager.h
| fw_[0] = firmwareVersion[0]; | ||
| fw_[1] = firmwareVersion[1]; | ||
| pn5180Ready_ = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if readEEprom has any return value or error indication in the PN5180 library
ast-grep --pattern 'readEEprom($$$)'
# Also check the header for pn5180Ready_ and fw_ declarations
rg -n "pn5180Ready_|fw_\[" --type cppRepository: SpoolSense/spoolsense_scanner
Length of output: 602
🏁 Script executed:
# Search for readEEprom method definition and declarations
rg -n "readEEprom" --type cpp -B 2 -A 5
# Check the broader context around lines 115-117
sed -n '100,125p' src/HardwareNFCConnection.cppRepository: SpoolSense/spoolsense_scanner
Length of output: 2606
🏁 Script executed:
# Check other readEEprom calls and their error handling
rg -n "readEEprom" --type cpp -B 1 -A 3
# Show the setupRF call handling for comparison
sed -n '118,125p' src/HardwareNFCConnection.cppRepository: SpoolSense/spoolsense_scanner
Length of output: 1340
Check the readEEprom() return value before setting pn5180Ready_.
The readEEprom() method returns a bool indicating success or failure, but line 114 ignores this return value. If the EEPROM read fails, fw_ will contain uninitialized data while pn5180Ready_ is still set to true. This is inconsistent with how setupRF() is handled immediately after (line 120), where the return value is properly checked.
Update the code to check the return value:
Proposed fix
// Read firmware version
uint8_t firmwareVersion[2];
- nfc_->readEEprom(PN5180_FIRMWARE_VERSION, firmwareVersion, 2);
- fw_[0] = firmwareVersion[0];
- fw_[1] = firmwareVersion[1];
- pn5180Ready_ = true;
+ if (nfc_->readEEprom(PN5180_FIRMWARE_VERSION, firmwareVersion, 2)) {
+ fw_[0] = firmwareVersion[0];
+ fw_[1] = firmwareVersion[1];
+ pn5180Ready_ = true;
+ } else {
+ Serial.println("HardwareNFCConnection: Failed to read firmware version");
+ return false;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnection.cpp` around lines 115 - 117, The code currently
ignores the boolean return of readEEprom() before assigning fw_ and setting
pn5180Ready_, which can mark the device ready with invalid firmware data; change
the logic in the block that calls readEEprom() so you capture its return value
and only assign fw_[0], fw_[1] and set pn5180Ready_ = true when readEEprom()
returns true, and handle the failure path consistently (e.g., log/error and
return false or leave pn5180Ready_ false), mirroring how setupRF() is checked.
| } catch(e) { | ||
| ['wifi','mqtt','spoolman','nfc','heap'].forEach(id => { | ||
| setCheck(id, 'fail', id, 'Error fetching diagnostics'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Minor: Error handler shows raw IDs instead of proper names.
When the diagnostics fetch fails, the error message shows the raw ID (e.g., "wifi") instead of the proper display name (e.g., "WiFi"). This is a minor cosmetic issue.
} catch(e) {
- ['wifi','mqtt','spoolman','nfc','heap'].forEach(id => {
- setCheck(id, 'fail', id, 'Error fetching diagnostics');
+ const names = {wifi:'WiFi', mqtt:'MQTT Broker', spoolman:'Spoolman', nfc:'NFC Reader (PN5180)', heap:'Memory'};
+ ['wifi','mqtt','spoolman','nfc','heap'].forEach(id => {
+ setCheck(id, 'fail', names[id], 'Error fetching diagnostics');
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TroubleshootingHTML.h` around lines 250 - 254, The catch block is passing
raw service IDs ('wifi','mqtt','spoolman','nfc','heap') into setCheck, which
shows raw IDs instead of user-facing names; change the catch to map each id to
its display name (e.g. via an existing helper or a new lookup like
getDisplayName(id) or a DISPLAY_NAME map) and pass that display name as the
third argument to setCheck (keep setCheck(id, 'fail', <displayName>, 'Error
fetching diagnostics')). Ensure you reference the same ids and the setCheck
function when implementing the lookup.
|
Addressing CodeRabbit findings: Fixed |
|
Good bot 🥕 — error handler now uses proper display names. |
|
All checks passing on hardware (ESP32-S3). CodeRabbit findings addressed — |
1 participant
Summary
Adds
/troubleshootingpage to the scanner web UI with live connectivity checks and a prominently displayed Device ID for middleware configuration.Changes
/troubleshootingpage with pass/fail checks for WiFi, MQTT, Spoolman, NFC reader, and free heap/api/diagnosticsendpoint returning JSON health datagetPN5180FirmwareVersion()added to NFCManager via HardwareNFCConnectionTagWriterHTML.hrenamed toOpenPrintTagWriterHTML.hfor consistency/troubleshootingnav link added to all pagesHow to Test
spoolsense.local/troubleshootingChecklist
pio run -e esp32devandpio run -e esp32s3zero)Closes #3
Summary by CodeRabbit
Release Notes
New Features
UI Updates