feat!: Cloud protocol support #53
Conversation
* All files related to local protocol have been moved to `pyg90alarm.local` hierarchy * Added support for cloud protocol and its messages (`pyg90alarm.cloud` hierarchy) * `G90Alarm` class no longer inherits from `G90DevicesNotifications` - to support different notification styles (local or cloud) the `G90NotificationProtocol` class has been introduced, that adds programmatic protocol for notifications, methods of which will be called by either `G90LocalNotifications` or `G90CloudNotifications` class instance (instantiated by `G90Alarm.use_local_notifications()` and `G90Alarm.use_cloud_notifications()` methods, respectively) * Added support for alert state `254` while processing history entries
There was a problem hiding this comment.
PR Overview
This pull request introduces cloud protocol support while refactoring local protocol support into a dedicated hierarchy, in addition to adding a new alert state and new notification routing methods. Key changes include:
- Moving local protocol code into the pyg90alarm.local hierarchy.
- Adding cloud protocol support and a new G90CloudNotifications class.
- Changing the inheritance of the G90Alarm class and updating the notifications API to support both local and cloud notifications.
Reviewed Changes
| File | Description |
|---|---|
| src/pyg90alarm/local/notifications.py | Introduces G90LocalNotifications with minor spelling issues in docstrings. |
| src/pyg90alarm/cloud/init.py | Adds new cloud notifications module and updates all. |
| src/pyg90alarm/cloud/const.py | Adds cloud constants including direction and command enumerations. |
| src/pyg90alarm/cloud/notifications.py | Implements G90CloudNotifications with cloud connection handling. |
| src/pyg90alarm/cloud/protocol.py | Implements protocols for cloud messaging including header parsing. |
| src/pyg90alarm/alarm.py | Updates G90Alarm class to use the new notifications API and new cloud parameters. |
| src/pyg90alarm/const.py | Introduces new cloud and alert-related constants. |
| src/pyg90alarm/local/history.py | Updates history mapping to include the new ALARM state. |
| Other local modules | Various adjustments in import paths and minor refactoring for consistency. |
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/pyg90alarm/local/notifications.py:92
- Correct 'alers' to 'alerts' in the docstring to avoid spelling errors.
""" Listens for notifications/alers from the device.
There was a problem hiding this comment.
PR Overview
This pull request adds cloud protocol support while refactoring local protocol handling. Key changes include:
- Moving local protocol files into the pyg90alarm/local hierarchy and updating relative imports.
- Introducing a new cloud protocol implementation and associated notifications.
- Adjusting the G90Alarm class to support switching between local and cloud notifications and adding a new alert state (254).
Reviewed Changes
| File | Description |
|---|---|
| src/pyg90alarm/cloud/__init__.py | Introduces cloud notifications export with minimal docstrings. |
| src/pyg90alarm/local/notifications.py | Reworks local notification handling with placeholder methods. |
| src/pyg90alarm/cloud/protocol.py | Implements cloud protocol message parsing and wire conversion. |
| src/pyg90alarm/cloud/notifications.py | Adds cloud protocol connection handling including upstream logic. |
| src/pyg90alarm/alarm.py | Updates G90Alarm to use notification protocol abstraction and support cloud notifications. |
| Other files | Adjust relative imports, constants, and setup configurations to support the changes. |
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
| @@ -338,7 +367,7 @@ async def get_host_info(self) -> G90HostInfo: | |||
| """ | |||
| res = await self.command(G90Commands.GETHOSTINFO) | |||
| info = G90HostInfo(*res) | |||
| self.device_id = info.host_guid | |||
| self._notifications.device_id = info.host_guid | |||
There was a problem hiding this comment.
Direct assignment to device_id on the _notifications instance bypasses encapsulation and any potential validation. Consider using a dedicated setter or initialization method to update the device identifier.
| self._notifications.device_id = info.host_guid | |
| self._notifications.set_device_id(info.host_guid) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
|
||
| # Implementation of datagram protocol, | ||
| # https://docs.python.org/3/library/asyncio-protocol.html#datagram-protocols | ||
| def connection_made(self, transport: BaseTransport) -> None: |
There was a problem hiding this comment.
The connection_made method is currently defined without any implementation. It would be beneficial to add minimal logging or error handling to track connection events, or remove the empty method if it is not required.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| return | ||
|
|
||
| try: | ||
| if not self._upstream_transport: |
There was a problem hiding this comment.
In the send_upstream method, connection errors are merely logged at debug level and no retry logic or error propagation is implemented. Consider adding a retry mechanism or raising an error if upstream communication is critical.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
nominate them as modules hence include into the packaging process
pyg90alarm.localhierarchypyg90alarm.cloudhierarchy)G90Alarmclass no longer inherits fromG90DevicesNotifications- to support different notification styles (local or cloud) theG90NotificationProtocolclass has been introduced, that adds programmatic protocol for notifications, methods of which will be called by eitherG90LocalNotificationsorG90CloudNotificationsclass instance (instantiated byG90Alarm.use_local_notifications()andG90Alarm.use_cloud_notifications()methods, respectively)254while processing history entries