demo: use ControllerVector for temperature ramp sub-controllers - #409
demo: use ControllerVector for temperature ramp sub-controllers#409coretl wants to merge 2 commits into
Conversation
Evolve the demo temperature controller composition example onto the documented ControllerVector pattern instead of a manual list + add_sub_controller loop, and add unit tests exercising cancel_all and the voltage-distributing scan against a mocked IPConnection. Closes #390
|
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:
✨ 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 #409 +/- ##
=========================================
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:
|
Address review comment: pyright already infers ControllerVector[TemperatureRampController] from the dict literal, so the explicit annotation was redundant.
shihab-dls
left a comment
shihab-dls
left a comment
There was a problem hiding this comment.
Looks good, just left one comment on a test that I think should be implemented. Approving anyways as it is a nit.
|
|
||
| await controller.cancel_all() | ||
|
|
||
| sent_commands = [ | ||
| call.args[0] for call in controller.connection.send_command.call_args_list | ||
| ] | ||
| for index in controller.ramps: | ||
| assert f"N{index:02d}=0\r\n" in sent_commands |
There was a problem hiding this comment.
I think this should do the checks on the attributes, not on the call arguments to send_command; check that enabled on each ramp has been set to false. This could be achieved simply by mocking out enabled.put(), and checking that this was called with OnOffEnum.Off for all ramps.
3 participants
Closes #390
Evolves
src/fastcs/demo/controllers.py's composition example onto the documentedControllerVectorpattern (seedocs/explanations/controllers.md) instead of a manuallist+add_sub_controllerloop, and adds unit tests exercisingcancel_alland the voltage-distributing@scanagainst a mockedIPConnection.This is the current-API baseline described in the issue (
Float(...),io_ref=...); the getter/setter cleanup lands separately when #392 lands.Instructions to reviewer on how to test:
uv run pytest tests/demo/test_controllers.py -vpython -m fastcs.demo run src/fastcs/demo/fastcs.yaml)Checks for reviewer
Notes
uv run --locked tox -e pre-commit,type-checking,tests.pre-commitandtype-checkingpass. In the sandbox this ran in, thetestsenv's plainpytestcollection also picks updocs/conf.py(needs outbound network) andtests/benchmarking(needs a PVA-capable socket family), both of which fail to even collect for reasons unrelated to this change. Excluding those two paths,pytest src tests --ignore=tests/benchmarkingpasses 310/320 with only PVA (p4p) IOC tests failing onRuntimeError: Address family not supported by protocol— the same known sandbox limitation. Real CI covers these.Generated by Claude Code