Feature/wifi setup improvements #187
Conversation
- Added _safe_remove_directory() method to handle permission errors gracefully - Fixes permissions on __pycache__ directories before removal - Updates uninstall_plugin() and install methods to use safe removal - Resolves [Errno 13] Permission denied errors during plugin install/uninstall
- Add 90-second grace period (3 checks at 30s intervals) before enabling AP mode - Change AP to open network (no password) for easier initial setup - Add verification script for WiFi setup - Update documentation with grace period details and open network info - Improve WiFi monitor daemon logging and error handling
📝 WalkthroughWalkthroughThis pull request enhances WiFi setup documentation, introduces a comprehensive verification script, adds extensive logging to the WiFi monitor daemon, and improves error handling in directory removal operations. The changes address setup verification, monitoring instrumentation, and robustness. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/WIFI_SETUP.md (1)
109-123: Documentation inconsistency:ap_passwordshown in config but docs describe open network.The configuration example shows
"ap_password": "ledmatrix123"on line 112, but the documentation repeatedly states the AP is an open network with no password (lines 55, 133, 282, 324). This is confusing for users.Either:
- Remove
ap_passwordfrom the example config if it's truly unused, or- Clarify that
ap_passwordexists for future/optional use but the default behavior is an open network
🤖 Fix all issues with AI agents
In @scripts/verify_wifi_setup.sh:
- Around line 229-248: The script sets ETH_CONNECTED but never uses it; either
remove the ETH_CONNECTED assignments or persist and use the flag in the
summary/decision logic. To fix, either delete all references to ETH_CONNECTED in
the Ethernet check block (leaving only the check_pass/check_warn calls), or keep
setting ETH_CONNECTED (true/false) and update the summary or downstream logic to
read ETH_CONNECTED (for example, include it in the overall connectivity summary
or conditional flows that consume connection state); locate the assignments to
ETH_CONNECTED and the related calls check_pass and check_warn to update
accordingly.
🧹 Nitpick comments (5)
scripts/utils/wifi_monitor_daemon.py (1)
132-142: Consider usinglogging.exception()in the error handler.While
exc_info=Trueworks, the static analysis suggests usinglogging.exception()which is more idiomatic. However, the current approach is functionally correct and the additional state logging on lines 136-140 provides useful debugging context.The inner try-except on line 139 catching a broad
Exceptionis acceptable here since it's purely for diagnostic logging and shouldn't affect the daemon's resilience.Optional: Use logging.exception for consistency
except Exception as e: - logger.error(f"Error in monitor loop: {e}", exc_info=True) + logger.exception(f"Error in monitor loop: {e}") logger.error(f"Error details - type: {type(e).__name__}, args: {e.args}")src/plugin_system/store_manager.py (2)
1418-1424: Unused loop variabledirscan be renamed to_dirs.The
dirsvariable fromos.walk()is not used within the loop body. Per Python convention, rename it to_dirsto indicate it's intentionally unused.Rename unused variable
- for root, dirs, files in os.walk(path): + for root, _dirs, files in os.walk(path):
1438-1455: Consider usinglogging.exception()to preserve stack traces.Lines 1439, 1451, and 1454 use
self.logger.error()withoutexc_info=True, which means stack traces won't be logged. This reduces debugging capability. The static analysis correctly flags this.Preserve stack traces with logging.exception or exc_info=True
except Exception as e2: - self.logger.error(f"Failed to remove {path} even after fixing permissions: {e2}") + self.logger.error(f"Failed to remove {path} even after fixing permissions: {e2}", exc_info=True) # Last resort: try with ignore_errors try: shutil.rmtree(path, ignore_errors=True) # Check if it actually got removed if not path.exists(): self.logger.warning(f"Removed {path} with ignore_errors=True (some files may remain)") return True else: self.logger.error(f"Could not remove {path} even with ignore_errors") return False except Exception as e3: - self.logger.error(f"Final removal attempt failed for {path}: {e3}") + self.logger.error(f"Final removal attempt failed for {path}: {e3}", exc_info=True) return False except Exception as e: - self.logger.error(f"Unexpected error removing {path}: {e}") + self.logger.error(f"Unexpected error removing {path}: {e}", exc_info=True) return Falsescripts/verify_wifi_setup.sh (2)
300-312: Hardcoded port 5001 may not match all deployments.The web interface port is hardcoded to 5001. Consider making this configurable or adding a comment noting the assumption.
Make port configurable or document assumption
# 11. Check web interface WiFi API echo "=== Web Interface WiFi API ===" +WEB_PORT="${LEDMATRIX_WEB_PORT:-5001}" if systemctl is-active --quiet ledmatrix-web.service 2>/dev/null; then # Try to test the WiFi status API endpoint - if curl -s -f "http://localhost:5001/api/v3/wifi/status" >/dev/null 2>&1; then + if curl -s -f "http://localhost:${WEB_PORT}/api/v3/wifi/status" >/dev/null 2>&1; then check_pass "WiFi status API endpoint is accessible" else check_warn "WiFi status API endpoint is not accessible (may be expected if web interface requires auth)"
284-298: WiFi Manager import test is useful but path insertion may fail.Line 290 inserts
$PROJECT_ROOTinto Python's path, which is good. However, if the WiFi Manager has unmet dependencies, the error message on line 293 ("may be expected") could be more descriptive about potential causes.Add more context to the warning message
# Try to instantiate (but don't fail if it errors - may need config) if python3 -c "import sys; sys.path.insert(0, '$PROJECT_ROOT'); from src.wifi_manager import WiFiManager; wm = WiFiManager(); print('OK')" 2>/dev/null; then check_pass "WiFi Manager can be instantiated" else - check_warn "WiFi Manager instantiation failed (may be expected)" + check_warn "WiFi Manager instantiation failed (may need config file or dependencies)" fi
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/WIFI_SETUP.mdscripts/utils/wifi_monitor_daemon.pyscripts/verify_wifi_setup.shsrc/plugin_system/store_manager.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
**/*.py: Prefer clear, readable code over clever optimizations (Simplicity First principle)
Make intentions clear through naming and structure (Explicit over Implicit principle)
Validate inputs and handle errors early (Fail Fast principle)
Use docstrings for classes and complex functions
Use PascalCase for class names (e.g.,NHLRecentManager)
Use snake_case for function and variable names (e.g.,fetch_game_data)
Use UPPER_SNAKE_CASE for constants (e.g.,ESPN_NHL_SCOREBOARD_URL)
Use leading underscore for private methods (e.g.,_fetch_data)
Use structured logging with context (e.g.,[NHL Recent]) for logging messages
Catch specific exceptions, not bareexcept:statements
Provide user-friendly error messages that explain what went wrong and potential solutions
Implement graceful degradation to continue operation when non-critical features fail
Use type hints for function parameters and return values
Validate required configuration fields on initialization
Provide sensible default values in code rather than in configuration files
Handle different deployment contexts with environment awareness in code
**/*.py: Code must run on Raspberry Pi, not Windows development machine
Optimize code for Raspberry Pi's limited RAM and CPU capabilities
Clean up resources regularly to manage memory effectively
Implement comprehensive logging for remote debugging on Raspberry Pi
Provide clear error messages for troubleshooting
Files:
scripts/utils/wifi_monitor_daemon.pysrc/plugin_system/store_manager.py
**/*manager*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
All sports managers should implement the BaseManager pattern with
__init__(self, config, display_manager, cache_manager),update()for fetching/processing data, anddisplay(self, force_clear=False)for rendering
Files:
src/plugin_system/store_manager.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
All sports, weather, and stock managers should be organized in src/ directory
src/**/*.py: Use rpi-rgb-led-matrix library for hardware control on Raspberry Pi
Use cache_manager.py for data persistence and caching
Implement non-blocking data fetching using background services
Minimize unnecessary redraws to optimize LED matrix display performance
Implement health checks and status reporting
Provide web interface for remote configuration and monitoring on Raspberry Pi
Files:
src/plugin_system/store_manager.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T19:17:41.914Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursor/rules/raspberry-pi-development.mdc:0-0
Timestamp: 2025-12-27T19:17:41.914Z
Learning: Applies to src/**/*.py : Provide web interface for remote configuration and monitoring on Raspberry Pi
Applied to files:
docs/WIFI_SETUP.md
📚 Learning: 2025-12-27T19:17:56.132Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursor/rules/testing-standards.mdc:0-0
Timestamp: 2025-12-27T19:17:56.132Z
Learning: Applies to test/*.py : Validate hardware integration on Raspberry Pi with actual LED matrix
Applied to files:
docs/WIFI_SETUP.md
🧬 Code graph analysis (1)
scripts/utils/wifi_monitor_daemon.py (1)
web_interface/blueprints/api_v3.py (2)
get_wifi_status(5628-5654)disable_ap_mode(5835-5857)
🪛 Ruff (0.14.11)
scripts/utils/wifi_monitor_daemon.py
134-134: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
138-138: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
139-139: Do not catch blind exception: Exception
(BLE001)
140-140: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/plugin_system/store_manager.py
1418-1418: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
1439-1439: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1451-1451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1453-1453: Do not catch blind exception: Exception
(BLE001)
1454-1454: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 Shellcheck (0.11.0)
scripts/verify_wifi_setup.sh
[warning] 242-242: ETH_CONNECTED appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (9)
docs/WIFI_SETUP.md (2)
144-168: Good addition of verification section.The new verification section clearly documents the script's capabilities and provides a helpful checklist of what gets verified. This aligns well with the new
scripts/verify_wifi_setup.shadded in this PR.
280-288: Security considerations appropriately updated.The security section properly warns users about the open AP network design and provides actionable guidance for production deployments. This is important given the intentional removal of AP password protection.
scripts/utils/wifi_monitor_daemon.py (3)
61-72: Good comprehensive startup logging.The initial configuration and status logging provides valuable context for remote debugging on Raspberry Pi. This aligns well with the coding guidelines requirement for comprehensive logging.
159-163: Correct handling ofdisable_ap_mode()return value.The tuple unpacking
(success, message)properly matches the API shown in the relevant code snippets fromapi_v3.py. The differentiated logging for success vs failure cases provides clear diagnostic information.
94-116: Thorough state change logging.The detailed state change logging with WiFi SSID, IP, signal strength, Ethernet status, and AP mode state provides excellent observability for troubleshooting. The visual separators (
===) make log parsing easier.src/plugin_system/store_manager.py (1)
1426-1432: Good addition of per-file permission fixes.Adding file permission fixes alongside directory permission fixes improves the robustness of
__pycache__cleanup. This handles cases where individual.pycfiles have restrictive permissions.scripts/verify_wifi_setup.sh (3)
1-5: Good use ofset -uwithoutset -e.Using
set -uto catch undefined variables while omittingset -eis the right choice for a verification script that should continue checking even when individual checks fail.
24-41: Well-designed helper functions for consistent output.The
check_pass,check_fail,check_warn, andinfofunctions provide clean, colorized output with automatic counter tracking. This is a good pattern for diagnostic scripts.
102-132: Good JSON validation approach using Python.Using
python3 -m json.toolfor validation and inline Python for field extraction is pragmatic given this runs on a Raspberry Pi where Python is always available. This avoids addingjqas a dependency.
| # 8. Check Ethernet connection | ||
| echo "=== Ethernet Status ===" | ||
| ETH_CONNECTED=false | ||
| if command -v nmcli >/dev/null 2>&1; then | ||
| ETH_STATUS=$(nmcli -t -f DEVICE,TYPE,STATE device status 2>/dev/null | grep -E "ethernet|eth" | grep "connected" || echo "") | ||
| if [ -n "$ETH_STATUS" ]; then | ||
| ETH_CONNECTED=true | ||
| check_pass "Ethernet is connected" | ||
| else | ||
| check_warn "Ethernet is not connected" | ||
| fi | ||
| elif command -v ip >/dev/null 2>&1; then | ||
| if ip addr show eth0 2>/dev/null | grep -q "inet " || ip addr show 2>/dev/null | grep -E "eth|enp" | grep -q "inet "; then | ||
| ETH_CONNECTED=true | ||
| check_pass "Ethernet appears to be connected" | ||
| else | ||
| check_warn "Ethernet does not appear to be connected" | ||
| fi | ||
| fi | ||
| echo "" |
There was a problem hiding this comment.
ETH_CONNECTED variable is set but never used.
The variable is assigned on lines 231, 235, and 242 but is never read elsewhere in the script. Either remove the variable assignments or use it (e.g., in the summary or for conditional logic).
Option 1: Remove unused variable
# 8. Check Ethernet connection
echo "=== Ethernet Status ==="
-ETH_CONNECTED=false
if command -v nmcli >/dev/null 2>&1; then
ETH_STATUS=$(nmcli -t -f DEVICE,TYPE,STATE device status 2>/dev/null | grep -E "ethernet|eth" | grep "connected" || echo "")
if [ -n "$ETH_STATUS" ]; then
- ETH_CONNECTED=true
check_pass "Ethernet is connected"
else
check_warn "Ethernet is not connected"
fi
elif command -v ip >/dev/null 2>&1; then
if ip addr show eth0 2>/dev/null | grep -q "inet " || ip addr show 2>/dev/null | grep -E "eth|enp" | grep -q "inet "; then
- ETH_CONNECTED=true
check_pass "Ethernet appears to be connected"
else
check_warn "Ethernet does not appear to be connected"
fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 8. Check Ethernet connection | |
| echo "=== Ethernet Status ===" | |
| ETH_CONNECTED=false | |
| if command -v nmcli >/dev/null 2>&1; then | |
| ETH_STATUS=$(nmcli -t -f DEVICE,TYPE,STATE device status 2>/dev/null | grep -E "ethernet|eth" | grep "connected" || echo "") | |
| if [ -n "$ETH_STATUS" ]; then | |
| ETH_CONNECTED=true | |
| check_pass "Ethernet is connected" | |
| else | |
| check_warn "Ethernet is not connected" | |
| fi | |
| elif command -v ip >/dev/null 2>&1; then | |
| if ip addr show eth0 2>/dev/null | grep -q "inet " || ip addr show 2>/dev/null | grep -E "eth|enp" | grep -q "inet "; then | |
| ETH_CONNECTED=true | |
| check_pass "Ethernet appears to be connected" | |
| else | |
| check_warn "Ethernet does not appear to be connected" | |
| fi | |
| fi | |
| echo "" | |
| # 8. Check Ethernet connection | |
| echo "=== Ethernet Status ===" | |
| if command -v nmcli >/dev/null 2>&1; then | |
| ETH_STATUS=$(nmcli -t -f DEVICE,TYPE,STATE device status 2>/dev/null | grep -E "ethernet|eth" | grep "connected" || echo "") | |
| if [ -n "$ETH_STATUS" ]; then | |
| check_pass "Ethernet is connected" | |
| else | |
| check_warn "Ethernet is not connected" | |
| fi | |
| elif command -v ip >/dev/null 2>&1; then | |
| if ip addr show eth0 2>/dev/null | grep -q "inet " || ip addr show 2>/dev/null | grep -E "eth|enp" | grep -q "inet "; then | |
| check_pass "Ethernet appears to be connected" | |
| else | |
| check_warn "Ethernet does not appear to be connected" | |
| fi | |
| fi | |
| echo "" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 242-242: ETH_CONNECTED appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In @scripts/verify_wifi_setup.sh around lines 229 - 248, The script sets
ETH_CONNECTED but never uses it; either remove the ETH_CONNECTED assignments or
persist and use the flag in the summary/decision logic. To fix, either delete
all references to ETH_CONNECTED in the Ethernet check block (leaving only the
check_pass/check_warn calls), or keep setting ETH_CONNECTED (true/false) and
update the summary or downstream logic to read ETH_CONNECTED (for example,
include it in the overall connectivity summary or conditional flows that consume
connection state); locate the assignments to ETH_CONNECTED and the related calls
check_pass and check_warn to update accordingly.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.