Refactor connection handling #66

michaelarnauts · 2025-10-31T14:38:08Z

This PR is the result of a session with Copilot where the connection handling is refactored.

In the process, tests were added as well.

I still need to review this properly.

Copilot

Pull Request Overview

This PR introduces comprehensive test coverage and refactors the connection management architecture to be more robust and maintainable. The changes introduce automated reconnection logic, improved error handling, and better resource cleanup.

Adds comprehensive test suite with unit and integration tests
Refactors connection management to use a reconnection loop pattern
Fixes boolean encoding bug in PDO value encoding
Improves type annotations and error handling throughout

Reviewed Changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
tests/test_util.py	Unit tests for PDO value encoding utilities
tests/test_integration.py	Integration tests for connection lifecycle and reconnection scenarios
tests/test_comfoconnect.py	Comprehensive unit tests for ComfoConnect class
tests/test_bridge.py	Unit tests for Bridge class and EventBus
tests/conftest.py	Shared test fixtures and helpers
pytest.ini	Pytest configuration for async test support
pyproject.toml	Added pytest-asyncio dependency
aiocomfoconnect/util.py	Fixed boolean encoding to use explicit byte conversion
aiocomfoconnect/comfoconnect.py	Refactored connection management with reconnection loop and improved type hints
aiocomfoconnect/bridge.py	Enhanced EventBus with better error handling and improved connection lifecycle management
.gitignore	Fixed to allow Python files and added VS Code exclusion
.github/copilot-instructions.md	Documentation for project tooling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-31T14:41:22Z

aiocomfoconnect/comfoconnect.py

-        self._sensor_callback_fn: Callable = sensor_callback
-        self._alarm_callback_fn: Callable = alarm_callback
+        self._sensor_callback_fn: Optional[Callable[[Sensor, Any], None]] = sensor_callback
+        self._alarm_callback_fn: Optional[Callable[[int, Dict[int, str]], None]] = alarm_callback


The type annotations for callbacks should use Optional consistently. However, line 56 appears correct but line 57 assumes alarm_callback always receives a Dict[int, str] for errors. Based on the test at test_comfoconnect.py:352, the errors parameter can be a dict, but the actual alarm handling code should be reviewed to ensure this type is accurate for all cases.

Suggested change

self._alarm_callback_fn: Optional[Callable[[int, Dict[int, str]], None]] = alarm_callback

self._alarm_callback_fn: Optional[Callable[[int, Optional[Dict[int, str]]], None]] = alarm_callback

Copilot · 2025-10-31T14:41:22Z

aiocomfoconnect/bridge.py


-        message = Message(cmd, msg, self._local_uuid, self.uuid)
+        if self._send_lock is None:
+            # TODO(stability): revisit this once message pipelining is supported safely.


This TODO comment lacks context about what specific stability concerns exist or what conditions would make message pipelining safe. Consider expanding the comment to explain the current limitation or risk.

Suggested change

# TODO(stability): revisit this once message pipelining is supported safely.

# TODO(stability): Currently, we use a lock to serialize outgoing messages because the underlying protocol and/or bridge device does not support concurrent (pipelined) requests safely.

# Concurrent writes may result in interleaved or corrupted messages, race conditions, or unexpected bridge behavior.

# Revisit this if the protocol is updated to support message pipelining, or if it can be demonstrated that concurrent requests are handled correctly by the bridge.

Copilot · 2025-10-31T14:41:23Z

tests/test_integration.py

+
+                # Simulate alarm
+                mock_alarm = Mock()
+                mock_alarm.swProgramVersion = 3222278145  # Firmware > 1.4.0


The magic number 3222278145 should be extracted as a named constant to improve readability and maintainability. Consider defining FIRMWARE_VERSION_1_4_0_PLUS = 3222278145 at the module level.

Copilot · 2025-10-31T14:41:23Z

tests/test_comfoconnect.py

+        comfoconnect._alarm_callback_fn = mock_callback
+
+        mock_alarm = Mock()
+        mock_alarm.swProgramVersion = 3222278145  # Firmware > 1.4.0


Same magic number appears here. This duplication reinforces the need for a shared constant to avoid inconsistency if this value needs to change.

Copilot · 2025-10-31T14:41:23Z

aiocomfoconnect/bridge.py

+
+            self._reference += 1
+
+        assert fut is not None


Using assert for runtime validation in production code can be problematic since assertions can be disabled with python -O. Consider replacing this with an explicit check and raising a RuntimeError or similar exception if fut is None.

Suggested change

assert fut is not None

if fut is None:

raise RuntimeError("Internal error: fut is None after send block")

Copilot · 2025-10-31T14:41:25Z

aiocomfoconnect/comfoconnect.py

-
-        await connected
+                    await self._reconnect_task
+                except asyncio.CancelledError:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except asyncio.CancelledError:

except asyncio.CancelledError:

# Task cancellation is expected during cleanup; ignore this exception.

Copilot · 2025-10-31T14:41:25Z

aiocomfoconnect/comfoconnect.py

+                self._reconnect_task.cancel()
+                try:
+                    await self._reconnect_task
+                except asyncio.CancelledError:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except asyncio.CancelledError:

except asyncio.CancelledError:

# Task was cancelled as part of cleanup; safe to ignore.

Copilot · 2025-10-31T14:41:25Z

aiocomfoconnect/comfoconnect.py

+            self._reconnect_task.cancel()
+            try:
+                await self._reconnect_task
+            except asyncio.CancelledError:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except asyncio.CancelledError:

except asyncio.CancelledError:

# Task cancellation is expected during disconnect; ignore this exception.

Copilot · 2025-10-31T14:41:26Z

tests/test_comfoconnect.py

+        with patch("asyncio.open_connection", side_effect=mock_open_connection):
+            try:
+                await comfoconnect.connect(LOCAL_UUID)
+            except AioComfoConnectTimeout:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except AioComfoConnectTimeout:

except AioComfoConnectTimeout:

# Exception is expected during this test; ignore it.

Copilot · 2025-10-31T14:41:26Z

tests/test_comfoconnect.py

+                    assert not connect_task.done()
+
+                    allow_start.set()
+                    await connect_task


This statement has no effect.

michaelarnauts added 2 commits October 31, 2025 15:35

Refactor pass one

99419ee

Refactor pass two

de839f7

michaelarnauts requested a review from Copilot October 31, 2025 14:38

Copilot AI reviewed Oct 31, 2025

View reviewed changes

Implement sending lock

83fb829

michaelarnauts changed the title ~~Refactor connection handling with the help of Copilot~~ Refactor connection handling Dec 22, 2025

michaelarnauts added 3 commits December 22, 2025 14:46

Codefix

2c93f00

Cleanup

24d4f9b

Remove instructions

9adcad0

michaelarnauts marked this pull request as ready for review December 22, 2025 13:57

michaelarnauts merged commit c281c33 into master Dec 22, 2025
4 checks passed

	self._alarm_callback_fn: Optional[Callable[[int, Dict[int, str]], None]] = alarm_callback
	self._alarm_callback_fn: Optional[Callable[[int, Optional[Dict[int, str]]], None]] = alarm_callback

-            # TODO(stability): revisit this once message pipelining is supported safely.
+            # TODO(stability): Currently, we use a lock to serialize outgoing messages because the underlying protocol and/or bridge device does not support concurrent (pipelined) requests safely.
+            # Concurrent writes may result in interleaved or corrupted messages, race conditions, or unexpected bridge behavior.
+            # Revisit this if the protocol is updated to support message pipelining, or if it can be demonstrated that concurrent requests are handled correctly by the bridge.

	assert fut is not None
	if fut is None:
	raise RuntimeError("Internal error: fut is None after send block")

	except asyncio.CancelledError:
	except asyncio.CancelledError:
	# Task cancellation is expected during cleanup; ignore this exception.

	except asyncio.CancelledError:
	except asyncio.CancelledError:
	# Task was cancelled as part of cleanup; safe to ignore.

	except asyncio.CancelledError:
	except asyncio.CancelledError:
	# Task cancellation is expected during disconnect; ignore this exception.

	except AioComfoConnectTimeout:
	except AioComfoConnectTimeout:
	# Exception is expected during this test; ignore it.

Uh oh!

Refactor connection handling #66

Refactor connection handling #66

Uh oh!

Conversation

michaelarnauts commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants