Add discovery and improve consistency #33
Conversation
|
""" WalkthroughA new UDP-based discovery protocol for Ubiquiti airOS devices is implemented, including packet parsing, error handling, and logging. Supporting exception classes and a binary fixture generator script are added. Tests are introduced for the discovery protocol. Additionally, a logger variable is renamed for consistency, and the project version is incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Device as airOS Device
participant UDP as UDP Socket
participant Protocol as AirosDiscoveryProtocol
participant Callback as User Callback
Device->>UDP: Broadcasts discovery packet (UDP 10002)
UDP->>Protocol: datagram_received(data, addr)
Protocol->>Protocol: parse_airos_packet(data, host_ip)
alt Packet valid
Protocol->>Callback: callback(parsed_device_info)
else Packet invalid
Protocol->>Protocol: Log warning/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
airos/discovery.py (3)
32-41: Consider adding type annotation for callback parameter.The callback parameter should be annotated as an async callable since it's being used with
asyncio.create_task().- def __init__(self, callback: Callable[[dict[str, Any]], None]) -> None: + def __init__(self, callback: Callable[[dict[str, Any]], Awaitable[None]]) -> None:You'll also need to import
Awaitablefromcollections.abc:from collections.abc import Callable +from collections.abc import Awaitable
82-108: Consider adding input validation for packet size.While the method handles short packets gracefully, consider adding an upper bound check to prevent processing excessively large packets that could consume memory or processing time.
def parse_airos_packet(self, data: bytes, host_ip: str) -> dict[str, Any] | None: """Parse a raw airOS discovery UDP packet. + + Args: + data: The raw byte data of the UDP packet payload. + host_ip: The IP address of the sender, used as a fallback or initial IP. + + Returns: + A dictionary containing parsed device information if successful, + otherwise None. Values will be None if not found or cannot be parsed. + """ + # Sanity check for packet size + if len(data) > 4096: # Reasonable upper bound for discovery packets + log = f"Packet too large ({len(data)} bytes), ignoring to prevent resource exhaustion" + _LOGGER.warning(log) + return None
134-147: Consider extracting MAC address parsing to a helper method.The MAC address parsing logic could be extracted to a helper method to improve readability and reusability.
+ def _parse_mac_address(self, data: bytes, offset: int) -> tuple[str | None, int]: + """Parse MAC address from packet data. + + Returns: + Tuple of (mac_address_string, new_offset) + """ + expected_length = 6 + if (len(data) - offset) >= expected_length: + mac_bytes = data[offset:offset + expected_length] + mac_address = ":".join(f"{b:02x}" for b in mac_bytes).upper() + _LOGGER.debug(f"Parsed MAC from type 0x06: {mac_address}") + return mac_address, offset + expected_length + else: + log = f"Truncated MAC address TLV (Type 0x06). Expected {expected_length}, got {len(data) - offset} bytes." + _LOGGER.warning(log) + return None, offsetThen in the main parsing loop:
if tlv_type == 0x06: # Device MAC Address (fixed 6-byte value) - expected_length: int = 6 - if (len(data) - offset) >= expected_length: - mac_bytes: bytes = data[offset : offset + expected_length] - parsed_info["mac_address"] = ":".join( - f"{b:02x}" for b in mac_bytes - ).upper() - offset += expected_length - log = f"Parsed MAC from type 0x06: {parsed_info['mac_address']}" - _LOGGER.debug(log) - else: - log = f"Truncated MAC address TLV (Type 0x06). Expected {expected_length}, got {len(data) - offset} bytes. Remaining: {data[offset:].hex()}" - _LOGGER.warning(log) - break + parsed_info["mac_address"], offset = self._parse_mac_address(data, offset) + if parsed_info["mac_address"] is None: + break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airos/airos8.py(6 hunks)airos/discovery.py(1 hunks)airos/exceptions.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airos/discovery.py (1)
airos/exceptions.py (3)
AirosDiscoveryError(28-29)AirosEndpointError(36-37)AirosListenerError(32-33)
🔇 Additional comments (6)
airos/exceptions.py (1)
28-37: Well-designed exception hierarchy for discovery functionality.The new exception classes follow proper inheritance patterns and provide appropriate granularity for different types of discovery errors. The naming is clear and descriptive, and the docstrings adequately explain their purpose.
pyproject.toml (1)
7-7: Appropriate version bump for new feature addition.The minor version increment from 0.1.8 to 0.2.0 correctly reflects the addition of new discovery functionality while maintaining backward compatibility.
airos/airos8.py (1)
21-21: Good consistency improvement with logger naming.Standardizing the logger variable name to
_LOGGERfollows common Python conventions and improves consistency across the codebase, particularly with the new discovery module.airos/discovery.py (3)
43-51: LGTM! Proper UDP socket configuration for broadcast listening.The socket setup correctly enables broadcast reception and address reuse, which are necessary for UDP discovery protocols.
255-261: Good error handling with appropriate exception types.The exception handling correctly differentiates between parsing errors (struct/index) and unexpected errors, raising appropriate custom exception types.
75-80: Fix connection_lost exception handling logic.The method raises
AirosDiscoveryErroreven whenexcisNone, which indicates normal connection closure. This will cause unnecessary exceptions during clean shutdown.def connection_lost(self, exc: Exception | None) -> None: """Handle connection is lost or closed.""" _LOGGER.debug("AirosDiscoveryProtocol connection lost.") if exc: _LOGGER.exception("AirosDiscoveryProtocol connection lost due to") - raise AirosDiscoveryError from None + raise AirosDiscoveryError from excLikely an incorrect or invalid review comment.
|
@coderabbitai please update the summary and review |
|
Tip For best results, initiate chat on the files or code changes. @CoMPaTech I'll update the summary and conduct a comprehensive review of the PR changes. [regenerate_summary] |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_discovery.py (1)
5-5: Remove unnecessary comment in import section.The comment "Add this import" is not needed in production code and should be removed for cleanliness.
-import socket # Add this import +import socket
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fixtures/airos_sta_discovery_packet.binis excluded by!**/*.bin
📒 Files selected for processing (3)
airos/discovery.py(1 hunks)script/generate_discovery_fixture.py(1 hunks)tests/test_discovery.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- script/generate_discovery_fixture.py
🚧 Files skipped from review as they are similar to previous changes (1)
- airos/discovery.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_discovery.py (2)
airos/discovery.py (6)
AirosDiscoveryProtocol(18-275)parse_airos_packet(88-275)datagram_received(52-73)connection_made(43-50)connection_lost(81-86)error_received(75-79)airos/exceptions.py (2)
AirosDiscoveryError(28-29)AirosEndpointError(36-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (9)
tests/test_discovery.py (9)
40-61: LGTM! Comprehensive test for successful packet parsing.The test covers all expected fields from the parsed packet and validates the scrubbed test data correctly. The assertions match the expected values from the fixture.
64-79: Well-structured test for invalid header handling.The test correctly validates both the exception raising and the logging behavior. Good use of mocking to verify internal logging calls.
82-97: Good test coverage for packet length validation.Properly tests the edge case of packets too short for the fixed header and verifies the appropriate logging.
100-115: Effective test for TLV parsing robustness.The test creates a realistic scenario with valid header and MAC TLV followed by a truncated TLV, which should trigger struct parsing errors.
137-150: Good error handling test with proper logging verification.The test correctly validates that parsing errors are caught, logged, and re-raised as the appropriate exception type.
153-167: Excellent test for connection setup validation.Comprehensive testing of the connection_made method including transport assignment and socket option configuration.
170-178: Good test for normal connection closure.Properly tests the happy path of connection closure without exceptions.
181-193: Well-structured test for exceptional connection closure.Good use of context managers to handle multiple patches and exception testing simultaneously.
196-204: Effective test for UDP error handling.Simple but effective test that validates error logging for network errors.
1 participant
Code suggestions by Gemini and CodeRabbit
Summary by CodeRabbit