demo: getter/setter-in-init temperature attr example#411
Conversation
Add temperature_attr.py: a small temperature controller with per-attribute IO (a fresh AttributeIO/AttributeIORef pair per attribute) wired directly in __init__ rather than shared class-body declarations, foreshadowing the AttrRW(getter=, setter=) constructor params landing in #392. Baseline against the current callback-IO API. Closes #404
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #411 +/- ##
=========================================
Coverage 91.25% 91.25%
=========================================
Files 72 72
Lines 2892 2892
=========================================
Hits 2639 2639
Misses 253 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/demo/temperature_attr.py`:
- Around line 76-77: Update the async close method to reset the controller’s
_connected state to False after awaiting self.connection.close(), so subsequent
connection status checks correctly report the closed state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a64717b0-dfc5-439f-a5e0-0a883dacae7c
📒 Files selected for processing (2)
src/fastcs/demo/temperature_attr.pytests/demo/test_temperature_attr.py
| async def close(self) -> None: | ||
| await self.connection.close() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the controller connection state after closing.
Line 74 sets _connected to True, but close() never clears it. After the socket closes, the controller can still report itself as connected.
Proposed fix
async def close(self) -> None:
await self.connection.close()
+ self._connected = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def close(self) -> None: | |
| await self.connection.close() | |
| async def close(self) -> None: | |
| await self.connection.close() | |
| self._connected = False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fastcs/demo/temperature_attr.py` around lines 76 - 77, Update the async
close method to reset the controller’s _connected state to False after awaiting
self.connection.close(), so subsequent connection status checks correctly report
the closed state.
Address CodeRabbit review comment: close() closed the socket but left _connected True, so a subsequent status check would still report connected.
…ape) Rewrites the getter/setter baseline to the intended shape: one generic TemperatureIO drives every attribute, and each TemperatureIORef carries the command-building callables (read_cmd/write_cmd) sourced from a single TemperatureProtocol class - mirroring fastcs-thorlabs-mff's MFFAttributeIO/MFFAttributeIORef/ThorlabsAPTProtocol. This is the honest precursor to #392's AttrRW(getter=, setter=): read_cmd/ write_cmd ARE the getter/setter, promoted onto the constructor when the IO/ref wrapper is deleted, while TemperatureProtocol survives unchanged. Replaces the previous per-attribute AttributeIO subclasses (RampRateIO/PowerIO), which hardcoded commands and foreshadowed nothing. Response parsing (float()) is inline in TemperatureIO.update rather than a response_handler callable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
0f877a4 to
3e774b3
Compare
3 participants
Closes #404
Adds Tutorial 2 (getter/setter) from the demo README ladder:
src/fastcs/demo/temperature_attr.py, a small temperature controller (ramp_rate,power) wired attribute-by-attribute in__init__rather than via shared class-body declarations.Retargeted onto the temperature sim (per the issue, 2026-07-22 update) rather than a standalone Thorlabs MFF device, so the tutorial ladder keeps to two backends.
Current-API baseline in the intended shape: a single generic
TemperatureIOdrives every attribute, and eachTemperatureIORefcarries the command-building callables (read_cmd/write_cmd) sourced from a singleTemperatureProtocolclass — mirroringfastcs-thorlabs-mff(MFFAttributeIO/MFFAttributeIORef/ThorlabsAPTProtocol).read_cmd/write_cmdare the getter/setter this foreshadows: #392 promotes them ontoAttrRW(getter=..., setter=...)and deletes the IO/ref wrapper, whileTemperatureProtocolsurvives unchanged. Response parsing (float()) is inline inTemperatureIO.update, not aresponse_handlercallable. (Contrast the composition examplecontrollers.py, #390, whose shared IO dispatches on anamestring.)Instructions to reviewer on how to test:
uv run pytest tests/demo/test_temperature_attr.py -vChecks for reviewer
Notes
uv run --locked tox -e pre-commit,type-checking, both green. As on demo: use ControllerVector for temperature ramp sub-controllers #409/demo: cut-down Eiger REST sim + introspectable controller example #410, this sandbox can't run thedocsenv (no outbound network) or PVA-touching tests (no PVA-capable socket family here). Excluding those,pytest src tests --ignore=tests/benchmarkingpasses 310/320 with only the same known pre-existing PVA/p4pfailures, unrelated to this change. Real CI coversdocsand PVA.Generated by Claude Code
Summary by CodeRabbit
New Features
Tests