Add async support with context managers and improved error handling#3
Add async support with context managers and improved error handling#3TheFermiSea merged 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces asynchronous support with context managers and improved error handling to the elliptec-controller library. The implementation adds thread-based communication for non-blocking device operations while maintaining full backward compatibility with existing synchronous functionality.
Key changes include:
- Asynchronous communication via dedicated worker threads with per-command response queues
- Context manager support for automatic thread lifecycle management
- Enhanced error handling and recovery mechanisms for async scenarios
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
elliptec_controller/controller.py |
Major refactor adding async thread support, per-command queues, and context manager implementation |
tests/test_async.py |
Comprehensive test suite for async functionality including thread management and error scenarios |
examples/async_example.py |
Demonstration of async usage patterns with context managers and mixed operation modes |
examples/error_handling_example.py |
Examples showing robust error handling in both sync and async modes |
qudi-addon.md |
Integration guide for using the library with the Qudi measurement framework |
docs/quickstart.md |
Updated documentation with async examples and usage patterns |
docs/api.md |
Enhanced API reference with async method documentation |
README.md |
Updated with async features and usage examples |
CHANGELOG.md |
Detailed summary of all async-related changes and improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| if wait: | ||
| pass | ||
| else: | ||
| return True |
There was a problem hiding this comment.
Bug: Direct Assignment to Read-Only Property
The is_moving property, designed to dynamically check device status, is being directly assigned values in several methods. As a read-only property without a setter, these assignments will raise an AttributeError at runtime. Internal state updates should target _is_moving_state instead, which also resolves duplicate state management and logging.
Additional Locations (1)
| status = self.get_status() | ||
| if status == "00": | ||
| with self._command_lock: | ||
| self.is_moving = False |
There was a problem hiding this comment.
Bug: Duplicate Logic and Incorrect Property Assignment
The home() method contains duplicated logic for status checks and command lock usage, leading to redundant get_status() calls. Critically, it attempts to assign False to the read-only is_moving property, which will cause an AttributeError. This assignment should target _is_moving_state instead.
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| """Context manager exit.""" | ||
| self.disconnect() |
There was a problem hiding this comment.
Bug: Unreferenced Duplicate Controller Implementation
The async_example.py file appears to be an accidentally committed development artifact. It's an unreferenced, alternative ElliptecController implementation with its own threading system and a distinct API, duplicating existing functionality and potentially causing confusion.
Additional Locations (1)
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| """Context manager exit.""" | ||
| self.disconnect() |
There was a problem hiding this comment.
Bug: Duplicate and Incomplete Code in Commit
The controller_refactor.py file appears to be an accidentally committed development version of the ElliptecRotator class, duplicating the main implementation. It contains uncleaned code, including a redundant debug log statement in continuous_move, indicating it's not ready for the main codebase.
Summary
This PR introduces major new features and substantial improvements to the elliptec-controller library:
🚀 New Features
__enter__,__exit__) for automatic thread lifecycle management📁 Major Changes
elliptec_controller/controller.py: Large refactor to support both sync and async operation, with a robust thread-safe command queue and response handlingasync_example.py,error_handling_example.pyto demonstrate async supporttest_async.pyfor comprehensive async test coverageREADME.md,docs/api.md,docs/quickstart.md, and newqudi-addon.mdhighlight async usage🔄 Backward Compatibility
📊 File Changes
🧪 Testing
tests/test_async.py📚 Documentation
🔍 Review Focus Areas
controller.py📖 Related Files
CHANGELOG.md- Summary of all changestests/test_async.py- Async test coverageexamples/async_example.py- Async usage patternsexamples/error_handling_example.py- Error handling examplesqudi-addon.md- Integration guideThis PR significantly enhances the library's capabilities while maintaining full backward compatibility, making it suitable for both synchronous and asynchronous applications.