Conversation
8357eed to
d4eb3b3
Compare
|
Hi @njoerd114, thanks for this massive PR! The architecture ( However, we cannot merge it yet because your branch is outdated and would revert recent features (v2026-02-13.3/4). Please:
Looking forward to merging this once rebased! |
d4eb3b3 to
934643c
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements support for Vodafone Station modems by introducing an abstract driver interface pattern. The changes refactor the existing FritzBox-specific code into a pluggable driver architecture, allowing DOCSight to support multiple modem types.
Changes:
- Introduced abstract
Driverinterface and driver loader system for pluggable modem support - Refactored FritzBox functionality from
app/fritzbox.pyintoapp/drivers/fritzbox.pyimplementing the new interface - Added Vodafone Station driver (
app/drivers/vodafone.py) with AES-CCM authentication support - Updated web UI with modem type dropdown selector and modem-specific defaults endpoint
- Added comprehensive test coverage for drivers, loader, and integration
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| app/drivers/interface.py | Defines abstract Driver interface with login(), get_docsis_data(), get_device_info(), and get_connection_info() methods |
| app/drivers/loader.py | Dynamic driver loading system with AVAILABLE_DRIVERS registry |
| app/drivers/fritzbox.py | FritzBox driver refactored to implement Driver interface (migrated from app/fritzbox.py) |
| app/drivers/vodafone.py | New Vodafone Station driver with AES-CCM encrypted authentication |
| app/drivers/init.py | Package initialization for drivers module |
| app/drivers/README.md | Comprehensive documentation for adding new drivers |
| app/fritzbox.py | Removed (functionality moved to app/drivers/fritzbox.py) |
| app/main.py | Updated polling loop to use driver loader instead of direct fritzbox imports |
| app/web.py | Updated endpoints to use driver loader; added /api/modem-defaults endpoint |
| app/config.py | Added modem_type config field and MODEM_DEFAULTS for driver-specific defaults |
| app/templates/setup.html | Added modem type dropdown with dynamic defaults loading |
| app/templates/settings.html | Added modem type dropdown with dynamic defaults loading |
| app/i18n/*.json | Added translations for modem type selector and hints |
| requirements.txt | Added pycryptodome>=3.15.0 for Vodafone Station AES encryption |
| docker-compose.dev.yml | Added DEBUG=1 environment variable for development |
| .gitignore | Added venv/ to ignored paths |
| tests/test_drivers.py | Comprehensive tests for Driver interface, loader, and FritzBox driver |
| tests/test_vodafone_driver.py | Comprehensive tests for Vodafone Station driver |
| tests/test_modem_integration.py | Integration tests for driver selection and testing |
| tests/test_web.py | Updated tests for poll endpoint; added tests for modem-defaults endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| secret = {"Password": password, "Nonce": current_session_id} | ||
| plaintext = json.dumps(secret).encode("ascii") |
There was a problem hiding this comment.
Potential password leak in debug logs. The code includes the password in a JSON payload that is logged at debug level (line 76-77). While debug logging is typically disabled in production, this creates a risk of password exposure in log files if debug mode is enabled. Consider sanitizing the payload before logging or avoiding logging the plaintext secret entirely.
|
I would have loved to reuse the existing dependency cryptography instead of introducing pycryptodome by the way, however it broke the login because of a missing method... might be a topic for a refactoring after a while... will look into the review items today in the afternoon. |
|
Hey @njoerd114, just a quick update on our side: We've been working on the project roadmap and published a v2.0 Milestone that includes a Unified Collector Architecture (#23). This will standardize how all data sources (FritzBox, Vodafone Station, Smokeping, etc.) integrate into the pipeline. Your Vodafone Station work is tracked in #14 and is part of this milestone. We've also overhauled the Adding Modem Support wiki page with the full raw data format and analyzer reference. Your PR is still very welcome! The review items from before still apply (rebase, i18n, auth check). The No rush, just wanted to keep you in the loop. |
fd8d5a9 to
8777cd7
Compare
e75ab0e to
db37899
Compare
Signed-off-by: Niklas Beinghaus <hi@niklasbeinghaus.com>
Signed-off-by: Niklas Beinghaus <hi@niklasbeinghaus.com>
Signed-off-by: Niklas Beinghaus <hi@niklasbeinghaus.com>
Signed-off-by: Niklas Beinghaus <hi@niklasbeinghaus.com>
e0ccb06 to
15aa43f
Compare
DISCLAIMER: heavily vibe-coded, but works for me
First, I created an abstract class (interface) for modem drivers and refactored the fritzbox driver to implement the new interface.
Afterwards, I added a new driver based on this repository, which provides a prometheus exporter for the Vodafone Stations https://github.com/icaruseffect/arris-tg3442de-exporter
I also updated the frontend to include a dropdown selector for the modem type and made the "Test connection" button work for both FritzBoxes and the Vodafone Station.
This implements #14