Add diagnostic information to the device #560
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds diagnostic data export functionality to the device, encapsulating device information, product details, and traits within a diagnostic_data() method. This enables local testing without requiring Home Assistant integration and provides a convenient way to inspect device state with automatic redaction of sensitive information (duid, localKey, mac, bssid).
Key Changes
- Added
diagnostic_data()method to return redacted device/product/trait information - Initialized device features and network info with default values to ensure complete diagnostic output
- Added
as_dict()method to traits for serialization support
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| roborock/devices/device.py | Implements diagnostic_data() method with sensitive data redaction |
| roborock/devices/traits/v1/init.py | Adds as_dict() method to serialize trait data |
| roborock/devices/traits/v1/device_features.py | Initializes all device feature fields to False for consistent state |
| roborock/devices/traits/v1/network_info.py | Initializes ip field to empty string |
| tests/devices/test_v1_device.py | Adds test coverage and snapshot validation for diagnostic_data() |
| tests/devices/snapshots/test_v1_device.ambr | Updates snapshots with diagnostic data output |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
allenporter
force-pushed
the
diagnostics
branch
from
83b2d3e to
537212c
Compare
| 'dnd': dict({ | ||
| }), | ||
| 'home': dict({ | ||
| }), | ||
| 'map_content': dict({ | ||
| }), | ||
| 'maps': dict({ | ||
| }), | ||
| 'network_info': dict({ | ||
| 'ip': '', | ||
| }), | ||
| 'rooms': dict({ | ||
| }), | ||
| 'sound_volume': dict({ | ||
| }), | ||
| 'status': dict({ | ||
| }), | ||
| }), |
There was a problem hiding this comment.
Are these supposed to be blank?
There was a problem hiding this comment.
oh just kidding - isee each one has a different one filled out.
There was a problem hiding this comment.
How about we omit unset values? Let me know what you think of that change. (pushed)
| """Initialize the trait.""" | ||
| self._device_uid = device_uid | ||
| self._cache = cache | ||
| self.ip = "" |
There was a problem hiding this comment.
What is the variable for? The NetworkInfo object should have ip on it, right?
There was a problem hiding this comment.
This is here because network info can't be inspected if this is not set since it is not an optional field. I ended up just hacking it here because i expect refresh() to be called on a trait to populate this. Another option is to make it nullable?
This encapsulated diagnostic information entirely within the device since the device has all of the local information such as traits, device information, product information, etc. The motivation is to facilitate local testing rather than testing via Home Assistant and dealing with mocks and it could be helpful for other users of the library as well.
allenporter
force-pushed
the
diagnostics
branch
from
8013659 to
2938578
Compare
|
|
||
| T = TypeVar("T") | ||
|
|
||
| REDACT_KEYS = {"duid", "localKey", "mac", "bssid", "sn", "ip"} |
There was a problem hiding this comment.
i'm not sure if we need to redact ip as it is the local ip and that shows up in the logs. but probably not worth the change for now, just going to approve but maybe if we touch this file again, it could be worth removing?
2 participants
This encapsulated diagnostic information entirely within the device since the device has all of the local information such as traits, device information, product information, etc. The motivation is to facilitate local testing rather than testing via Home Assistant and dealing with mocks and it could be helpful for other users of the library as well.
The diagnostic redaction is copied from nest.