DiamondLightSource / DiamondLightSource/fastcs
Tutorial/demo TemperatureController.connect() never sets _connected so scan loop stays paused
- Dominant language
- Python
- Stars
- 6
- Forks
- 8
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 3
Description
## What
`TemperatureController.connect` in [`src/fastcs/demo/controllers.py:96`](https://github.com/DiamondLightSource/fastcs/blob/main/src/fastcs/demo/controllers.py#L96) and the equivalent override in the final tutorial snippet [`docs/snippets/static15.py:93`](https://github.com/DiamondLightSource/fastcs/blob/main/docs/snippets/static15.py#L93) override the base class without calling `super().connect()` and without setting `self._connected = True`:
```python
async def connect(self) -> None:
await self.connection.connect(self._settings.ip_settings)
```
The base `Controller.connect` ([controllers/controller.py:67](https://github.com/DiamondLightSource/fastcs/blob/main/src/fastcs/controllers/controller.py#L67)) sets that flag, and the periodic scan coroutine gates on it ([controllers/controller.py:147](https://github.com/DiamondLightSource/fastcs/blob/main/src/fastcs/controllers/controller.py#L147)):
```python
async def scan_coro() -> None:
while True:
if not self._connected:
await asyncio.sleep(1)
continue
...
```
So after the override runs, `_connected` is still `False` (initialised in `Controller.__init__`, [line 24](https://github.com/DiamondLightSource/fastcs/blob/main/src/fastcs/controllers/controller.py#L24)) and the scan loop never enters its body. Result: every periodic `AttributeIO.update` and every `@scan(...)` method silently never fires. Writes (`attribute.put`) and `@command` methods still work because they bypass the scan loop.
The demo's `reconnect()` does set `_connected = True` ([line 107](https://github.com/DiamondLightSource/fastcs/blob/main/src/fastcs/demo/controllers.py#L107)), so once a transient failure forces a reconnect the polling starts working — but on a fresh run it never starts.
## How it was found
Surfaced while building a small demo project (`fastcs-demo`) that drives the tutorial controller against the bundled tickit simulator and asserts caput → caget round trips on read-back PVs. `caput MAIN:RampRate 7.5` reaches the simulator (\"Set ramp rate to 7.5\" is logged by tickit), but `caget MAIN:RampRate_RBV` keeps returning `0.0` because the read-back is only ever populated by the periodic update.
`tests/test_docs_snippets.py` doesn't catch this because it just `runpy.run_path`s each snippet and lets pytest-timeout kill the run — there's no assertion that an update loop has populated anything.
## Proposed fixes
Either of these would prevent the footgun; the first is the smaller change.
1. Have `FastCS.serve` flip `_connected = True` immediately after `await controller.connect()` succeeds (and reset it to `False` on failure). The `connect`/`reconnect` overrides then become \"connect this device, raise on failure\" without needing to know about the flag.
2. Keep the current contract but make the docstring on `Controller.connect` shout louder — and update both the tutorial snippet and `fastcs/demo/controllers.py` to call `super().connect()` so they're consistent with the rest of the codebase.
A regression test would also help: a doctest-or-equivalent that boots the demo controller, asserts a read-back PV picks up a value within an update cycle, would have caught this and would catch any future regression.
cc @coretl @gilesknap
Contributor guide
Assessment
This issue has not been assessed yet.