DiamondLightSource / DiamondLightSource/fastcs

Tutorial/demo TemperatureController.connect() never sets _connected so scan loop stays paused

Open
#372 1 comment 1 reaction 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.