diff --git a/docs/conf.py b/docs/conf.py index 4f74d36ee..f75dee477 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -91,6 +91,8 @@ ("py:class", "p4p.nt.NTTable"), # httpx and fastapi don't have intersphinx mappings ("py:class", "httpx.AsyncBaseTransport"), + ("py:class", "httpx.AsyncClient"), + ("py:class", "httpx.Response"), ("py:class", "fastapi.applications.FastAPI"), # Problems in FastCS itself ("py:class", "BaseController"), @@ -116,6 +118,8 @@ nitpick_ignore_regex = [ ("py:class", r"fastcs.*.DType_T"), ("py:class", r"fastcs.*.Numeric_T"), + ("py:class", r"fastcs.*.Connection_T"), + ("py:obj", r"fastcs.*.Connection_T"), ("py:obj", r"fastcs.*.DType_T"), (r"py:.*", r"fastcs\.demo.*"), (r"py:.*", r"tickit.*"), diff --git a/docs/explanations/connections.md b/docs/explanations/connections.md new file mode 100644 index 000000000..c38aa4c4d --- /dev/null +++ b/docs/explanations/connections.md @@ -0,0 +1,215 @@ +# Connections + +A `Connection` is a link to hardware, and it owns its own health state. Controllers +hold a connection; several controllers may hold the same one. + +Connections, not controllers, are the unit of failure and recovery. A tree of five +sub controllers behind one socket has one health state, one reconnect task and one +retry budget between them - not five of each, four of which can do nothing about the +link that is actually down. + +## Writing one + +Subclass `Connection`, open the link in `connect` and close it in `close`: + +```python +from fastcs.connections import Connection + + +class DetectorConnection(Connection): + # Class defaults sit between the framework defaults and any constructor argument. + reconnect_period = 5.0 + reconnect_attempts = 60 + + def __init__(self, settings: DetectorSettings, **kwargs) -> None: + super().__init__(**kwargs) + self._settings = settings + self._client: AsyncClient | None = None + + async def connect(self) -> None: + base = f"http://{self._settings.host}:{self._settings.port}" + self._client = AsyncClient(base_url=base) + + async def close(self) -> None: + if self._client is not None: + await self._client.aclose() + self._client = None + + async def get(self, path: str): + try: + response = await self._client.get(path) + except (ConnectError, ReadTimeout): + # The transport is gone. Everything holding this connection is now down. + self.set_disconnected() + raise + # A 400 from the detector is a device complaint, not a dead link - it + # propagates to the caller without touching connection state. + response.raise_for_status() + return response.json()["value"] +``` + +`connect` means "make the link usable", not merely "open the socket": a device that +needs a mode set before a driver can read it has that write here. + +### The ones that come with FastCS + +Most drivers need none of the above. `IPConnection` and `SerialConnection` cover the +stream transports; `HTTPConnection` covers REST devices, with `get`, `get_bytes`, +`put` and the `request` underneath them, so a driver that only needs a different URL +layout writes that and nothing else: + +```python +class DetectorConnection(HTTPConnection): + # The detector wraps every value as {"value": ...}, which is the detector's + # convention rather than HTTP's - so this is the whole subclass. + async def get(self, path: str): + return (await super().get(path))["value"] +``` + +`SimConnection` is the base for a simulated device: it opens and closes trivially, +can never fail, and its reconnect task idles forever. A simulator is a *sibling* of +the real transport rather than a subclass - inheriting `SerialConnection` would +inherit a serial handle it never opens - and which one an application gets is decided +by `type:` in `fastcs.yaml`, not by a magic port value or an environment check. + +**The important part is the `except` clause.** The connection is the only place that +can tell "the socket died" from "the device rejected that parameter", and only the +first is a connection failure. Nothing above a connection has to catch anything, and +no exception type is a contract between layers. + +**The framework sets the state; authors do the work and raise.** No driver touches a +connected flag: `connect` opens the link or raises, and the framework decides what +that means. The one thing a driver calls is `set_disconnected`, from its own IO. + +## Holding one + +A controller claims a connection by name from the `Connections` registry, which is +forwarded down the tree. Passing the registry rather than a bare connection means a +controller's constructor signature does not change when something three tiers below +it needs a new connection: + +```python +class DetectorController(Controller): + # Narrows the base class's connection so this controller's own code can call + # the methods of the connection it actually holds. + connection: DetectorConnection + + def __init__(self, connections: Connections) -> None: + # Claimed by name, type asserted. Raises at construction - before anything + # opens - if the name is missing or the type is wrong. + self.connection = connections.get("detector", DetectorConnection) + super().__init__() + + async def build(self) -> None: + # The connection is open by now, so this can ask the device what it has. + for parameter in await self.connection.get("detector/api/1.8.0/config/keys"): + ... # one attribute per reported key +``` + +A controller holds at most one connection - two devices means two controllers. A +controller with no connection at all (a soft controller that only groups others, or a +`ControllerVector`) is never gated and never reconnected. + +**No controller ever reads another controller's state.** A sub controller that shares +its parent's connection is not consulting its parent - it holds the same object. +Failure, gating and recovery all resolve through that shared object, never through +the tree. + +## Declaring them + +Every connection an application has is declared up front, which is what lets the +runner open them all before the tree is walked. The registry is built once and +forwarded down the tree - by hand, or by the launcher from the `connections:` block +of a controller's own entry in `fastcs.yaml`: + +```yaml +controllers: + - id: PITCH + type: fastcs_motor.MotorController + connections: + motor: + type: fastcs.SerialConnection + settings: {port: /dev/ttyS0} +``` + +Per entry rather than globally, because the key is the *role* the driver asks for and +the entry identifies the instance: two motors both claim `"motor"` and each resolves +to a different object. A single global block cannot express that, since the driver's +hardcoded role name and the deployment's instance name would have to be the same +string. + +A connection cannot be created later: one made during `build` could not have been +opened before the tree was walked, so it would never be supervised or reconnected, +and the runner rejects it saying so. + +The consequence of the per-entry block is that sibling entries cannot share a +connection or depend on each other. A gateway with several instruments behind one link is one tree, with the +gateway as the top-level controller. + +See [](../how-to/launch-framework.md) for the configuration in full. + +## Startup + +The `ControllerRunner` owns the order: + +1. Open every connection, in dependency order - declaration order, except that + anything named in a `depends_on` is opened before whatever names it. +2. Walk the tree calling `build`, repeating over anything newly added until a pass + adds nothing. +3. Call `setup` across the whole built tree. +4. Warn about anything suspicious, run the initial reads, and start the tasks. + +A failure anywhere in startup aborts. A partly built tree means an application with a +silently incomplete set of parameters, which is worse than no application at all, +because clients connect successfully and never find what they are looking for. The +orchestrator owns the retry. + +## Failure and recovery + +Failure is detected in exactly one place: the connection's own IO. `set_disconnected` +wakes that connection's reconnect task and gates every scan that uses it. + +There is one reconnect task per connection, idle until that connection actually goes +down - a healthy connection costs nothing, and each connection recovers at its own +pace. A detector that wants to retry every five seconds does not have to compromise +with a writer that wants one. + +Each attempt closes the link and reopens it. `reconnect_attempts` consecutive +failures is terminal until the process restarts; a clean connection restores the +budget. + +### Dependencies + +A connection layered over others declares them, rather than having them derived from +where controllers sit in the tree - one, or several: + +```python +odin = OdinConnection(settings, depends_on=detector) +motion = PmacMotionConnection(settings, depends_on=[ssh, status]) +``` + +All of them must be up: a connection layered over two links is no more usable with +one of them than with neither. While any is down, the dependent waits instead of +attempting - and because no attempt means no increment, its retry budget freezes +rather than being burnt against a dead dependency. If any one of them gives up +entirely, the dependent is released rather than left hanging: it logs that it is +stalled and waits for a restart. Cycles are caught at startup, and in config before +that. + +## Warnings + +- A connection declared but never claimed is warned about at startup: it would + otherwise be opened and reconnected forever while doing nothing. +- A connection with no polled attribute or scan method among any of its controllers + is warned about, phrased as fact rather than fault - all-on-demand is a legitimate + design, it just means nothing will detect the link failing until the next write. + +There is no separate health-check hook. A connection with any polling is proved alive +by that polling; a device that genuinely needs a heartbeat gets a `@scan` on one of +its controllers, which is ordinary driver code. + +## Shutdown + +Closing is a runner operation, not an author hook: every connection is closed in +reverse declaration order, so anything layered over another is closed before what it +rides on. `setup` is not undone - devices keep their last configured state. diff --git a/docs/explanations/controllers.md b/docs/explanations/controllers.md index b697fca1a..5a995995d 100644 --- a/docs/explanations/controllers.md +++ b/docs/explanations/controllers.md @@ -7,23 +7,38 @@ FastCS provides three controller classes: `Controller`, `ControllerVector`, and `Controller` is the primary building block for FastCS drivers. It can serve two roles: -**Root controller:** passed directly to the `FastCS` launcher. In this role, FastCS -will call its lifecycle hooks and run the scan tasks it creates on the event loop. +**Root controller:** passed directly to the `FastCS` launcher. **Sub controller:** attached to a parent controller via `add_sub_controller()` or by -assigning it as an attribute. In this role, the sub controller's lifecycle hooks -(`connect`, `reconnect`, `initialise`, `disconnect`) are not called automatically by -FastCS. The parent controller is responsible for calling them as part of its own -lifecycle, if required. +assigning it as an attribute. + +The `ControllerRunner` owns the order of the startup sequence and calls the +lifecycle hooks of **every** controller in the tree, root and sub alike. A parent +never drives a child's lifecycle to compensate for sequencing. ### Lifecycle hooks | Method | Purpose | |---|---| -| `initialise` | Fill declared attributes, and add dynamic ones, before the API is built | -| `connect` | Open connection to device | -| `reconnect` | Re-open connection after scan error | -| `disconnect` | Release device resources before shutdown | +| `__init__` | Everything knowable without the device: settings, static attributes | +| `build` | Structure that depends on the device - fill declared attributes, and add dynamic attributes and sub controllers | +| `setup` | Hardware writes and checks, once the whole tree is built | + +The same question, three ways: + +| What do I need to answer this? | Where it goes | +|---|---| +| Nothing - settings and the class | `__init__` | +| The device | `build` | +| My children, connected | `setup` | + +There is no `connect`, `reconnect` or `disconnect` hook. Opening the link, reopening +it after a failure and closing it at shutdown belong to the `Connection` and the +runner - see [connections](./connections.md). + +`build` runs with every connection already open, so a controller that has to ask the +device what it has - how many channels, which parameters - reads it there and creates +what it finds. Attributes are constructed in `__init__`, or declared as class-body type hints and created for you - see [](declaring-attributes.md) for which to use when. @@ -32,12 +47,14 @@ shared by every instance of the controller. ### Scan task behaviour -When used as the root controller, FastCS collects all `@scan` methods and readable -attributes whose `getter` is wrapped in `Polled`, across the whole controller -hierarchy, to be run as background tasks by FastCS. Scan tasks are gated on the -`_connected` flag: if a scan -raises an exception, `_connected` is set to `False` and tasks pause until `reconnect` -sets it back to `True`. +FastCS collects all `@scan` methods and readable attributes whose `getter` is wrapped +in `Polled`, across the whole controller hierarchy, to be run as background tasks. +Scan tasks are gated on the controller's **connection**: while that connection is +down they wait for it to come back rather than polling a link that cannot answer. A +controller with no connection is never gated. + +A scan that raises is logged and retried. It does not mark the connection down - +only the connection's own IO can tell a dead transport from a device complaint. ```python from fastcs.controllers import Controller @@ -46,62 +63,54 @@ from fastcs.methods import scan class TemperatureController(Controller): - def __init__(self, host, port): + connection: DeviceConnection + + def __init__(self, connections: Connections): + self.connection = connections.get("device", DeviceConnection) super().__init__() - self._host, self._port = host, port self.temperature = AttrR(float, units="degC") self.setpoint = AttrRW(float, units="degC") - async def connect(self): - self._client = await DeviceClient.connect(self._host, self._port) - self._connected = True - - async def reconnect(self): - try: - self._client = await DeviceClient.connect(self._host, self._port) - self._connected = True - except Exception: - logger.error("Failed to reconnect") - - async def disconnect(self): - await self._client.close() - @scan(period=1.0) async def update_temperature(self): - value = await self._client.get_temperature() + # Gated on the connection: while it is down this does not run at all. + value = await self.connection.get_temperature() await self.temperature.update(value) ``` ### Using Controller as a sub controller When a `Controller` is nested inside another, it organises the driver into logical -sections and its attributes are exposed under a prefixed path. If the sub -controller also has connection logic, the parent must invoke it explicitly: +sections and its attributes are exposed under a prefixed path. A sub controller that +talks to the same device holds the *same* connection object as its parent rather +than consulting it, so the two share one health state and one reconnect task: ```python class ChannelController(Controller): - def __init__(self): + connection: DeviceConnection + + def __init__(self, connection: DeviceConnection): + self.connection = connection super().__init__() - self.value = AttrR(float) - async def connect(self): - ... - self._connected = True + self.value = AttrR(float) class RootController(Controller): + connection: DeviceConnection + channel: ChannelController - def __init__(self): + def __init__(self, connections: Connections): + self.connection = connections.get("device", DeviceConnection) super().__init__() - self.channel = ChannelController() - - async def connect(self): - await self.channel.connect() - self._connected = True + self.channel = ChannelController(self.connection) ``` +A sub controller that talks to a *different* device claims its own connection by +name from the registry instead. Nothing is inferred from tree position. + ## ControllerVector `ControllerVector` is a convenience wrapper for a set of controllers of the same type, @@ -133,12 +142,6 @@ class RootController(Controller): {i: ChannelController() for i in range(num_channels)} ) - async def connect(self): - for channel in self.channels.values(): - await channel.connect() - - self._connected = True - async def update_all(self): for index, channel in self.channels.items(): value = await self._client.get_channel(index) @@ -158,7 +161,7 @@ Use `ControllerVector` when: - The device has a set of identical channels, axes, or modules identified by number - You need to iterate over sub controllers and perform the same action on each -- The number of instances may vary (e.g. determined at runtime during `initialise`) +- The number of instances may vary (e.g. determined at runtime during `build`) Use a plain `Controller` with named sub controllers when the sub controllers are distinct components with different types or roles. diff --git a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md index 4f67b0528..8a011dafb 100644 --- a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md +++ b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md @@ -118,3 +118,73 @@ surface referenced by decision 13 of #388. responsibility**. 4. **Who owns reconnect?** The runner owns the whole lifecycle, including reconnect. + +## Amendment: connections own reconnect (#422) + +Question 4 above says the runner owns reconnect, and stops there. It left the +*subject* of reconnect implicit, and the first implementation took it to be the +controller — which cannot work once controllers share a link, because +reconnecting a controller that does not own its connection is a no-op. + +The subject is the **connection**. Connection state moves off `Controller` onto a +first-class `Connection` object: controllers hold one, several may hold the same +one, and it owns its own health, reconnect task and retry budget. See +[connections](../connections.md) for the shape, and the design attached to +[issue #422](https://github.com/DiamondLightSource/fastcs/issues/422). + +What this amends: + +- **`Controller.connect`/`reconnect`/`disconnect`/`_connected` are removed.** A + driver never touches a connected flag; `Connection.connect()` opens the link or + raises, and the framework decides what that means. +- **`initialise`/`post_initialise` become `build`/`setup`**, splitting "structure + that depends on the device" from "hardware writes once the tree is built". + `build` runs with the connection open, so a controller that must ask the device + what it has does so there. +- **The runner owns the startup order** — connections, then `build` to a fixpoint, + then `setup`, then tasks — and shutdown, closing connections in reverse. + +Points the review of #420 asked to settle, and how they land: + +- **`connection._connected = True` from outside.** There is a framework-only + `_set_connected()` next to `set_disconnected()`, so the flag has one owner. +- **What "fatal" means.** Not `sys.exit`: an embedded FastCS must survive it. The + runner sets `fatal_error` (an `asyncio.Event`) and records `fatal_reason`; + `FastCS.serve` raises it, and an embedder observes it instead. +- **`check()` skipped while IO is succeeding.** Dropped. There is no separate + health-check hook: a connection with any polling is proved alive by that + polling, and a device that needs a heartbeat gets a `@scan`, which is ordinary + driver code. A connection nothing polls is warned about at startup. +- **`depends_on` cycles.** Declared rather than derived, and detected at startup. +- **`reconnect_attempts` exhausting to a terminal state.** Kept as the design specifies + (default 10), and *not* propagated to dependents — the parent's give-up message + names what it blocks. Whether the default should instead be retry-forever is + left open; it is one constant. + +## Amendment: no introspection, and one HTTP connection (#424 review) + +The amendment above made `Connection` generic in what `connect()` returned, handed +that value to `build`, and compared it on every reconnect. All of it is removed, +per the spec attached to [this review +comment](https://github.com/DiamondLightSource/fastcs/pull/424#issuecomment-5604015457). + +- **Introspection is a larger design problem than it looked**, and gets its own + issue rather than riding along here. A controller that must ask the device what it + has still does - in `build`, against an open connection - but the framework neither + carries the answer nor compares it. The unsolved part is a connection whose + *existence* depends on introspection: it cannot be declared in `fastcs.yaml`, + because it does not exist at config-parse time. +- **Every connection is declared up front**, so the runner's list is exactly what the + launcher built. No tree walking to collect connections, no deduplication by + identity, no second sweep after the build phase - and `ControllerRunner` requires + its connections argument rather than falling back to the tree. +- **`HTTPConnection` joins `IPConnection` and `SerialConnection`.** `fastcs-eiger` + and `fastcs-odin` each hand-roll an HTTP client, on different libraries; one + framework class replaces both. httpx, because it is already a FastCS dependency. +- **`SimConnection` is the base for a simulated device**, a sibling of the real + transports rather than a subclass of one, selected by `type:` in `fastcs.yaml`. +- **`max_attempts` becomes `reconnect_attempts`**, pairing with `reconnect_period`. + +The consequence for `fatal_error` is that nothing in the framework sets it: its one +producer was the introspection mismatch. The channel is kept, because the problem it +solves - a background task that cannot usefully raise - has not gone anywhere. diff --git a/docs/explanations/declaring-attributes.md b/docs/explanations/declaring-attributes.md index f99535f97..643d1dd6a 100644 --- a/docs/explanations/declaring-attributes.md +++ b/docs/explanations/declaring-attributes.md @@ -47,7 +47,7 @@ attribute from it: class OdinDetector(Controller): frames: AttrRW[int] - async def initialise(self) -> None: + async def build(self) -> None: for name, spec in await self._query_parameter_tree(): self.filler.fill_attribute( name, getter=spec.getter, setter=spec.setter, **spec.meta @@ -59,7 +59,7 @@ class OdinDetector(Controller): The hint is not a promise to build something later. `self.frames` **exists as soon as `__init__` returns** — as an `AttrRW[int]` with no IO yet — so the rest of `__init__` can reference it, hand it to a sibling, or subscribe to it. That -rule is what makes `initialise` safe to run in parallel across controllers: +rule is what makes `build` safe to run in parallel across controllers: only `__init__` is serial, and by the time it ends every attribute anything refers to is there. @@ -79,7 +79,7 @@ class EigerDetector(Controller): state: AttrR # enum built from the device's `allowed_values` ``` -FastCS cannot create that one, so it is a **promise** instead: introspection +FastCS cannot create that one, so it is a **promise** instead: `build` must add it with `add_attribute`, and `check_filled` fails if nothing did. The access mode is still checked — adding an `AttrW` where an `AttrR` was promised raises. @@ -104,8 +104,8 @@ own without FastCS knowing anything about it. `check_filled()` raises if anything the class body declared is missing, listing it by name. FastCS calls it across the whole controller tree after -`initialise`, so a driver that forgets cannot serve a half-built API; call it -yourself at the end of your own `initialise` to fail before anything else runs. +`build`, so a driver that forgets cannot serve a half-built API; call it +yourself at the end of your own `build` to fail before anything else runs. An `| None` hint is not required. ## Summary @@ -113,9 +113,9 @@ An `| None` hint is not required. | You know | Write | |---|---| | Everything about the attribute | `self.x = AttrRW(...)` in `__init__` | -| Its type, but not its IO or metadata | `x: AttrRW[int]` and fill it in `initialise` | -| Its access mode only | `x: AttrR` and `add_attribute` it in `initialise` | -| Nothing until the device answers | No declaration; `add_attribute` in `initialise` | +| Its type, but not its IO or metadata | `x: AttrRW[int]` and fill it in `build` | +| Its access mode only | `x: AttrR` and `add_attribute` it in `build` | +| Nothing until the device answers | No declaration; `add_attribute` in `build` | See [ADR 0013](decisions/0013-declarative-procedural-split-and-controller-filler.md) for why there is one declarative mechanism rather than two. diff --git a/docs/explanations/stable-interface.md b/docs/explanations/stable-interface.md index 52f9c4b21..1c551f4bb 100644 --- a/docs/explanations/stable-interface.md +++ b/docs/explanations/stable-interface.md @@ -14,30 +14,40 @@ transports, no interactive shell. `FastCS` is a caller of it. ```python from fastcs.controllers import ControllerRunner -runner = ControllerRunner(controller) -apis = await runner.setup() # initialise, and build the ControllerAPIs -await runner.start() # connect, run initial tasks, start scanning +runner = ControllerRunner(controller, connections=connections) +apis = await runner.build() # open connections, build the tree, build the APIs +await runner.start() # setup, run initial tasks, start scanning ... -await runner.stop() # stop the tasks, disconnect +await runner.stop() # stop the tasks, close the connections ``` -- **`setup()`** runs `initialise()` and `post_initialise()` on each controller - and builds their `ControllerAPI`s. It exists as a separate step because - anything serving the controllers has to register its callbacks *before* the - first values are read, or it misses them. -- **`start()`** connects the controllers, runs the initial (`ONCE`) tasks, and - starts the periodic ones. It runs `setup()` first if you have not, so an - embedder that does not need the APIs in between can just call `start()`. -- **`stop()`** cancels the tasks and disconnects. +- **`build()`** opens every declared `Connection`, walks the tree calling + `build()` on each controller, and builds their `ControllerAPI`s. It exists as + a separate step because anything serving the controllers has to register its + callbacks *before* the first values are read, or it misses them. +- **`start()`** runs `setup()` across the tree, the initial (`ONCE`) tasks, and + then starts the periodic ones and one reconnect task per connection. It runs + `build()` first if you have not, so an embedder that does not need the APIs in + between can just call `start()`. +- **`stop()`** cancels the tasks and closes every connection, in reverse + declaration order. **Idempotency is the caller's responsibility.** Starting a running runner, or stopping a stopped one, is not defined — an embedder whose own connect may run more than once has to keep track itself. -The runner also owns **reconnect**. A scan task whose callback raises marks its -controller disconnected and pauses rather than dying; the runner notices and -calls `Controller.reconnect()` until it comes back. This is deliberately not -left to each controller, so every controller recovers the same way. +The runner also owns **reconnect**, per connection rather than per controller: +see [connections](./connections.md). A connection's own IO marks it down when its +transport fails, and that connection's reconnect task brings it back at its own +pace. This is deliberately not left to each controller, so every connection +recovers the same way and controllers sharing one recover together. + +**A fatal runner condition is observable rather than fatal to the process.** +`runner.fatal_error` is an `asyncio.Event` set when the runner cannot carry on — +a device coming back from a reconnect describing itself differently, say — with +`runner.fatal_reason` carrying why. Nothing calls `sys.exit`, so an embedded +FastCS inside another process decides for itself what to do; `FastCS.serve` +raises it. ## Reading the structure: `ControllerAPI` diff --git a/docs/how-to/launch-framework.md b/docs/how-to/launch-framework.md index 7b9d811ee..984588156 100644 --- a/docs/how-to/launch-framework.md +++ b/docs/how-to/launch-framework.md @@ -128,6 +128,81 @@ the full set, and uses the per-entry id as the addressing prefix (EPICS PV prefix, REST route prefix, GraphQL top-level Query field, Tango device name segment). +### Declaring connections + +A controller's connections are declared in its own entry, under `connections:`. +Hand the `Connection` classes an entry may name to `launch` alongside the +controller classes; each is selected by the same dotted `type:` discriminator: + +```python +launch(MotorController, connection_classes=[SerialConnection]) +``` + +```yaml +# fastcs.yaml +controllers: + - id: PITCH + type: my_driver.MotorController + connections: + motor: + type: fastcs.SerialConnection + settings: + port: /dev/ttyS0 + - id: YAW + type: my_driver.MotorController + connections: + motor: + type: fastcs.SerialConnection + settings: + port: /dev/ttyS1 + reconnect_period: 5.0 + +transport: + - epicsca: {} +``` + +The key is the **role** the driver code asks for - `connections.get("motor", ...)` - +and the entry it sits in identifies the instance. Both motors above claim `"motor"` +and each resolves to a different object, which a single global block could not +express, because the driver's hardcoded role name and the deployment's instance name +would have to be the same string. + +One `Connections` registry is built per entry, from that entry's block, and passed +to a controller that takes a `connections` argument: + +```python +class MotorController(Controller): + def __init__(self, connections: Connections) -> None: + self.connection = connections.get("motor", SerialConnection) + super().__init__() +``` + +`connections` is a reserved name, in both the config and the signature: it does not +count towards the argument limit below, so a controller may take it *and* an options +object, and an options type may not declare a field called `connections`. + +Sibling entries therefore cannot share a connection or depend on each other. A +gateway device with several instruments behind one link is modelled as one tree, with +the gateway as the top-level controller. + +A connection layered over others names them by role, as one name or a list: + +```yaml +connections: + ssh: + type: my_driver.PmacSshConnection + settings: {ip: 192.168.0.9, port: 22} + status: + type: my_driver.StatusConnection + settings: {ip: 192.168.0.9} + motion: + type: my_driver.PmacMotionConnection + depends_on: [ssh, status] +``` + +Names resolve within the same entry. An unknown name is a config error listing that +entry's declared roles, and a cycle is rejected before anything is opened. + ## Schema Generation Generate JSON schema for the configuration yaml: @@ -204,7 +279,8 @@ FastCS: 0.12.0 The `launch()` function requires: -1. Controller `__init__` must have at most 2 arguments (including `self`) +1. Controller `__init__` must have at most 2 arguments (including `self`), not + counting a `connections` argument, which is reserved and always allowed 2. If a configuration argument exists, it must have a type hint Using a dataclass or Pydantic model is recommended for the configuration type, as it enables JSON schema generation. Other type-hinted types will work, but will not produce a useful schema. @@ -225,6 +301,11 @@ class BadController(Controller): def __init__(self, settings): # Error: no type hint super().__init__() +# Valid - `connections` does not count towards the limit +class ConnectedController(Controller): + def __init__(self, connections: Connections, settings: MySettings): + super().__init__() + # Invalid - too many arguments class TooManyArgs(Controller): def __init__(self, settings: MySettings, extra: str): # Error diff --git a/docs/how-to/update-attributes-from-device.md b/docs/how-to/update-attributes-from-device.md index f67dca33a..47391b209 100644 --- a/docs/how-to/update-attributes-from-device.md +++ b/docs/how-to/update-attributes-from-device.md @@ -165,8 +165,11 @@ class MultiChannelController(Controller): await channel.voltage.update(float(voltage)) ``` -The scan period (here `0.1` seconds) sets how often the batched query runs. Scans that -raise an exception will pause and wait for `reconnect()` to be called before resuming. +The scan period (here `0.1` seconds) sets how often the batched query runs. A scan that +raises is logged and tried again on the next period. If the failure was the connection +itself going down, the scan waits for the connection to come back up rather than +querying a dead link - the runner reopens it, and there is nothing for a driver to +call. See [](../explanations/connections.md). ### Scan as a cache for getters @@ -236,13 +239,13 @@ class SubscriptionController(Controller): super().__init__() self._client = subscription_client - async def connect(self): + async def setup(self): # Register an async callback that forwards updates into the attribute. + # `setup` runs once the whole tree is built and every connection is open. async def on_temperature_change(value: float) -> None: await self.temperature.update(value) await self._client.subscribe("temperature", on_temperature_change) - await super().connect() ``` If the library only supports synchronous callbacks, schedule the coroutine onto the diff --git a/docs/snippets/dynamic.py b/docs/snippets/dynamic.py index 4e7c00e72..011d0cfca 100644 --- a/docs/snippets/dynamic.py +++ b/docs/snippets/dynamic.py @@ -4,7 +4,7 @@ from pydantic import BaseModel, ConfigDict, ValidationError from fastcs.attributes import Attribute, AttrR, AttrRW -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.datatypes import DType from fastcs.launch import FastCS @@ -80,17 +80,23 @@ async def setter(value, command=command, dtype=datatype): class TemperatureRampController(Controller): + connection: IPConnection + def __init__( self, index: int, parameters: dict[str, TemperatureControllerParameter], protocol: TemperatureProtocol, + connection: IPConnection, ): self._parameters = parameters self._protocol = protocol + # The same connection the parent holds, so this controller's polled + # attributes pause with it while it is down. + self.connection = connection super().__init__(f"Ramp{index}") - async def initialise(self): + async def build(self): for name, attribute in create_attributes( self._parameters, self._protocol ).items(): @@ -98,20 +104,20 @@ async def initialise(self): class TemperatureController(Controller): + connection: IPConnection + def __init__(self, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + # Opening it, and reopening it after a failure, is the runner's job. + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() - async def connect(self): - await self._connection.connect(self._ip_settings) - - async def initialise(self): - await self.connect() - - api = json.loads((await self._connection.send_query("API?\r\n")).strip("\r\n")) + async def build(self): + # Runs with the connection already open. The ramp controllers added here get + # their own `build` called by the runner on a later pass, so there is no + # need - and no way - to drive their lifecycle from this one. + api = json.loads((await self.connection.send_query("API?\r\n")).strip("\r\n")) ramps_api = api.pop("Ramps") @@ -120,19 +126,19 @@ async def initialise(self): for idx, ramp_parameters in enumerate(ramps_api): ramp_controller = TemperatureRampController( - idx + 1, ramp_parameters, self._protocol + idx + 1, ramp_parameters, self._protocol, self.connection ) - await ramp_controller.initialise() self.add_sub_controller(f"Ramp{idx + 1:02d}", ramp_controller) - await self._connection.close() - epics_ca = EpicsCATransport() connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": diff --git a/docs/snippets/static06.py b/docs/snippets/static06.py index 98af3b41b..b93c75ce4 100644 --- a/docs/snippets/static06.py +++ b/docs/snippets/static06.py @@ -1,7 +1,7 @@ from pathlib import Path from fastcs.attributes import AttrR -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -9,16 +9,14 @@ class TemperatureController(Controller): + connection: IPConnection + device_id: AttrR[str] def __init__(self, settings: IPConnectionSettings): super().__init__() - self._ip_settings = settings - self._connection = IPConnection() - - async def connect(self): - await self._connection.connect(self._ip_settings) + self.connection = IPConnection(settings) gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") @@ -26,7 +24,10 @@ async def connect(self): connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": diff --git a/docs/snippets/static07.py b/docs/snippets/static07.py index 2aea3bb76..7b64a989d 100644 --- a/docs/snippets/static07.py +++ b/docs/snippets/static07.py @@ -1,7 +1,7 @@ from pathlib import Path from fastcs.attributes import AttrR, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -9,28 +9,29 @@ class TemperatureController(Controller): + connection: IPConnection + def __init__(self, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() + self.connection = IPConnection(settings) super().__init__() self.device_id = AttrR(str, getter=Polled(self._get_device_id, period=0.2)) async def _get_device_id(self) -> str: - response = await self._connection.send_query("ID?\r\n") + response = await self.connection.send_query("ID?\r\n") return response.strip("\r\n") - async def connect(self): - await self._connection.connect(self._ip_settings) - gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") epics_ca = EpicsCATransport(gui=gui_options) connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static08.py b/docs/snippets/static08.py index 95363380b..5d5d1ed80 100644 --- a/docs/snippets/static08.py +++ b/docs/snippets/static08.py @@ -2,7 +2,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -27,10 +27,11 @@ async def send_query(self, param: str, dtype: type[ValueT]) -> ValueT: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -43,16 +44,16 @@ async def _get_device_id(self) -> str: async def _get_power(self) -> float: return await self._protocol.send_query("P", float) - async def connect(self): - await self._connection.connect(self._ip_settings) - gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") epics_ca = EpicsCATransport(gui=gui_options) connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static09.py b/docs/snippets/static09.py index 10c07b334..0321548f2 100644 --- a/docs/snippets/static09.py +++ b/docs/snippets/static09.py @@ -2,7 +2,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -27,10 +27,11 @@ async def send_query(self, param: str, dtype: type[ValueT]) -> ValueT: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -54,16 +55,16 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") epics_ca = EpicsCATransport(gui=gui_options) connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static10.py b/docs/snippets/static10.py index e6ea3292d..ea333c089 100644 --- a/docs/snippets/static10.py +++ b/docs/snippets/static10.py @@ -2,7 +2,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -27,8 +27,11 @@ async def send_query(self, param: str, dtype: type[ValueT]) -> ValueT: class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -53,10 +56,11 @@ async def _set_end(self, value: int) -> None: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -70,7 +74,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -86,16 +90,16 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") epics_ca = EpicsCATransport(gui=gui_options) connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static11.py b/docs/snippets/static11.py index 222d0cdda..055a051f1 100644 --- a/docs/snippets/static11.py +++ b/docs/snippets/static11.py @@ -3,7 +3,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.transports.epics import EpicsGUIOptions @@ -33,8 +33,11 @@ class OnOffEnum(enum.StrEnum): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -70,10 +73,11 @@ async def _set_enabled(self, value: OnOffEnum) -> None: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -87,7 +91,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -103,16 +107,16 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") epics_ca = EpicsCATransport(gui=gui_options) connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static12.py b/docs/snippets/static12.py index db7ced42c..9cc2ca10c 100644 --- a/docs/snippets/static12.py +++ b/docs/snippets/static12.py @@ -4,7 +4,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.methods import scan @@ -35,8 +35,11 @@ class OnOffEnum(enum.StrEnum): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -81,10 +84,11 @@ async def _get_actual(self) -> float: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -98,7 +102,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -114,13 +118,10 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - @scan(0.1) async def update_voltages(self): voltages = json.loads( - (await self._connection.send_query("V?\r\n")).strip("\r\n") + (await self.connection.send_query("V?\r\n")).strip("\r\n") ) for index, controller in enumerate(self._ramp_controllers): await controller.voltage.update(float(voltages[index])) @@ -131,7 +132,10 @@ async def update_voltages(self): connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static13.py b/docs/snippets/static13.py index 420acd994..f5a00d988 100644 --- a/docs/snippets/static13.py +++ b/docs/snippets/static13.py @@ -5,7 +5,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.methods import command, scan @@ -36,8 +36,11 @@ class OnOffEnum(enum.StrEnum): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -82,10 +85,11 @@ async def _get_actual(self) -> float: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -99,7 +103,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -115,13 +119,10 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - @scan(0.1) async def update_voltages(self): voltages = json.loads( - (await self._connection.send_query("V?\r\n")).strip("\r\n") + (await self.connection.send_query("V?\r\n")).strip("\r\n") ) for index, controller in enumerate(self._ramp_controllers): await controller.voltage.update(float(voltages[index])) @@ -139,7 +140,10 @@ async def disable_all(self) -> None: connection_settings = IPConnectionSettings("localhost", 25565) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static14.py b/docs/snippets/static14.py index 9e25a6418..5d50214b1 100644 --- a/docs/snippets/static14.py +++ b/docs/snippets/static14.py @@ -5,7 +5,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.logging import configure_logging, logger @@ -40,8 +40,11 @@ class OnOffEnum(enum.StrEnum): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -86,10 +89,11 @@ async def _get_actual(self) -> float: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -103,7 +107,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -119,13 +123,10 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - @scan(0.1) async def update_voltages(self): voltages = json.loads( - (await self._connection.send_query("V?\r\n")).strip("\r\n") + (await self.connection.send_query("V?\r\n")).strip("\r\n") ) for index, controller in enumerate(self._ramp_controllers): await controller.voltage.update(float(voltages[index])) @@ -147,7 +148,10 @@ async def disable_all(self) -> None: logger.info("Configuring connection settings", connection_settings=connection_settings) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/snippets/static15.py b/docs/snippets/static15.py index ac1a1d0d9..9de005b65 100644 --- a/docs/snippets/static15.py +++ b/docs/snippets/static15.py @@ -5,7 +5,7 @@ from typing import TypeVar from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import Controller from fastcs.launch import FastCS from fastcs.logging import LogLevel, configure_logging, logger @@ -48,8 +48,11 @@ class OnOffEnum(enum.StrEnum): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, connection: IPConnection) -> None: suffix = f"{index:02d}" + self.connection = connection self._protocol = TemperatureProtocol(connection, suffix) super().__init__(f"Ramp{suffix}") @@ -94,10 +97,11 @@ async def _get_actual(self) -> float: class TemperatureController(Controller): + connection: IPConnection + def __init__(self, ramp_count: int, settings: IPConnectionSettings): - self._ip_settings = settings - self._connection = IPConnection() - self._protocol = TemperatureProtocol(self._connection) + self.connection = IPConnection(settings) + self._protocol = TemperatureProtocol(self.connection) super().__init__() @@ -111,7 +115,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings): self._ramp_controllers: list[TemperatureRampController] = [] for index in range(1, ramp_count + 1): - controller = TemperatureRampController(index, self._connection) + controller = TemperatureRampController(index, self.connection) self._ramp_controllers.append(controller) self.add_sub_controller(f"R{index}", controller) @@ -127,13 +131,10 @@ async def _get_ramp_rate(self) -> float: async def _set_ramp_rate(self, value: float) -> None: await self._protocol.send_command("R", value, float) - async def connect(self): - await self._connection.connect(self._ip_settings) - @scan(0.1) async def update_voltages(self): voltages = json.loads( - (await self._connection.send_query("V?\r\n")).strip("\r\n") + (await self.connection.send_query("V?\r\n")).strip("\r\n") ) for index, controller in enumerate(self._ramp_controllers): await controller.voltage.update(float(voltages[index])) @@ -155,7 +156,10 @@ async def disable_all(self) -> None: logger.info("Configuring connection settings", connection_settings=connection_settings) controller = TemperatureController(4, connection_settings) controller.set_path(["DEMO"]) -fastcs = FastCS(controller, [epics_ca]) +# Every connection an application runs is declared up front, so the runner can +# open them all before it walks the tree. +connections = Connections({"temperature": controller.connection}) +fastcs = FastCS(controller, [epics_ca], connections=connections) if __name__ == "__main__": fastcs.run() diff --git a/docs/tutorials/dynamic-drivers.md b/docs/tutorials/dynamic-drivers.md index ae55dc608..84a6552b8 100644 --- a/docs/tutorials/dynamic-drivers.md +++ b/docs/tutorials/dynamic-drivers.md @@ -31,14 +31,14 @@ or a description for the parameter. ## FastCS Initialisation -Specific `Controller` classes can optionally implement an async `initialise` method to +Specific `Controller` classes can optionally implement an async `build` method to perform any start up logic. The intention here is that the `__init__` method should be -minimal and the `initialise` method performs any long running calls, such as querying an +minimal and the `build` method performs any long running calls, such as querying an API, allowing FastCS to run these concurrently to reduce start times. Take the driver implementation from the previous tutorial and remove the statically defined `Attributes` and creation of sub controllers in `__init__`. Then -implement an `initialise` method to create these dynamically instead. +implement a `build` method to create these dynamically instead. Create a pydantic model to validate the response from the device @@ -56,12 +56,12 @@ construction time just like statically-declared ones do. :lines: 50-79 ::: -Update the controllers to not define attributes statically and implement initialise +Update the controllers to not define attributes statically and implement build methods to create these attributes dynamically, passing the shared `TemperatureProtocol` down to `create_attributes` so the dynamically-created getters/setters can use it. :::{literalinclude} /snippets/dynamic.py -:lines: 82-128 +:lines: 82-131 ::: TODO: Add `enabled` back in to `TemperatureRampController` and recreate `disable_all` to diff --git a/docs/tutorials/static-drivers.md b/docs/tutorials/static-drivers.md index 78ecf9eb5..cf77b9b34 100644 --- a/docs/tutorials/static-drivers.md +++ b/docs/tutorials/static-drivers.md @@ -151,9 +151,14 @@ its own getter/setter logic and connection, but there are some built in connecti options. Update the controller to create an `IPConnection` to communicate with the simulator over -TCP and implement a `connect` method that establishes the connection. The `connect` -method is called by the FastCS application at the appropriate time during start up to -ensure the connection is established before it is used. +TCP, giving it the settings it needs. A driver never opens the connection itself: FastCS +opens it at the appropriate time during start up, before anything uses it, and reopens +it if it drops. Declaring `connection: IPConnection` on the class narrows the base +class's connection so this controller's own code can call `IPConnection`'s methods. + +Every connection an application runs is declared up front, in a `Connections` registry +handed to `FastCS`, so the framework can open them all before it walks the controller +tree - a connection it has not been given is one it could never reopen. :::{note} The simulator control connection is on port 25565. @@ -163,7 +168,7 @@ The simulator control connection is on port 25565. :class: dropdown, hint :::{literalinclude} /snippets/static06.py -:emphasize-lines: 4,15-22,27-28 +:emphasize-lines: 4,12,19,25-26 ::: :::: @@ -189,7 +194,7 @@ Passing the getter bare, as here, means it is called once at start up. Wrap it i :class: dropdown, hint :::{literalinclude} /snippets/static07.py -:emphasize-lines: 13-19,21-23 +:emphasize-lines: 12,15,19,21-23 ::: :::: @@ -232,7 +237,7 @@ constructor to perform the cast. :class: dropdown, hint :::{literalinclude} /snippets/static08.py -:emphasize-lines: 12,15-27,34,38-39,41-45 +:emphasize-lines: 12,15-27,35,39-40,42-46 ::: :::: @@ -256,7 +261,7 @@ The set commands do not return a response, so the setter uses `send_command` ins :class: dropdown, hint :::{literalinclude} /snippets/static09.py -:emphasize-lines: 4,40-45,53-57 +:emphasize-lines: 4,41-46,54-57 ::: :::: @@ -297,11 +302,15 @@ Create a `TemperatureRampController` with two `AttrRW`s for the ramp start and e to define how many ramps there are, which is used to register the correct number of ramp controllers with the parent. +Each ramp holds the *same* `IPConnection` object as its parent rather than one of its +own, so the whole tree has one health state and one reconnect task between it: when the +link drops, every ramp's polling pauses with it and they all resume together. + ::::{admonition} Code 10 :class: dropdown, hint :::{literalinclude} /snippets/static10.py -:emphasize-lines: 30-53,57,73-77 +:emphasize-lines: 30,32-56,75-79 ::: :::: @@ -326,7 +335,7 @@ Add an `AttrRW` to the `TemperatureRampController`s with an `Enum` type, using a :class: dropdown, hint :::{literalinclude} /snippets/static11.py -:emphasize-lines: 1,31-33,48-53,67-71 +:emphasize-lines: 1,31-33,51-56,70-74 ::: :::: @@ -381,7 +390,7 @@ above. :class: dropdown, hint :::{literalinclude} /snippets/static12.py -:emphasize-lines: 11,56-58,78-82,123-129 +:emphasize-lines: 11,59-61,81-85,121-127 ::: :::: @@ -402,7 +411,7 @@ controller by calling `set` on each `enabled` attribute. :class: dropdown, hint :::{literalinclude} /snippets/static13.py -:emphasize-lines: 1,132-137 +:emphasize-lines: 1,133-138 ::: :::: @@ -434,7 +443,7 @@ inside `TemperatureProtocol.send_command` to log the commands it sends. :class: dropdown, hint :::{literalinclude} /snippets/static14.py -:emphasize-lines: 12,28,145,150 +:emphasize-lines: 12,28,146,154 ::: :::: @@ -462,7 +471,7 @@ is enabled the messages are visible. :class: dropdown, hint :::{literalinclude} /snippets/static15.py -:emphasize-lines: 12,14,21,34-36,41,125,153 +:emphasize-lines: 12,14,21,34-36,41,129,154 ::: :::: diff --git a/src/fastcs/connections/__init__.py b/src/fastcs/connections/__init__.py index 5001409a1..3fda02df0 100644 --- a/src/fastcs/connections/__init__.py +++ b/src/fastcs/connections/__init__.py @@ -1,5 +1,13 @@ +from .connection import DEFAULT_RECONNECT_ATTEMPTS as DEFAULT_RECONNECT_ATTEMPTS +from .connection import DEFAULT_RECONNECT_PERIOD as DEFAULT_RECONNECT_PERIOD +from .connection import Connection as Connection +from .http_connection import HTTPConnection as HTTPConnection +from .http_connection import HTTPConnectionSettings as HTTPConnectionSettings +from .ip_connection import DisconnectedError as DisconnectedError from .ip_connection import IPConnection as IPConnection from .ip_connection import IPConnectionSettings as IPConnectionSettings from .ip_connection import StreamConnection as StreamConnection +from .registry import Connections as Connections from .serial_connection import SerialConnection as SerialConnection from .serial_connection import SerialConnectionSettings as SerialConnectionSettings +from .sim_connection import SimConnection as SimConnection diff --git a/src/fastcs/connections/connection.py b/src/fastcs/connections/connection.py new file mode 100644 index 000000000..04e45658a --- /dev/null +++ b/src/fastcs/connections/connection.py @@ -0,0 +1,143 @@ +from __future__ import annotations + +import asyncio +from abc import ABC, abstractmethod +from collections.abc import Sequence + +DEFAULT_RECONNECT_PERIOD = 1.0 +"""Seconds a connection waits between reconnect attempts, unless it says otherwise.""" + +DEFAULT_RECONNECT_ATTEMPTS = 10 +"""Reconnect attempts a connection makes before giving up, unless it says otherwise.""" + + +def normalise_depends_on( + depends_on: Connection | Sequence[Connection] | None, +) -> list[Connection]: + """``depends_on`` as a list, whether it was given as one, several or nothing.""" + if depends_on is None: + return [] + if isinstance(depends_on, Connection): + return [depends_on] + return list(depends_on) + + +class Connection(ABC): + """A link to hardware. Owns its own health state. + + Several controllers may share one instance - a sub controller that talks to the + same device as its parent holds the same object rather than consulting the parent. + Failure, gating and recovery all resolve through that shared object, so a tree of + controllers behind one socket has one health state, one reconnect task and one + retry budget between them. + + A concrete connection opens the link in ``connect`` and closes it in ``close``, + and calls `set_disconnected` from its own IO when the *transport* fails. That is the + one place that can tell "the socket died" from "the device rejected that + parameter", and only the first is a connection failure:: + + async def get(self, path: str): + try: + response = await self._client.get(path) + except (ConnectError, ReadTimeout): + self.set_disconnected() # transport is gone + raise + response.raise_for_status() # a device complaint, not a dead link + return response.json()["value"] + + Nothing above a connection has to catch anything, and no exception type is a + contract between layers. + + The `ControllerRunner` keys its per-connection state by identity, so a + ``Connection`` must never define ``__eq__``: two sockets with matching settings + are two connections, and an ``__eq__`` would silently collapse them. + + Args: + depends_on: The connection(s) this one is layered over, if any - one, or a + sequence of them. Declared, never derived: the runner will not attempt + this one until *every* one of them is up, and stalls it if any gives up. + reconnect_period: Seconds between reconnect attempts. Defaults to the class + attribute of the same name. + reconnect_attempts: Consecutive failed attempts before this connection gives + up. Defaults to the class attribute of the same name. + + """ + + # Class defaults. Framework defaults below, class attributes on a concrete + # connection, constructor arguments on top - three tiers, each overriding the last. + reconnect_period: float = DEFAULT_RECONNECT_PERIOD + reconnect_attempts: int = DEFAULT_RECONNECT_ATTEMPTS + + def __init__( + self, + depends_on: Connection | Sequence[Connection] | None = None, + reconnect_period: float | None = None, + reconnect_attempts: int | None = None, + ) -> None: + self._connected = False + self._up = asyncio.Event() + self._down = asyncio.Event() + self._down.set() + + # Declared, never derived. A connection layered over others names them here; + # the runner will not attempt this one until all of them are up. Always a + # list, so the runner has one shape to handle rather than three. + self.depends_on: list[Connection] = normalise_depends_on(depends_on) + + if reconnect_period is not None: + self.reconnect_period = reconnect_period + if reconnect_attempts is not None: + self.reconnect_attempts = reconnect_attempts + + @property + def connected(self) -> bool: + """Whether the link is currently believed to be usable. + + Set by the framework - a driver never writes it. ``connect`` returning cleanly + marks it up; `set_disconnected` from the connection's own IO marks it down. + """ + return self._connected + + @abstractmethod + async def connect(self) -> None: + """Open the link, or raise. + + This means "make the link usable", not merely "open the socket" - a device + that needs a mode set before a driver can talk to it has that write here, + rather than in a controller's ``build``. + + The framework marks the connection connected when this returns cleanly. + """ + + @abstractmethod + async def close(self) -> None: + """Close the link. Called at shutdown and before every reconnect attempt. + + Must tolerate being called on a link that is already closed. + """ + + def set_disconnected(self) -> None: + """Called by the connection's own IO when its transport fails. + + Wakes this connection's reconnect task and gates every scan that uses it. + """ + self._connected = False + self._up.clear() + self._down.set() + + def _set_connected(self) -> None: + """Framework only. Wakes anything awaiting this connection's recovery.""" + self._connected = True + self._down.clear() + self._up.set() + + async def wait_up(self) -> None: + """Block until this connection is up. Returns immediately if it already is.""" + await self._up.wait() + + async def wait_down(self) -> None: + """Block until this connection is down. Returns immediately if it already is.""" + await self._down.wait() + + def __repr__(self) -> str: + return f"{type(self).__name__}(connected={self._connected})" diff --git a/src/fastcs/connections/http_connection.py b/src/fastcs/connections/http_connection.py new file mode 100644 index 000000000..addb7dc62 --- /dev/null +++ b/src/fastcs/connections/http_connection.py @@ -0,0 +1,113 @@ +from dataclasses import dataclass, field +from typing import Any + +from httpx import AsyncBaseTransport, AsyncClient, ConnectError, ReadTimeout, Response + +from fastcs.connections.connection import Connection +from fastcs.connections.ip_connection import DisconnectedError + + +@dataclass +class HTTPConnectionSettings: + host: str = "127.0.0.1" + port: int = 80 + scheme: str = "http" + timeout: float = 10.0 + headers: dict[str, str] = field(default_factory=dict) + + @property + def base_url(self) -> str: + return f"{self.scheme}://{self.host}:{self.port}" + + +class HTTPConnection(Connection): + """An HTTP connection. + + The settings are given at construction rather than to ``connect``, because the + framework opens and reopens the link without knowing anything about it. IO marks + the connection down when the *transport* fails, so everything holding it stops + and its reconnect task wakes. + + One framework class rather than one per driver: every REST device does the same + few things, and a driver needing a verb or a response shape this does not cover + uses `request` rather than rolling its own client. + + Args: + settings: Where to connect to + kwargs: Passed to `Connection` - ``depends_on``, ``reconnect_period``, + ``reconnect_attempts`` + + """ + + def __init__( + self, settings: HTTPConnectionSettings | None = None, **kwargs + ) -> None: + super().__init__(**kwargs) + self._settings = settings or HTTPConnectionSettings() + + self._transport: AsyncBaseTransport | None = None + """Set by a subclass to talk to an in-process ASGI app instead of a socket. + + Not a constructor argument: a connection's ``__init__`` signature is its + config schema, and a transport object is not config. It is how a driver + tests against its own simulator without a network. + """ + + self.__client: AsyncClient | None = None + + @property + def _client(self) -> AsyncClient: + if self.__client is None: + raise DisconnectedError( + "Need to call connect() before using HTTPConnection." + ) + + return self.__client + + async def connect(self) -> None: + self.__client = AsyncClient( + base_url=self._settings.base_url, + timeout=self._settings.timeout, + headers=self._settings.headers, + transport=self._transport, + ) + + async def close(self) -> None: + if self.__client is None: + return + + await self.__client.aclose() + self.__client = None + + async def get(self, path: str) -> Any: + """GET, returning the parsed JSON body.""" + return (await self.request("GET", path)).json() + + async def get_bytes(self, path: str) -> bytes: + """GET, returning the raw body. For frame and file data.""" + return (await self.request("GET", path)).content + + async def put(self, path: str, value: Any) -> Any: + """PUT a JSON value, returning the parsed JSON body if there is one.""" + response = await self.request("PUT", path, json=value) + return response.json() if response.content else None + + async def request(self, method: str, path: str, **kwargs) -> Response: + """Every request goes through here. + + The only method that touches connection state, so overriding `get` or `put` + does not silently change the others, and one failure cannot mark the + connection down twice. + """ + try: + response = await self._client.request(method, path, **kwargs) + except (ConnectError, ReadTimeout, OSError): + # The socket is gone, rather than the device complaining. Everything + # holding this connection is now down. + self.set_disconnected() + raise + + # A 4xx or 5xx is a device complaint - it propagates to the caller without + # touching connection state. + response.raise_for_status() + return response diff --git a/src/fastcs/connections/ip_connection.py b/src/fastcs/connections/ip_connection.py index f021aa913..97f0617cb 100644 --- a/src/fastcs/connections/ip_connection.py +++ b/src/fastcs/connections/ip_connection.py @@ -1,11 +1,16 @@ import asyncio from dataclasses import dataclass +from fastcs.connections.connection import Connection from fastcs.tracer import Tracer -class DisconnectedError(Exception): - """Raised if the ip connection is disconnected.""" +class DisconnectedError(ConnectionError): + """Raised if the ip connection is disconnected. + + A `ConnectionError`, and so an `OSError`, because that is what the rest of this + module treats as "the transport is gone" rather than "the device complained". + """ pass @@ -46,12 +51,26 @@ async def close(self): await self.writer.wait_closed() -class IPConnection(Tracer): - """For connecting to an ip using a `StreamConnection`.""" +class IPConnection(Connection, Tracer): + """For connecting to an ip using a `StreamConnection`. + + The settings are given at construction rather than to ``connect``, because the + framework opens and reopens the link without knowing anything about it. IO + marks the connection down when the *transport* fails, so everything holding it + stops and its reconnect task wakes. + + Args: + settings: Where to connect to + kwargs: Passed to `Connection` - ``depends_on``, ``reconnect_period``, + ``reconnect_attempts`` + + """ - def __init__(self): - super().__init__() - self.__connection = None + def __init__(self, settings: IPConnectionSettings | None = None, **kwargs) -> None: + Connection.__init__(self, **kwargs) + Tracer.__init__(self) + self._settings = settings or IPConnectionSettings() + self.__connection: StreamConnection | None = None @property def _connection(self) -> StreamConnection: @@ -60,18 +79,40 @@ def _connection(self) -> StreamConnection: return self.__connection - async def connect(self, settings: IPConnectionSettings): - reader, writer = await asyncio.open_connection(settings.ip, settings.port) + async def connect(self) -> None: + reader, writer = await asyncio.open_connection( + self._settings.ip, self._settings.port + ) self.__connection = StreamConnection(reader, writer) async def send_command(self, message: str) -> None: async with self._connection as connection: - await connection.send_message(message) + try: + await connection.send_message(message) + except OSError: + # The socket is gone, rather than the device complaining. Everything + # holding this connection is now down. + self.set_disconnected() + raise async def send_query(self, message: str) -> str: async with self._connection as connection: - await connection.send_message(message) - response = await connection.receive_response() + try: + await connection.send_message(message) + response = await connection.receive_response() + if not response: + # ``readline`` returns b"" at EOF, so a peer that closed the + # socket rather than answering looks like an empty reply. It + # is a dead link, and nothing else here would notice: the + # caller would get "" and fail to parse it, over and over, + # while the reconnect task stayed idle. + raise DisconnectedError( + "Connection closed by peer while awaiting a response" + ) + except OSError: + self.set_disconnected() + raise + self.log_event( "Received query response", query=message.strip(), @@ -79,7 +120,7 @@ async def send_query(self, message: str) -> str: ) return response - async def close(self): + async def close(self) -> None: if self.__connection is None: return diff --git a/src/fastcs/connections/registry.py b/src/fastcs/connections/registry.py new file mode 100644 index 000000000..cc5de2838 --- /dev/null +++ b/src/fastcs/connections/registry.py @@ -0,0 +1,100 @@ +from __future__ import annotations + +from typing import TypeVar + +from fastcs.connections.connection import Connection + +Connection_T = TypeVar("Connection_T", bound=Connection) + + +class Connections: + """The connections available to a controller tree, claimed by name. + + Built once - by the launcher from the ``connections:`` block, or by hand - and + forwarded down the tree. A controller claims what it needs with `get` rather than + receiving a bare connection object, so a controller's constructor signature does + not change when something three tiers below it needs a new connection:: + + class EigerController(Controller): + def __init__(self, connections: Connections) -> None: + super().__init__() + self.add_sub_controller("DET", EigerDetectorController(connections)) + self.add_sub_controller("OD", OdinController(connections)) + + class EigerDetectorController(Controller): + def __init__(self, connections: Connections) -> None: + self.connection = connections.get("eiger", EigerConnection) + super().__init__() + + Args: + connections: The declared connections, keyed by the name controllers claim + them under. Iteration order is declaration order, which is the order the + runner opens them in. + + """ + + def __init__(self, connections: dict[str, Connection]) -> None: + self._connections = dict(connections) + self._claimed: set[str] = set() + + def get(self, name: str, expected: type[Connection_T]) -> Connection_T: + """Claim a connection by name, asserting its type. + + Called from ``__init__``, so a bad name or type fails at construction - + before anything is opened - rather than at the first IO. + + Args: + name: The name the connection was declared under + expected: The `Connection` subclass the caller intends to use + + Returns: + The declared connection + + Raises: + KeyError: If nothing was declared under that name + TypeError: If what was declared is not an ``expected`` + + """ + try: + connection = self._connections[name] + except KeyError: + raise KeyError( + f"No connection named {name!r}. Declared: {sorted(self._connections)}" + ) from None + + if not isinstance(connection, expected): + raise TypeError( + f"Connection {name!r} is {type(connection).__name__}, " + f"but {expected.__name__} was expected" + ) + + self._claimed.add(name) + return connection + + def unclaimed(self) -> set[str]: + """Names declared but never claimed. + + A config typo that would otherwise be opened and reconnected forever while + doing nothing, so the runner warns about it at startup. + """ + return set(self._connections) - self._claimed + + def name_of(self, connection: Connection) -> str | None: + """The name a connection was declared under, by identity.""" + for name, declared in self._connections.items(): + if declared is connection: + return name + return None + + def values(self) -> list[Connection]: + """The declared connections, in declaration order.""" + return list(self._connections.values()) + + def __contains__(self, name: object) -> bool: + return name in self._connections + + def __len__(self) -> int: + return len(self._connections) + + def __repr__(self) -> str: + return f"Connections({sorted(self._connections)})" diff --git a/src/fastcs/connections/serial_connection.py b/src/fastcs/connections/serial_connection.py index 65bbf6801..2c9a66cfe 100644 --- a/src/fastcs/connections/serial_connection.py +++ b/src/fastcs/connections/serial_connection.py @@ -3,6 +3,8 @@ import aioserial +from fastcs.connections.connection import Connection + class NotOpenedError(Exception): """If the serial stream is not opened.""" @@ -16,15 +18,29 @@ class SerialConnectionSettings: baud: int = 115200 -class SerialConnection: - """A serial connection.""" +class SerialConnection(Connection): + """A serial connection. + + The settings are given at construction rather than to ``connect``, because the + framework opens and reopens the link without knowing anything about it. + + Args: + settings: Which port to open, and at what baud rate + kwargs: Passed to `Connection` - ``depends_on``, ``reconnect_period``, + ``reconnect_attempts`` - def __init__(self): - self.stream = None + """ + + def __init__(self, settings: SerialConnectionSettings, **kwargs) -> None: + super().__init__(**kwargs) + self._settings = settings self._lock = asyncio.Lock() + self.__stream: aioserial.AioSerial | None = None - async def connect(self, settings: SerialConnectionSettings) -> None: - self.__stream = aioserial.AioSerial(port=settings.port, baudrate=settings.baud) + async def connect(self) -> None: + self.__stream = aioserial.AioSerial( + port=self._settings.port, baudrate=self._settings.baud + ) @property def _stream(self) -> aioserial.AioSerial: @@ -45,12 +61,24 @@ async def send_query(self, message: bytes, response_size: int) -> bytes: return await self._receive_response(response_size) async def _send_message(self, message): - await self._stream.write_async(message) + try: + await self._stream.write_async(message) + except (OSError, aioserial.SerialException): + # The port is gone, rather than the device complaining. + self.set_disconnected() + raise async def _receive_response(self, size): - return await self._stream.read_async(size) + try: + return await self._stream.read_async(size) + except (OSError, aioserial.SerialException): + self.set_disconnected() + raise async def close(self) -> None: async with self._lock: - self._stream.close() + if self.__stream is None: + return + + self.__stream.close() self.__stream = None diff --git a/src/fastcs/connections/sim_connection.py b/src/fastcs/connections/sim_connection.py new file mode 100644 index 000000000..3c9730cf4 --- /dev/null +++ b/src/fastcs/connections/sim_connection.py @@ -0,0 +1,35 @@ +from fastcs.connections.connection import Connection +from fastcs.logging import logger + + +class SimConnection(Connection): + """Base for simulated connections. + + Opens and closes trivially and can never fail, so it is connected for the life of + the process and its reconnect task idles forever. Subclasses implement whatever IO + methods their driver calls, and never call ``set_disconnected()`` - there is no + transport that can go away. + + A sibling of the real transports rather than a subclass of one: a simulator that + inherits `IPConnection` also inherits a stream handle it will never open and a + property that raises. Inheriting this instead means the only thing a driver + writes is the pretending:: + + class SimSerialConnection(SimConnection): + def __init__(self, **kwargs) -> None: + super().__init__(**kwargs) + self._position = 0 + + async def send_query(self, message: bytes, response_size: int) -> bytes: + ... # canned responses + + Which one an application gets is decided by ``type:`` in ``fastcs.yaml``, not by + a magic port value or an environment check, so real and simulated hardware are + interchangeable in config with no change to driver code. + """ + + async def connect(self) -> None: + logger.info("[SIM] Connected", connection=type(self).__name__) + + async def close(self) -> None: + logger.info("[SIM] Disconnected", connection=type(self).__name__) diff --git a/src/fastcs/control_system.py b/src/fastcs/control_system.py index 44467fe63..f4666679e 100644 --- a/src/fastcs/control_system.py +++ b/src/fastcs/control_system.py @@ -7,6 +7,7 @@ from IPython.terminal.embed import InteractiveShellEmbed +from fastcs.connections import Connections from fastcs.controllers import Controller, ControllerAPI, ControllerRunner from fastcs.logging import logger from fastcs.tracer import Tracer @@ -38,6 +39,10 @@ class FastCS: either a single ``Controller`` or a sequence of them. transports: A list of transports to serve the API over loop: Optional event loop to run the control system in + connections: The declared connections - one `Connections` registry, or one + per top-level controller entry, since role names are local to an entry. + These are the connections the runner opens and reconnects; a tree of + purely soft controllers declares none. """ def __init__( @@ -45,6 +50,7 @@ def __init__( controllers: Controller | Sequence[Controller], transports: Sequence[Transport], loop: asyncio.AbstractEventLoop | None = None, + connections: Connections | Sequence[Connections] | None = None, ): if isinstance(controllers, Controller): controllers = [controllers] @@ -61,7 +67,9 @@ def __init__( self._transports = transports self._loop = loop or asyncio.get_event_loop() - self._runner = ControllerRunner(self._controllers, self._loop) + if connections is None: + connections = [] + self._runner = ControllerRunner(self._controllers, connections, self._loop) self.controller_apis: list[ControllerAPI] = [] def run(self, interactive: bool = True): @@ -95,10 +103,34 @@ async def serve(self, interactive: bool = True) -> None: interactive: Whether to create an interactive IPython shell """ + try: + coros = await self._start(interactive) + except BaseException: + # A failure during startup aborts: a partly built application is worse + # than none, because clients connect successfully and never find what + # they are looking for. Close whatever was opened on the way up, then + # let the caller - or the orchestrator - see why. + await self._runner.stop() + raise + + try: + await asyncio.gather(*coros) + except asyncio.CancelledError: + pass + except Exception: + logger.exception("Unhandled exception in serve") + finally: + logger.info("Shutting down FastCS") + await self._runner.stop() + if self._runner.fatal_reason is not None: + raise self._runner.fatal_reason + + async def _start(self, interactive: bool) -> list[Coroutine]: + """Bring the application up, and return what ``serve`` should await.""" # Build the APIs before wiring transports to them: a transport # registers its callbacks when it connects, and would miss the first # readback if the controllers had already started. - self.controller_apis = await self._runner.setup() + self.controller_apis = await self._runner.build() context = { "controllers": {_context_key(c): c for c in self._controllers}, @@ -143,15 +175,20 @@ async def block_forever(): await self._runner.start() - try: - await asyncio.gather(*coros) - except asyncio.CancelledError: - pass - except Exception: - logger.exception("Unhandled exception in serve") - finally: - logger.info("Shutting down FastCS") - await self._runner.stop() + # A fatal runner condition - a device coming back describing itself + # differently, say - happens in a background task, where a raise would be + # invisible. The runner records it instead, and this coroutine is where the + # process notices and comes down rather than serving a tree that no longer + # matches the hardware. Nothing calls ``sys.exit``, so an embedder sees an + # exception out of ``serve`` rather than losing its process. + async def fail_on_fatal() -> None: + await self._runner.fatal_error.wait() + assert self._runner.fatal_reason is not None + raise self._runner.fatal_reason + + coros.append(fail_on_fatal()) + + return coros async def _interactive_shell(self, context: dict[str, Any]): """Spawn interactive shell in another thread and wait for it to complete.""" diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index 162a68a61..0c9d601fb 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -1,7 +1,7 @@ from __future__ import annotations from inspect import getattr_static -from typing import TypeVar +from typing import Any, TypeVar from fastcs.attributes import Attribute, UnboundAttr from fastcs.controllers.controller_api import ControllerAPI @@ -30,6 +30,29 @@ class BaseController(Tracer): root_attribute: Attribute | None = None description: str | None = None + connection: Any = None + """The link this controller does its IO over, if it has one. + + A `Connection`, or ``None``. Set in ``__init__``, usually by claiming it from a + `Connections` registry. A controller holds at most one - two devices means two + controllers - and several controllers may hold the same object, in which case + they share one health state, one reconnect task and one retry budget. + + A controller with no connection (a soft grouping controller, or a + `ControllerVector`) is never gated and never reconnected. + + Typed ``Any`` rather than ``Connection | None`` so that a driver can narrow it to + the connection it actually holds, and call that connection's own methods:: + + class TemperatureController(Controller): + connection: IPConnection + + A mutable attribute is invariant, so a driver cannot narrow a declared + ``Connection | None`` without a type checker objecting to every driver in + existence. The framework reads this attribute in exactly two places - the scan + gate and the runner - both of which state the type they expect. + """ + def __init__( self, path: list[str] | None = None, @@ -151,19 +174,41 @@ def __setattr__(self, name, value): else: super().__setattr__(name, value) - async def initialise(self): - """Hook for subclasses to dynamically add attributes before building the API""" + async def build(self): + """Hook for structure that depends on the device. + + Called by the framework once this controller's connection is open, and + before anything is set up. Add the attributes and sub controllers that could + only be known by asking the device - the ones knowable without it belong in + ``__init__``, which is where a controller can be constructed and inspected in + a test with no hardware. + + The connection is open by the time this runs, so a controller that has to + ask the device what it has - how many channels, which parameters - reads it + here and creates what it finds. + + No hardware *writes* here. A device that needs a mode set before it can be + read has that write in `Connection.connect`, which means "make the link + usable" rather than merely "open the socket". + """ pass - def post_initialise(self): - """Hook to call after all attributes added, before serving the application""" - self.check_filled() + async def setup(self): + """Hook for hardware writes and checks, once the whole tree is built. + + Called by the framework after every controller's ``build`` has run and every + connection is open, so this can read and write across the tree. + + No new attributes or sub controllers here - anything created now would never + get its own ``build`` or ``setup`` called. + """ + pass def check_filled(self): """Check that every class-body declaration was provisioned, recursively. A driver may call ``self.filler.check_filled()`` itself at the end of - its own ``initialise``; the framework calls this afterwards so that a + its own ``build``; the framework calls this afterwards so that a controller which forgot to does not serve a half-built API. """ self.filler.check_filled() diff --git a/src/fastcs/controllers/controller.py b/src/fastcs/controllers/controller.py index 0bee8d7d8..781a5438e 100755 --- a/src/fastcs/controllers/controller.py +++ b/src/fastcs/controllers/controller.py @@ -3,6 +3,7 @@ from collections.abc import Sequence from fastcs.attributes.attr_r import AttrR +from fastcs.connections import Connection from fastcs.controllers.base_controller import BaseController from fastcs.controllers.controller_api import ControllerAPI from fastcs.logging import logger @@ -18,7 +19,6 @@ def __init__( description: str | None = None, ) -> None: super().__init__(description=description) - self._connected = False def add_sub_controller(self, name: str, sub_controller: BaseController): if name.isdigit(): @@ -30,37 +30,15 @@ def add_sub_controller(self, name: str, sub_controller: BaseController): @property def connected(self) -> bool: - """Whether the controller believes it can talk to its device. + """Whether this controller can talk to its device. - Set by `connect`/`reconnect`, and cleared when a scan task raises. The - `ControllerRunner` reads it to decide when to reconnect. + A read-through to ``self.connection.connected`` - the connection is the one + object that knows, and controllers sharing a connection all report the same + value, which is correct because they share one link. A controller with no + connection has nothing to read through to and is always ``True``. """ - return self._connected - - async def connect(self) -> None: - """Hook to perform initial connection to device - - This should set ``_connected`` to ``True`` if the connection was successful to - enable scan tasks. - - """ - self._connected = True - - async def reconnect(self): - """Hook to reconnect to device after an error - - This should set ``_connected`` to ``True`` if the connection was successful to - enable scan tasks. - - If the connection cannot be re-established it should log an error with the - reason. It should not raise an exception. - - """ - self._connected = True - - async def disconnect(self) -> None: - """Hook to tidy up resources before stopping the application""" - pass + connection: Connection | None = self.connection + return connection is None or connection.connected def create_api_and_tasks( self, @@ -113,10 +91,15 @@ def _create_periodic_scan_coro( ) -> ScanCallback: """Create a coroutine to run scans at a given period - This returns a coroutine that runs scans at a given period. If an exception is - raised in a callback it is caught and the updates for the controller are - paused, waiting for `_connected` to be set back to true via the `reconnect` - method. + The returned coroutine is gated on this controller's connection: while that + connection is down it waits for the connection to come back rather than + polling a link that cannot answer. A controller with no connection is never + gated. + + The gate is the only thing a scan does about connection health. Failure is + detected in exactly one place - the connection's own IO, which knows a dead + transport from a device complaint - so a raising scan is logged and retried + rather than being read as a disconnection here. Args: period: The period to run the scans at @@ -128,8 +111,9 @@ def _create_periodic_scan_coro( async def scan_coro() -> None: while True: - if not self._connected: - await asyncio.sleep(1) + connection: Connection | None = self.connection + if connection is not None and not connection.connected: + await connection.wait_up() continue try: @@ -138,9 +122,8 @@ async def scan_coro() -> None: ) except Exception: logger.exception("Exception in scan task", period=period) - self._connected = False - - await asyncio.sleep(1) # Wait so this message appears last - logger.error("Pausing scan tasks and waiting for reconnect") + # Do not spin: a scan that raises immediately would otherwise + # retry as fast as the event loop allows. + await asyncio.sleep(period) return scan_coro diff --git a/src/fastcs/controllers/filler.py b/src/fastcs/controllers/filler.py index 3a53495ad..88d57d628 100644 --- a/src/fastcs/controllers/filler.py +++ b/src/fastcs/controllers/filler.py @@ -13,11 +13,11 @@ class OdinDetector(Controller): frames: AttrRW[int] # exists as soon as __init__ returns - async def initialise(self) -> None: + async def build(self) -> None: self.filler.fill_attribute("frames", getter=..., setter=...) so ``self.frames`` can be referenced by the rest of ``__init__`` - the rule -ADR 0013 takes from ophyd-async, and what makes ``initialise`` safe to run in +ADR 0013 takes from ophyd-async, and what makes ``build`` safe to run in parallel across controllers. Filling provisions the IO and metadata on the attribute that is already there, so a reference taken during ``__init__`` stays valid. @@ -25,7 +25,7 @@ async def initialise(self) -> None: A hint that cannot say what it holds - ``state: AttrR``, where the datatype is an enum whose members only exist on the wire - is a **promise** instead: it is not created, and `ControllerFiller.check_filled` requires that introspection -added it by the time the controller is initialised. +added it by the time the controller is built. """ from __future__ import annotations diff --git a/src/fastcs/controllers/runner.py b/src/fastcs/controllers/runner.py index ead653e9e..3e543fd7e 100644 --- a/src/fastcs/controllers/runner.py +++ b/src/fastcs/controllers/runner.py @@ -1,40 +1,80 @@ import asyncio -from collections.abc import Sequence +from collections import deque +from collections.abc import Iterator, Sequence +from dataclasses import dataclass, field +from fastcs.connections import Connection, Connections +from fastcs.controllers.base_controller import BaseController from fastcs.controllers.controller import Controller from fastcs.controllers.controller_api import ControllerAPI from fastcs.logging import logger from fastcs.methods import ScanCallback +from fastcs.util import ONCE -RECONNECT_PERIOD = 1.0 -"""Seconds between checks for a controller that has dropped its connection""" +MAX_BUILD_PASSES = 32 +"""Passes the build phase makes before deciding the tree is not settling. + +A ``build`` that adds a sub controller whose ``build`` adds another needs one pass +per tier; a cap catches runaway construction rather than hanging. +""" + + +@dataclass +class _ReconnectState: + """What the runner remembers about one connection.""" + + attempts: int = 0 + """Consecutive failed attempts. Reset by a clean connection.""" + + exhausted: asyncio.Event = field(default_factory=asyncio.Event) + """Set when this connection has given up. Terminal until the process restarts. + + An `asyncio.Event` rather than a flag because dependents await it: setting it + releases anything waiting on this connection, so they stall loudly instead of + hanging silently. + """ class ControllerRunner: """Runs one or more `Controller` s, without serving them anywhere. - This owns the whole controller lifecycle - initialising, connecting, - running the initial and periodic tasks, reconnecting after a failure, and - tidying up - and nothing about how the controllers are presented. `FastCS` - uses it and adds transports on top; an embedded caller that only wants the - controllers running can use it on its own:: + This owns the whole controller lifecycle - opening connections, building and + setting up the tree, running the initial and periodic tasks, reconnecting after a + failure, and tidying up - and nothing about how the controllers are presented. + `FastCS` uses it and adds transports on top; an embedded caller that only wants + the controllers running can use it on its own:: - runner = ControllerRunner(controller) + runner = ControllerRunner(controller, connections) await runner.start() ... await runner.stop() - Starting has two halves, because anything serving the controllers needs - their `ControllerAPI` before the first values are read: ``setup`` initialises - them and builds the APIs, and ``start`` connects and starts the tasks. - Calling ``start`` on its own does both. + **The runner owns the order of the startup sequence.** Every connection is opened + first, then the tree is walked calling ``build``, then ``setup`` runs across the + whole built tree, then the tasks start. Controllers never call their own hooks to + compensate for sequencing. + + Starting is in two halves, because anything serving the controllers needs their + `ControllerAPI` before the first values are read: ``build`` opens the connections, + builds the tree and returns the APIs, and ``start`` does the rest. Calling + ``start`` on its own does both. + + **A failure anywhere in startup aborts.** A partly built tree means an + application with a silently incomplete set of parameters, which is worse than no + application at all, because clients connect successfully and never find what they + are looking for. The orchestrator owns the retry. - **Idempotency is the caller's responsibility.** Starting a running runner, - or stopping a stopped one, is not defined. + **Idempotency is the caller's responsibility.** Starting a running runner, or + stopping a stopped one, is not defined. Args: controllers: The controller(s) to run. Accepts either a single ``Controller`` or a sequence of them. + connections: The declared connections - one `Connections` registry, or one + per top-level entry, since role names are local to an entry. Required, + and the whole list: every connection is declared up front, so the runner + never looks in the tree for one. A tree with no connections at all + passes an empty registry. loop: Optional event loop to create the tasks in """ @@ -42,36 +82,88 @@ class ControllerRunner: def __init__( self, controllers: Controller | Sequence[Controller], + connections: Connections | Sequence[Connections], loop: asyncio.AbstractEventLoop | None = None, ) -> None: if isinstance(controllers, Controller): controllers = [controllers] self._controllers: list[Controller] = list(controllers) self._loop = loop + if isinstance(connections, Connections): + connections = [connections] + self._registries: list[Connections] = list(connections) + + self._connections: list[Connection] = [] + self._state: dict[Connection, _ReconnectState] = {} self._controller_apis: list[ControllerAPI] = [] self._scan_coros: list[ScanCallback] = [] self._initial_coros: list[ScanCallback] = [] self._tasks: set[asyncio.Task] = set() + self.fatal_error: asyncio.Event = asyncio.Event() + """Set when the runner has hit something it cannot carry on from. + + A background task cannot usefully raise - nothing is awaiting it - and an + embedded FastCS must not call ``sys.exit``, so a fatal condition is reported + here instead. `FastCS` awaits it and shuts down; an embedder can do the same, + and read `fatal_reason` for what happened. + + Nothing in the framework sets this today: its one producer was the + introspection mismatch on reconnect, which went with introspection itself. + The channel is kept because the problem it solves - a background task that + cannot raise - has not gone anywhere. + """ + + self.fatal_reason: BaseException | None = None + """Why `fatal_error` was set, if it was.""" + @property def controller_apis(self) -> list[ControllerAPI]: - """The API of each controller. Empty until ``setup`` has run.""" + """The API of each controller. Empty until ``build`` has run.""" return self._controller_apis - async def setup(self) -> list[ControllerAPI]: - """Initialise the controllers and build their APIs. + @property + def connections(self) -> list[Connection]: + """The connections this runner supervises, in the order it opens them.""" + return list(self._connections) + + async def build(self) -> list[ControllerAPI]: + """Open every connection, build the controller tree and create the APIs. - Runs before anything connects, so that a transport can be wired to the - APIs and catch the first readback. + Runs before anything is set up or scanned, so that a transport can be wired + to the APIs and catch the first readback. Returns: The API of each controller, in the order they were given """ + try: + return await self._open_and_build() + except BaseException: + # Startup aborts, but the connections opened before the failure are + # still open, and no task exists yet for a later ``stop`` to be called + # to cancel - so nothing else would ever close them. + await self._close_connections() + raise + + async def _open_and_build(self) -> list[ControllerAPI]: + self._connections = self._collect_connections() + self._check_dependencies() + # Only safe once the cycle check above has passed. + self._connections = self._in_dependency_order(self._connections) + + for connection in self._connections: + self._state[connection] = _ReconnectState() + await connection.connect() + connection._set_connected() # noqa: SLF001 + + await self._build_phase() + for controller in self._controllers: - await controller.initialise() - controller.post_initialise() + # Every class-body declaration must have been provisioned by now: the + # build walk is finished, so nothing else is going to fill one in. + controller.check_filled() self._controller_apis = [] self._scan_coros = [] @@ -85,15 +177,27 @@ async def setup(self) -> list[ControllerAPI]: return self._controller_apis async def start(self) -> None: - """Connect the controllers and start their tasks. + """Set the tree up and start its tasks. - Runs ``setup`` first if it has not already run. + Runs ``build`` first if it has not already run. """ if not self._controller_apis: - await self.setup() + await self.build() - for controller in self._controllers: - await controller.connect() + try: + await self._setup_and_run() + except BaseException: + # As in ``build``: a ``setup`` or an initial read that raises leaves + # every connection open with nothing to close them. + await self.stop() + raise + + async def _setup_and_run(self) -> None: + for controller in self._walk_controllers(): + await controller.setup() + + self._warn_about_unclaimed_connections() + self._warn_about_unpolled_connections() for coro in self._initial_coros: await coro() @@ -101,41 +205,350 @@ async def start(self) -> None: loop = self._loop or asyncio.get_event_loop() self._tasks = {loop.create_task(coro()) for coro in self._scan_coros} self._tasks |= { - loop.create_task(self._reconnect_loop(controller)) - for controller in self._controllers + loop.create_task(self._reconnect_loop(connection)) + for connection in self._connections } async def stop(self) -> None: - """Stop the tasks and disconnect the controllers.""" + """Stop the tasks and close every connection. + + Shutdown is a runner operation rather than an author hook: connections are + closed in reverse declaration order, so anything layered over another is + closed before what it rides on. ``setup`` is not undone - devices keep their + last configured state. + """ self._cancel_tasks() + await self._close_connections() - for controller in self._controllers: + async def _close_connections(self) -> None: + for connection in reversed(self._connections): try: - await controller.disconnect() + await connection.close() except Exception: - logger.exception( - "Exception during disconnect", controller=controller.path + logger.exception("Exception while closing connection") + + # Startup + + def _collect_connections(self) -> list[Connection]: + """Every connection the runner supervises, in the order it opens them. + + The declared ones, in declaration order, and nothing else. They are known + before any controller is constructed, which is what lets a ``build`` add a + sub controller holding an already-open connection - and what makes the + list exact: a connection created later could not have been opened up front, + so there is nothing to find by walking the tree. + """ + return [ + connection + for registry in self._registries + for connection in registry.values() + ] + + def _check_dependencies(self) -> None: + """``depends_on`` is declared, so it can name anything at all. + + A connection can name one the runner does not supervise, or two can name + each other. Either leaves a connection waiting forever with nothing said, + so both fail at startup instead. + """ + for connection in self._connections: + self._check_dependencies_of(connection, [connection]) + + def _check_dependencies_of( + self, connection: Connection, path: list[Connection] + ) -> None: + """Depth-first over one connection's dependencies, carrying the path. + + A connection may name several, so the walk branches; ``path`` is the chain + that got here, which is both how a cycle is spotted and what names it. + """ + for dependency in connection.depends_on: + if not self._supervises(dependency): + # It would never be opened, so it would sit at + # ``connected is False`` forever and this connection would + # never be attempted again. + raise ValueError( + f"{type(connection).__name__} depends on a " + f"{type(dependency).__name__} the runner does not " + "supervise, so it would never be opened. Declare it " + "alongside the connection that depends on it." + ) + if any(dependency is node for node in path): + chain = " -> ".join(type(node).__name__ for node in path) + raise ValueError( + f"Cycle in connection dependencies: {chain} -> " + f"{type(dependency).__name__}" ) + self._check_dependencies_of(dependency, [*path, dependency]) - async def _reconnect_loop(self, controller: Controller) -> None: - """Bring a controller back after its scan tasks hit an error. + @staticmethod + def _in_dependency_order(connections: list[Connection]) -> list[Connection]: + """Declaration order, except that a dependency comes before its dependent. - A scan task that raises marks its controller disconnected and pauses - rather than dying, so something has to try to bring it back. That is the - runner's job rather than the controller's, so that every controller - reconnects the same way whether or not its author thought about it. + The initial open is sequential, so a connection layered over another must + not be opened first - and ``depends_on`` need not follow the order they were + declared in. Shutdown walks this list backwards, which closes a dependent + before what it rides on for the same reason. + + Assumes the dependency graph is acyclic - `_check_dependencies` has run. """ - while True: - await asyncio.sleep(RECONNECT_PERIOD) + ordered: list[Connection] = [] - if controller.connected: + def visit(connection: Connection) -> None: + if any(connection is done for done in ordered): + return + for dependency in connection.depends_on: + visit(dependency) + ordered.append(connection) + + for connection in connections: + visit(connection) + return ordered + + def _supervises(self, connection: Connection) -> bool: + """Whether this runner opened, and will reconnect, a connection.""" + return any(connection is known for known in self._connections) + + async def _build_phase(self) -> None: + """Walk the tree top-down calling ``build``, to a fixpoint. + + A ``build`` may add sub controllers, which need building themselves, so the + walk repeats over anything newly added until a pass adds nothing. + """ + built: set[int] = set() + + for _ in range(MAX_BUILD_PASSES): + pending = [c for c in self._walk_controllers() if id(c) not in built] + if not pending: + self._check_connections_are_known() + return + + for controller in pending: + built.add(id(controller)) + await controller.build() + + raise RuntimeError( + f"Controller tree did not settle in {MAX_BUILD_PASSES} build passes. " + "A `build` that adds a sub controller on every pass never finishes." + ) + + def _check_connections_are_known(self) -> None: + """A connection the runner never opened would never be reconnected either.""" + for controller in self._walk_controllers(): + connection: Connection | None = controller.connection + if connection is None or connection in self._state: continue - logger.info("Attempting to reconnect", controller=controller.path) - try: - await controller.reconnect() - except Exception: - logger.exception("Reconnect failed", controller=controller.path) + raise self._unsupervised_connection_error(controller, connection) + + @staticmethod + def _unsupervised_connection_error( + controller: BaseController, connection: Connection + ) -> RuntimeError: + return RuntimeError( + f"Controller {'.'.join(controller.path) or type(controller).__name__} " + f"holds a {type(connection).__name__} the runner did not open. A " + "connection created during `build` cannot be supervised - declare it " + "up front and claim it from the `Connections` registry." + ) + + def _warn_about_unclaimed_connections(self) -> None: + for registry in self._registries: + for name in sorted(registry.unclaimed()): + self._warn_unclaimed(name) + + @staticmethod + def _warn_unclaimed(name: str) -> None: + logger.warning( + "Connection declared but never used. It will be opened and " + "reconnected forever while doing nothing.", + connection=name, + ) + + def _warn_about_unpolled_connections(self) -> None: + """Nothing detects a connection failing unless something uses it regularly. + + Phrased as fact rather than fault: an all-on-demand device is a legitimate + design, it just will not notice a failure until the next write. + """ + polled: set[int] = set() + for controller in self._walk_controllers(): + connection: Connection | None = controller.connection + if connection is None: + continue + if self._has_polling(controller): + polled.add(id(connection)) + + for connection in self._connections: + if id(connection) in polled: + continue + + logger.warning( + "Connection has no polled attribute or scan method among its " + "controllers, so nothing will detect it failing until the next " + "write. It will not reconnect automatically.", + connection=self._name_of(connection), + ) + + @staticmethod + def _has_polling(controller: BaseController) -> bool: + from fastcs.attributes.attr_r import AttrR + + for method in controller.scan_methods.values(): + if method.period is not ONCE: + return True + + for attribute in controller.attributes.values(): + if not (isinstance(attribute, AttrR) and attribute.has_getter()): + continue + if attribute.poll_period is not ONCE and attribute.poll_period is not None: + return True + + return False + + # Failure and recovery + + async def _reconnect_loop(self, connection: Connection) -> None: + """Keep one connection alive, at its own pace. + + One task per connection, idle until that connection actually goes down - a + healthy connection costs nothing, and a detector that wants to retry every + five seconds does not have to compromise with a writer that wants one. + """ + state = self._state[connection] + + while True: + await connection.wait_down() + + if state.exhausted.is_set(): + return + + # If anything we ride on is down, wait for it rather than attempting. No + # attempt means no increment, so the retry budget freezes while waiting. + # All of them must be up: a connection layered over two links is no more + # usable with one of them than with neither. + down = [ + dependency + for dependency in connection.depends_on + if not dependency.connected + ] + if down: + logger.info( + "Waiting on dependencies", + connection=self._name_of(connection), + dependencies=[self._name_of(d) for d in down], + ) + await self._await_dependencies(down) + + stalled = [d for d in down if not d.connected] + if stalled: + # A dependency gave up. This connection cannot succeed, but it + # is not itself exhausted - it has spent nothing. Say so, then + # wait; only a restart will change anything. + logger.error( + "Stalled: dependency gave up", + connection=self._name_of(connection), + dependencies=[self._name_of(d) for d in stalled], + ) + return + + await self._attempt(connection) + + if not connection.connected and not state.exhausted.is_set(): + await asyncio.sleep(connection.reconnect_period) + + async def _await_dependencies(self, dependencies: list[Connection]) -> None: + """Block until every dependency is back, or any one of them gives up. + + Waiting on recovery alone would hang forever once a dependency exhausts, so + both outcomes are awaited and whichever lands first wins. Recovery is *all* + of them - a gather - while exhaustion is any single one, because one that has + given up is enough to make this connection unusable. + """ + + async def all_up() -> None: + await asyncio.gather(*(dependency.wait_up() for dependency in dependencies)) + + recovered = asyncio.create_task(all_up()) + gave_up = [ + asyncio.create_task(self._state[dependency].exhausted.wait()) + for dependency in dependencies + ] + + _, pending = await asyncio.wait( + {recovered, *gave_up}, return_when=asyncio.FIRST_COMPLETED + ) + for task in pending: + task.cancel() + + async def _attempt(self, connection: Connection) -> None: + """One reconnect attempt. + + Owns retry accounting, and is the only place a connection is marked back up. + """ + state = self._state[connection] + state.attempts += 1 + + try: + await connection.close() # tolerate an already-closed link + await connection.connect() + except Exception: + logger.exception("Reconnect failed", connection=self._name_of(connection)) + if state.attempts >= connection.reconnect_attempts: + # Terminal until the process restarts. Setting the event releases + # anything waiting on this connection, so dependents stall loudly + # instead of hanging silently. + state.exhausted.set() + logger.error( + "Giving up", + connection=self._name_of(connection), + attempts=state.attempts, + blocks=[ + self._name_of(dependent) + for dependent in self._dependents_of(connection) + ], + ) + return + + connection._set_connected() # noqa: SLF001 + state.attempts = 0 # a clean connection restores the budget + + def fail(self, error: BaseException) -> None: + """Report a condition the runner cannot carry on from. + + Raising here would be invisible - this runs in a background task with nothing + awaiting it - and an embedded FastCS must not call ``sys.exit``, so the + failure is recorded and whatever is running the runner decides what to do. + """ + if self.fatal_reason is None: + self.fatal_reason = error + self.fatal_error.set() + + def _dependents_of(self, connection: Connection) -> list[Connection]: + return [ + other + for other in self._connections + # identity: declared, not derived + if any(dependency is connection for dependency in other.depends_on) + ] + + # Helpers + + def _name_of(self, connection: Connection) -> str: + """What to call a connection in a log line.""" + for registry in self._registries: + name = registry.name_of(connection) + if name is not None: + return name + return type(connection).__name__ + + def _walk_controllers(self) -> Iterator[BaseController]: + """Every controller in the tree, level order.""" + queue: deque[BaseController] = deque(self._controllers) + while queue: + controller = queue.popleft() + yield controller + queue.extend(controller.sub_controllers.values()) def _cancel_tasks(self) -> None: # ``Task.cancel`` does not raise - it returns whether the task was diff --git a/src/fastcs/demo/README.md b/src/fastcs/demo/README.md index b6e5db9db..2c01f72fb 100644 --- a/src/fastcs/demo/README.md +++ b/src/fastcs/demo/README.md @@ -63,7 +63,10 @@ Notes: the lesson — "small & known → declare; large & self-describing → introspect" — and the REST sim also exercises an HTTP client backend the temp examples never touch, matching real downstream drivers (`fastcs-eiger`, `fastcs-secop`, - PandABlocks). + PandABlocks). The walk happens in `EigerDetector.build`, against an open + connection; `EigerConnection` is a plain `HTTPConnection` subclass that knows + the detector's URL layout and its `{"value": ...}` envelope, and nothing about + what the detector turns out to have. ## Baselines vs framework PRs diff --git a/src/fastcs/demo/__main__.py b/src/fastcs/demo/__main__.py index 467be4f81..525b586e3 100644 --- a/src/fastcs/demo/__main__.py +++ b/src/fastcs/demo/__main__.py @@ -1,6 +1,7 @@ from fastcs import __version__ +from fastcs.connections import IPConnection from fastcs.launch import launch from .temperature_attr import TemperatureController -launch(TemperatureController, version=__version__) +launch(TemperatureController, version=__version__, connection_classes=[IPConnection]) diff --git a/src/fastcs/demo/eiger.py b/src/fastcs/demo/eiger.py index afebc0c7c..eb9117fce 100644 --- a/src/fastcs/demo/eiger.py +++ b/src/fastcs/demo/eiger.py @@ -1,21 +1,24 @@ -"""Example 5 - introspectable controller: a cut-down Eiger over the fake REST sim. +"""Example 5 - dynamic controller: a cut-down Eiger over the fake REST sim. Half the attributes (``count_time``, ``state``) are declared as type hints and -checked by the current ``HintedAttribute`` introspection-validation mechanism; the -rest of the parameter tree is discovered at ``initialise()`` time by walking the -sim's ``keys`` endpoints and is added dynamically, with no static check. A device -that describes itself over the wire is exactly the case where introspection earns -its complexity - contrast with the (deliberately non-introspectable) SCPI/temperature -examples. +checked by the filler; the rest of the parameter tree is discovered by walking the +sim's ``keys`` endpoints in ``build`` and is added dynamically, with no static +check. A device that describes itself over the wire is exactly the case where +asking earns its complexity - contrast with the (deliberately self-describing-free) +SCPI/temperature examples. + +The walk happens in `EigerDetector.build`, which the framework calls once the +connection is open. `EigerConnection` is a plain `HTTPConnection` subclass: it knows +how to talk to the detector, and nothing about what the detector turns out to have. """ import enum -from dataclasses import dataclass -from typing import Any, cast +from typing import Any, NamedTuple, cast import httpx from fastcs.attributes import AttrR, AttrRW, Polled +from fastcs.connections import Connections, HTTPConnection, HTTPConnectionSettings from fastcs.controllers import Controller from fastcs.datatypes import DType from fastcs.demo.simulation.eiger import API_PREFIX, Subsystem, ValueType @@ -30,161 +33,187 @@ # Poll period (seconds) for read-only status params that change on the device. UPDATE_PERIOD = 0.2 +SUBSYSTEMS: tuple[Subsystem, ...] = ("config", "status") -def _datatype(param: str, data: dict[str, Any]) -> type[DType]: + +class ParameterInfo(NamedTuple): + """What the device says about one of its parameters. + + Deliberately the *shape* of the parameter and not its value: the shape is what + ``build`` turns into an attribute, and the value changes every time it is read. + """ + + subsystem: Subsystem + name: str + value_type: ValueType + access_mode: str + allowed_values: tuple[str, ...] | None + + +def _datatype(info: ParameterInfo) -> type[DType]: """Build a datatype for a parameter from the metadata the device reports. A parameter that reports ``allowed_values`` is discrete, so it becomes an enum class built from those values. The members are only knowable over the wire, - which is exactly the case introspection exists for. + which is exactly the case a runtime walk exists for. """ - allowed_values = data.get("allowed_values") - if allowed_values is None: - return _DATATYPES[data["value_type"]] + if info.allowed_values is None: + return _DATATYPES[info.value_type] - name = "".join(part.title() for part in param.split("_")) + name = "".join(part.title() for part in info.name.split("_")) # The functional API builds a class; type checkers only see the instance signature. return cast( - type[enum.Enum], enum.Enum(name, {value: value for value in allowed_values}) + type[enum.Enum], + enum.Enum(name, {value: value for value in info.allowed_values}), ) -@dataclass -class EigerConnectionSettings: - base_url: str = "http://localhost:8000" - +class EigerConnection(HTTPConnection): + """HTTP to the Eiger REST sim, and the one thing that knows when it is down. -class EigerConnection: - """Thin async HTTP client wrapper for the Eiger REST sim. + Everything about being an HTTP connection - the client, the disconnect on a + transport failure, the reconnect budget - comes from `HTTPConnection`. What is + here is only what is Eiger's rather than HTTP's: the URL layout, and the + ``{"value": ...}`` envelope the detector wraps every parameter in. A ``transport`` can be supplied to point directly at an in-process ASGI app (e.g. in tests), bypassing the network entirely. - """ - def __init__(self, transport: httpx.AsyncBaseTransport | None = None): - self._transport = transport - self._client: httpx.AsyncClient | None = None + Args: + settings: Where the detector's REST API lives + transport: Optional httpx transport, for talking to an in-process app + kwargs: Passed to `HTTPConnection` - async def connect(self, settings: EigerConnectionSettings) -> None: - self._client = httpx.AsyncClient( - base_url=settings.base_url, transport=self._transport - ) + """ - async def close(self) -> None: - if self._client is not None: - await self._client.aclose() - self._client = None + def __init__( + self, + settings: HTTPConnectionSettings | None = None, + transport: httpx.AsyncBaseTransport | None = None, + **kwargs, + ) -> None: + super().__init__(settings or HTTPConnectionSettings(port=8000), **kwargs) + self._transport = transport - @property - def client(self) -> httpx.AsyncClient: - if self._client is None: - raise RuntimeError("EigerConnection is not connected") - return self._client + async def get(self, path: str) -> Any: + """The detector wraps every parameter as ``{"value": ...}`` - unwrap it.""" + return (await super().get(path))["value"] async def keys(self, subsystem: Subsystem) -> list[str]: - response = await self.client.get(f"{API_PREFIX}/{subsystem}/keys") - response.raise_for_status() - return response.json() + """The parameter names one subsystem reports.""" + # The listing endpoint answers with a bare list rather than an envelope, + # so it goes through the un-overridden `get`. + return await super().get(f"{API_PREFIX}/{subsystem}/keys") + + async def describe(self, subsystem: Subsystem, param: str) -> dict: + """The whole envelope for one parameter: its value *and* its metadata.""" + return await super().get(f"{API_PREFIX}/{subsystem}/{param}") - async def get(self, subsystem: Subsystem, param: str) -> dict: - response = await self.client.get(f"{API_PREFIX}/{subsystem}/{param}") - response.raise_for_status() - return response.json() + async def get_parameter(self, subsystem: Subsystem, param: str) -> Any: + return await self.get(f"{API_PREFIX}/{subsystem}/{param}") - async def put(self, subsystem: Subsystem, param: str, value) -> None: - response = await self.client.put( - f"{API_PREFIX}/{subsystem}/{param}", json={"value": value} - ) - response.raise_for_status() + async def put_parameter(self, subsystem: Subsystem, param: str, value: Any) -> None: + await self.put(f"{API_PREFIX}/{subsystem}/{param}", {"value": value}) class EigerDetector(Controller): - """Cut-down Eiger controller: half declared, half introspected.""" + """Cut-down Eiger controller: half declared, half discovered at runtime.""" - # Declared (checked): must exist, with this access mode and dtype, after - # initialise() introspects the parameter tree. ``state`` is discrete, and its - # enum class is built from the ``allowed_values`` the device reports, so there - # is no author-time type to hint - only the access mode can be pinned here. + connection: EigerConnection + + # Declared (checked): must exist, with this access mode and dtype, once + # build() has turned what the device reports into attributes. ``state`` + # is discrete, and its enum class is built from the ``allowed_values`` the + # device reports, so there is no author-time type to hint - only the access + # mode can be pinned here. count_time: AttrRW[float] state: AttrR - # Derived (soft): built on top of the introspected ``state`` param. Declaring + # Derived (soft): built on top of the discovered ``state`` param. Declaring # ``state`` as a checked attribute is what lets us reference it in code and # publish something computed from it - here, whether the detector is idle. idle: AttrR[bool] - def __init__( - self, - settings: EigerConnectionSettings | None = None, - transport: httpx.AsyncBaseTransport | None = None, - ) -> None: - self.connection = EigerConnection(transport=transport) + def __init__(self, connections: Connections) -> None: + self.connection = connections.get("eiger", EigerConnection) super().__init__() - self._settings = settings or EigerConnectionSettings() - def _getter(self, subsystem: Subsystem, param: str): async def get() -> Any: - data = await self.connection.get(subsystem, param) # No cast here - ``update`` validates against the datatype, which is the # one place a bad value from the device should be coerced or complained # about. - return data["value"] + return await self.connection.get_parameter(subsystem, param) return get def _setter(self, subsystem: Subsystem, param: str): async def put(value: Any) -> None: - await self.connection.put(subsystem, param, value) + await self.connection.put_parameter(subsystem, param, value) return put - async def connect(self) -> None: - await self.connection.connect(self._settings) - self._connected = True + async def _walk(self) -> list[ParameterInfo]: + """Ask the detector what it has. - async def disconnect(self) -> None: - await self.connection.close() - - async def initialise(self) -> None: - for subsystem in ("config", "status"): + The connection is open by the time ``build`` runs, so this is a plain + sequence of reads - the shape of the tree is whatever the device answers + with on this particular startup. + """ + parameters: list[ParameterInfo] = [] + for subsystem in SUBSYSTEMS: for param in await self.connection.keys(subsystem): - data = await self.connection.get(subsystem, param) - datatype = _datatype(param, data) - - if data["access_mode"] == "rw": - getter = self._getter(subsystem, param) - setter = self._setter(subsystem, param) - else: - # Read-only params are status values that change on the device, - # so poll them periodically rather than reading once. - getter = Polled( - self._getter(subsystem, param), period=UPDATE_PERIOD - ) - setter = None - - declaration = self.filler.declarations.get(param) - if declaration is not None and declaration.child is not None: - # A parameter the class body declared already exists as an - # unfilled attribute, so provision that one rather than - # adding a second of the same name. The filler checks the - # access mode and datatype the hint promised against what - # the device turned out to report. - self.filler.fill_attribute( - param, datatype=datatype, getter=getter, setter=setter - ) - elif setter is None: - self.add_attribute(param, AttrR(datatype, getter=getter)) - else: - self.add_attribute( - param, AttrRW(datatype, getter=getter, setter=setter) + data = await self.connection.describe(subsystem, param) + allowed_values = data.get("allowed_values") + parameters.append( + ParameterInfo( + subsystem=subsystem, + name=param, + value_type=data["value_type"], + access_mode=data["access_mode"], + allowed_values=( + None if allowed_values is None else tuple(allowed_values) + ), ) + ) + return parameters + + async def build(self) -> None: + """Turn what the device reports into attributes.""" + for parameter in await self._walk(): + datatype = _datatype(parameter) + getter = self._getter(parameter.subsystem, parameter.name) + setter = None + + if parameter.access_mode == "rw": + setter = self._setter(parameter.subsystem, parameter.name) + else: + # Read-only params are status values that change on the device, + # so poll them periodically rather than reading once. + getter = Polled(getter, period=UPDATE_PERIOD) + + declaration = self.filler.declarations.get(parameter.name) + if declaration is not None and declaration.child is not None: + # A parameter the class body declared already exists as an + # unfilled attribute, so provision that one rather than adding a + # second of the same name. The filler checks the access mode and + # datatype the hint promised against what the device turned out + # to report. + self.filler.fill_attribute( + parameter.name, datatype=datatype, getter=getter, setter=setter + ) + elif setter is None: + self.add_attribute(parameter.name, AttrR(datatype, getter=getter)) + else: + self.add_attribute( + parameter.name, AttrRW(datatype, getter=getter, setter=setter) + ) # Every hinted parameter should have turned up in the tree the device # reported. self.filler.check_filled() - # Keep the derived ``idle`` flag in sync with the introspected ``state``. + # Keep the derived ``idle`` flag in sync with the discovered ``state``. self.state.add_readback_callback(self._update_idle) async def _update_idle(self, state: enum.Enum) -> None: diff --git a/src/fastcs/demo/fastcs.yaml b/src/fastcs/demo/fastcs.yaml index 1784ef2f4..957db6692 100644 --- a/src/fastcs/demo/fastcs.yaml +++ b/src/fastcs/demo/fastcs.yaml @@ -2,15 +2,21 @@ controllers: - id: MAIN type: fastcs.TemperatureController - ip_settings: - ip: "localhost" - port: 25565 + connections: + temperature: + type: fastcs.IPConnection + settings: + ip: "localhost" + port: 25565 num_ramp_controllers: 4 - id: AUX type: fastcs.TemperatureController - ip_settings: - ip: "localhost" - port: 25566 + connections: + temperature: + type: fastcs.IPConnection + settings: + ip: "localhost" + port: 25566 num_ramp_controllers: 2 transport: - graphql: diff --git a/src/fastcs/demo/schema.json b/src/fastcs/demo/schema.json index 210c36256..aa5608e07 100644 --- a/src/fastcs/demo/schema.json +++ b/src/fastcs/demo/schema.json @@ -174,6 +174,70 @@ "title": "GraphQLTransport", "type": "object" }, + "IPConnectionConfig": { + "additionalProperties": false, + "properties": { + "type": { + "const": "fastcs.IPConnection", + "title": "Type", + "type": "string" + }, + "depends_on": { + "anyOf": [ + { + "type": "string" + }, + { + "items": { + "type": "string" + }, + "type": "array" + } + ], + "title": "Depends On" + }, + "settings": { + "anyOf": [ + { + "$ref": "#/$defs/IPConnectionSettings" + }, + { + "type": "null" + } + ], + "default": null + }, + "reconnect_period": { + "anyOf": [ + { + "type": "number" + }, + { + "type": "null" + } + ], + "default": null, + "title": "Reconnect Period" + }, + "max_attempts": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "default": null, + "title": "Max Attempts" + } + }, + "required": [ + "type" + ], + "title": "IPConnectionConfig", + "type": "object" + }, "IPConnectionSettings": { "properties": { "ip": { @@ -259,19 +323,22 @@ "title": "Type", "type": "string" }, + "connections": { + "additionalProperties": { + "$ref": "#/$defs/IPConnectionConfig" + }, + "title": "Connections", + "type": "object" + }, "num_ramp_controllers": { "title": "Num Ramp Controllers", "type": "integer" - }, - "ip_settings": { - "$ref": "#/$defs/IPConnectionSettings" } }, "required": [ "id", "type", - "num_ramp_controllers", - "ip_settings" + "num_ramp_controllers" ], "title": "TemperatureControllerEntry", "type": "object" diff --git a/src/fastcs/demo/temperature_attr.py b/src/fastcs/demo/temperature_attr.py index 89a7caabc..690a16902 100755 --- a/src/fastcs/demo/temperature_attr.py +++ b/src/fastcs/demo/temperature_attr.py @@ -31,7 +31,7 @@ import numpy as np from fastcs.attributes import AttrR, AttrRW, Polled -from fastcs.connections import IPConnection, IPConnectionSettings +from fastcs.connections import Connections, IPConnection from fastcs.controllers import Controller, ControllerVector from fastcs.datatypes import Array1D, DType_T from fastcs.logging import logger @@ -46,7 +46,6 @@ class OnOffEnum(enum.StrEnum): @dataclass class TemperatureControllerSettings: num_ramp_controllers: int - ip_settings: IPConnectionSettings class TemperatureProtocol: @@ -126,8 +125,19 @@ async def get_actual(self) -> float: class TemperatureController(Controller): - def __init__(self, settings: TemperatureControllerSettings) -> None: - self.connection = IPConnection() + # Narrows the base class's `Connection | None`, so this controller's own code + # can call the methods of the connection it actually holds. + connection: IPConnection + + def __init__( + self, connections: Connections, settings: TemperatureControllerSettings + ) -> None: + # Claimed by the role name this driver's code asks for, which the deployment + # declares under `connections:` in its own entry. The ramps below hold this + # same object rather than consulting this controller, so the whole tree has + # one health state and one reconnect task between it. Opening it, and + # reopening it after a failure, is the runner's job - nothing here connects. + self.connection = connections.get("temperature", IPConnection) self._settings = settings self._protocol = TemperatureProtocol(self.connection) @@ -156,22 +166,6 @@ async def cancel_all(self) -> None: # TODO: The requests all get concatenated and the sim doesn't handle it await asyncio.sleep(0.1) - async def connect(self) -> None: - await self.connection.connect(self._settings.ip_settings) - - async def reconnect(self): - try: - await self.connection.close() - await self.connection.connect(self._settings.ip_settings) - except BaseException: - logger.exception("Reconnect failed") - return - - self._connected = True - - async def close(self) -> None: - await self.connection.close() - @scan(0.1) async def update_voltages(self): voltages = await self._protocol.get_voltages() @@ -186,6 +180,8 @@ async def update_voltages(self): class TemperatureRampController(Controller): + connection: IPConnection + def __init__(self, index: int, conn: IPConnection) -> None: self._protocol = TemperatureRampProtocol(conn, index) diff --git a/src/fastcs/launch.py b/src/fastcs/launch.py index 7bbe7726e..a50ea7704 100644 --- a/src/fastcs/launch.py +++ b/src/fastcs/launch.py @@ -10,6 +10,7 @@ from ruamel.yaml import YAML from fastcs import __version__ +from fastcs.connections import Connection, Connections from fastcs.control_system import FastCS from fastcs.controllers import Controller from fastcs.exceptions import LaunchError @@ -24,24 +25,42 @@ ) from fastcs.transports import Transport +CONNECTIONS_KEY = "connections" +"""Reserved, at both entry level in the config and in a Controller's ``__init__``. + +The block under an entry declares that controller's connections; the parameter of +the same name is how the registry built from it reaches the controller. Reserved +whether or not any connection classes are registered, so that registering one +later cannot collide with an existing driver's options field. +""" + +_ENTRY_KEYS = ("id", "type", CONNECTIONS_KEY) +"""Keys an entry model owns, rather than inlining from the options type.""" + @dataclasses.dataclass(frozen=True) class _RegisteredClass: cls: type[Controller] expects_options: bool options_type: Any + expects_connections: bool = False _ENTRY_REGISTRY: dict[type[BaseModel], _RegisteredClass] = {} """Maps each dynamically-built Entry model class to its originating Controller class and whether it expects an options arg (with the -options-type, if any). Populated by ``_build_entry_model`` and read by -``_instantiate_controllers``.""" +options-type, if any) and a `Connections` registry. Populated by +``_build_entry_model`` and read by ``_instantiate_controllers``.""" + +_CONNECTION_REGISTRY: dict[type[BaseModel], type[Connection]] = {} +"""Maps each dynamically-built connection model class to its Connection class. +Populated by ``_build_connection_model`` and read by ``_build_connections``.""" def launch( controller_classes: type[Controller] | list[type[Controller]], version: str | None = None, + connection_classes: type[Connection] | list[type[Connection]] | None = None, ) -> None: """ Serves as an entry point for starting FastCS applications. @@ -53,10 +72,14 @@ def launch( Args: controller_classes: One or more FastCS Controller classes to make available for instantiation. Each must have a type-hinted - __init__ method and no more than 2 arguments. The chosen class - for each id is selected by a required ``type`` discriminator - in the config. + __init__ method taking an optional ``connections`` argument and + no more than one options argument. The chosen class for each id + is selected by a required ``type`` discriminator in the config. version (Optional[str]): The version of the FastCS application. + connection_classes: The `Connection` classes an entry's + ``connections:`` block may declare, handed over explicitly like + the Controller classes rather than discovered from a global + registry. Omit it and no entry may declare connections. Raises: LaunchError: If a class's __init__ is not as expected. @@ -65,8 +88,9 @@ def launch( if __name__ == "__main__": launch(MyController) # single class launch([MyControllerA, MyControllerB]) # multi-class + launch(MyController, connection_classes=[MyConnection]) """ - _launch(controller_classes, version)() + _launch(controller_classes, version, connection_classes)() def _normalise_classes( @@ -79,13 +103,23 @@ def _normalise_classes( return [controller_classes] -def _discriminator(controller_class: type[Controller]) -> str: - """Type discriminator used in fastcs.yaml under each entry's ``type:`` key. +def _normalise_connection_classes( + connection_classes: type[Connection] | list[type[Connection]] | None, +) -> list[type[Connection]]: + if connection_classes is None: + return [] + if isinstance(connection_classes, list): + return connection_classes + return [connection_classes] + - Defaults to ``.`` so that Controllers from +def _discriminator(controller_class: type[Controller] | type[Connection]) -> str: + """Type discriminator used in fastcs.yaml under a ``type:`` key. + + Defaults to ``.`` so that classes from independently-distributed packages cannot collide. May be overridden verbatim (no prefix added) by setting ``type_name: ClassVar[str]`` on - the Controller class. + the Controller or Connection class. """ top_level_package = controller_class.__module__.split(".", 1)[0] default = f"{top_level_package}.{controller_class.__name__}" @@ -95,9 +129,12 @@ def _discriminator(controller_class: type[Controller]) -> str: def _launch( controller_classes: type[Controller] | list[type[Controller]], version: str | None = None, + connection_classes: type[Connection] | list[type[Connection]] | None = None, ) -> typer.Typer: classes = _normalise_classes(controller_classes) - fastcs_options = _build_options_model(classes) + fastcs_options = _build_options_model( + classes, _normalise_connection_classes(connection_classes) + ) app_name = classes[0].__name__ if len(classes) == 1 else "FastCS" launch_typer = typer.Typer() @@ -182,12 +219,15 @@ def run( raise LaunchError("Failed to validate config") from e - controllers = _instantiate_controllers(instance_options.controllers) + controllers, connections = _instantiate_controllers( + instance_options.controllers + ) instance = FastCS( controllers, instance_options.transport, loop=asyncio.get_event_loop(), + connections=connections, ) instance.run() @@ -197,16 +237,27 @@ def run( def _instantiate_controllers( controllers_options: list[Any], -) -> list[Controller]: +) -> tuple[list[Controller], list[Connections]]: """Instantiate each entry under `controllers:` and seed its path. Each item in ``controllers_options`` is a dynamically-built Pydantic - model that exposes ``id``, ``type`` and the controller's options fields - inlined as siblings. The originating Controller class and its - options-type are looked up in ``_ENTRY_REGISTRY`` (populated by - ``_build_entry_model``). The entry's ``id`` is seeded into the - controller's ``_path`` via ``set_path([id])`` so that - ``ControllerAPI.path`` is rooted at the YAML id. + model that exposes ``id``, ``type``, an optional ``connections`` block + and the controller's options fields inlined as siblings. The originating + Controller class and its options-type are looked up in + ``_ENTRY_REGISTRY`` (populated by ``_build_entry_model``). The entry's + ``id`` is seeded into the controller's ``_path`` via ``set_path([id])`` + so that ``ControllerAPI.path`` is rooted at the YAML id. + + One `Connections` registry is built per entry, from that entry's own + block, and forwarded down its subtree. Role names are therefore local to + an entry: two motors can both claim ``"motor"`` and get different + objects, which a single global block could not express. + + Returns: + The controllers, and the registry of each entry that declared one - + which is the whole set of connections the runner will supervise, since + a connection that is not declared here cannot be created later. + """ seen_ids: set[str] = set() duplicates: list[str] = [] @@ -220,21 +271,112 @@ def _instantiate_controllers( ) controllers: list[Controller] = [] + registries: list[Connections] = [] for entry in controllers_options: entry_cls: type[BaseModel] = type(entry) registered = _ENTRY_REGISTRY[entry_cls] + + block = getattr(entry, CONNECTIONS_KEY, {}) + args: list[Any] = [] + + if registered.expects_connections: + registry = _build_connections(entry.id, block) + registries.append(registry) + args.append(registry) + elif block: + raise LaunchError( + f"Controller {entry.id!r} declares connections " + f"{sorted(block)}, but {registered.cls.__name__}.__init__ takes " + f"no `{CONNECTIONS_KEY}` argument, so nothing would receive them." + ) + if registered.expects_options: field_values = { name: getattr(entry, name) for name in entry_cls.model_fields - if name not in ("id", "type") + if name not in _ENTRY_KEYS } - controller = registered.cls(registered.options_type(**field_values)) - else: - controller = registered.cls() + args.append(registered.options_type(**field_values)) + + controller = registered.cls(*args) controller.set_path([entry.id]) controllers.append(controller) - return controllers + return controllers, registries + + +def _depends_on_names(declared: str | list[str]) -> list[str]: + """``depends_on`` as a list of names, whether one or several were given.""" + return [declared] if isinstance(declared, str) else list(declared) + + +def _build_connections(entry_id: str, block: dict[str, Any]) -> Connections: + """Build one entry's `Connections` registry from its ``connections:`` block. + + Every connection is constructed first and ``depends_on`` resolved afterwards, + because it names connections by role and none of them exist while the block is + being read. Unknown names and cycles are rejected here rather than at startup, + so a config typo is a config error naming the roles it could have meant. + """ + instances: dict[str, Connection] = {} + dependency_names: dict[str, list[str]] = {} + + for name, options in block.items(): + connection_class = _CONNECTION_REGISTRY[type(options)] + kwargs = { + field: getattr(options, field) + for field in type(options).model_fields + if field not in ("type", "depends_on") + } + instances[name] = connection_class(**kwargs) + dependency_names[name] = _depends_on_names(options.depends_on) + + _check_dependency_names(entry_id, dependency_names) + + for name, dependencies in dependency_names.items(): + instances[name].depends_on = [instances[each] for each in dependencies] + + return Connections(instances) + + +def _check_dependency_names( + entry_id: str, dependency_names: dict[str, list[str]] +) -> None: + """Reject an unknown ``depends_on`` name, and any cycle between them. + + A cycle deadlocks silently at runtime - every connection in it waits on the + others forever, saying nothing beyond "waiting on dependencies" - so it is + caught here, where the roles have names to put in the message. + """ + for name, dependencies in dependency_names.items(): + for dependency in dependencies: + if dependency not in dependency_names: + raise LaunchError( + f"Connection {name!r} in controller {entry_id!r} depends on " + f"{dependency!r}, which is not declared. Declared: " + f"{sorted(dependency_names)}" + ) + + visiting: list[str] = [] + settled: set[str] = set() + + def visit(name: str) -> None: + if name in settled: + return + if name in visiting: + cycle = " -> ".join([*visiting[visiting.index(name) :], name]) + raise LaunchError( + f"Cycle in `depends_on` for controller {entry_id!r}: {cycle}. " + "Every connection in a cycle waits on the others forever." + ) + + visiting.append(name) + for dependency in dependency_names[name]: + visit(dependency) + visiting.pop() + settled.add(name) + + for name in dependency_names: + visit(name) def _options_field_definitions(options_type: type) -> dict[str, tuple[Any, Any]]: @@ -268,39 +410,157 @@ def _options_field_definitions(options_type: type) -> dict[str, tuple[Any, Any]] ) -def _build_entry_model(controller_class: type[Controller]) -> type[BaseModel]: +def _parameter_default(parameter: inspect.Parameter) -> Any: + return ... if parameter.default is inspect.Parameter.empty else parameter.default + + +def _connection_field_definitions( + connection_class: type[Connection], +) -> dict[str, tuple[Any, Any]]: + """Field-by-field definitions for one connection, from its ``__init__``. + + A connection's settings are constructor arguments rather than an options + object, so its config block is built from the signature directly - the same + treatment `_options_field_definitions` gives an options type. ``**kwargs`` + forwarded to `Connection` stands in for that base class's own arguments, so a + connection that only forwards them still gets ``reconnect_period`` and + ``reconnect_attempts`` in its schema. + + ``depends_on`` is deliberately absent: it names other connections, which do + not exist while the block is being read, so `_build_connections` resolves it + once they all do. + """ + signature = inspect.signature(connection_class.__init__) + hints = get_type_hints(connection_class.__init__) + base_signature = inspect.signature(Connection.__init__) + base_hints = get_type_hints(Connection.__init__) + + fields: dict[str, tuple[Any, Any]] = {} + for name, parameter in signature.parameters.items(): + if name in ("self", "depends_on"): + continue + + if parameter.kind is inspect.Parameter.VAR_POSITIONAL: + raise LaunchError( + f"Cannot build config for {connection_class.__name__}: " + f"`*{name}` cannot be expressed as a config field." + ) + + if parameter.kind is inspect.Parameter.VAR_KEYWORD: + for base_name, base_parameter in base_signature.parameters.items(): + if base_name in ("self", "depends_on") or base_name in fields: + continue + fields[base_name] = ( + base_hints[base_name], + _parameter_default(base_parameter), + ) + continue + + if name not in hints: + raise LaunchError( + f"Expected typehinting in '{connection_class.__name__}" + f".__init__' but received {signature}. Add a typehint for `{name}`." + ) + fields[name] = (hints[name], _parameter_default(parameter)) + + return fields + + +def _build_connection_model(connection_class: type[Connection]) -> type[BaseModel]: + """Build a Pydantic model for one connection in an entry's ``connections:``.""" + discriminator = _discriminator(connection_class) + + fields: dict[str, Any] = { + "type": (Literal[discriminator], ...), + "depends_on": (Union[str, list[str]], Field(default_factory=list)), # noqa: UP007 + } + for name, definition in _connection_field_definitions(connection_class).items(): + if name in fields: + raise LaunchError( + f"Connection {connection_class.__name__} takes a {name!r} argument, " + f"which collides with a launch-framework key." + ) + fields[name] = definition + + connection_model = create_model( + f"{connection_class.__name__}Config", + __config__={"extra": "forbid"}, + **fields, + ) + _CONNECTION_REGISTRY[connection_model] = connection_class + return connection_model + + +def _connections_field( + connection_classes: list[type[Connection]], +) -> tuple[Any, Any]: + """The ``connections:`` field of an entry: role name -> connection config. + + A discriminated union over the registered Connection classes, following the + Controller pattern rather than the Transport one - the classes are handed to + `launch` explicitly, so there is no global ``Connection.subclasses`` and no new + public surface. + """ + models = [_build_connection_model(cls) for cls in connection_classes] + + if len(models) == 1: + value_type: Any = models[0] + else: + value_type = Annotated[ + Union[tuple(models)], Field(discriminator="type") # noqa: UP007 + ] + + return (dict[str, value_type], Field(default_factory=dict)) + + +def _build_entry_model( + controller_class: type[Controller], + connection_classes: list[type[Connection]], +) -> type[BaseModel]: """Build a Pydantic model for one entry under `controllers:`. - Each entry exposes ``id`` and a ``type`` discriminator literal alongside - the options-type's fields, inlined as siblings (no nested ``controller:`` - block). The Controller class and its options-type are recorded in - ``_ENTRY_REGISTRY`` for use by ``_instantiate_controllers``. + Each entry exposes ``id``, a ``type`` discriminator literal and - when any + Connection classes are registered - a ``connections`` block, alongside the + options-type's fields inlined as siblings (no nested ``controller:`` block). + The Controller class and its options-type are recorded in ``_ENTRY_REGISTRY`` + for use by ``_instantiate_controllers``. + + ``connections`` is a reserved parameter name: a Controller that takes one gets + the registry built from its own block, and it counts towards neither the + argument limit nor the inlined option fields. """ sig = inspect.signature(controller_class.__init__) args = inspect.getfullargspec(controller_class.__init__)[0] discriminator = _discriminator(controller_class) + expects_connections = CONNECTIONS_KEY in args + counted = [arg for arg in args if arg != CONNECTIONS_KEY] + fields: dict[str, Any] = { "id": (str, ...), "type": (Literal[discriminator], ...), } + if connection_classes: + fields[CONNECTIONS_KEY] = _connections_field(connection_classes) + expects_options = False options_type: Any = None - if len(args) == 1: + if len(counted) == 1: pass - elif len(args) == 2: + elif len(counted) == 2: expects_options = True + options_arg = counted[-1] hints = get_type_hints(controller_class.__init__) hints.pop("return", None) - if not hints: + if options_arg not in hints: raise LaunchError( f"Expected typehinting in '{controller_class.__name__}" - f".__init__' but received {sig}. Add a typehint for `{args[-1]}`." + f".__init__' but received {sig}. Add a typehint for `{options_arg}`." ) - options_type = list(hints.values())[-1] + options_type = hints[options_arg] options_fields = _options_field_definitions(options_type) - for reserved in ("id", "type"): + for reserved in _ENTRY_KEYS: if reserved in options_fields: raise LaunchError( f"Options type {options_type.__name__} for " @@ -311,7 +571,7 @@ def _build_entry_model(controller_class: type[Controller]) -> type[BaseModel]: else: raise LaunchError( f"Expected no more than 2 arguments for '{controller_class.__name__}" - f".__init__' but received {len(args)} as `{sig}`" + f".__init__' but received {len(counted)} as `{sig}`" ) entry_model = create_model( @@ -323,12 +583,14 @@ def _build_entry_model(controller_class: type[Controller]) -> type[BaseModel]: cls=controller_class, expects_options=expects_options, options_type=options_type, + expects_connections=expects_connections, ) return entry_model def _build_options_model( controller_classes: list[type[Controller]], + connection_classes: list[type[Connection]] | None = None, ) -> type[BaseModel]: """Build the top-level Pydantic model for fastcs.yaml. @@ -338,7 +600,10 @@ def _build_options_model( fields are required. Duplicate ``id`` values across the list are rejected by ``_instantiate_controllers``. """ - entries = [_build_entry_model(cls) for cls in controller_classes] + connection_classes = connection_classes or [] + entries = [ + _build_entry_model(cls, connection_classes) for cls in controller_classes + ] if len(entries) == 1: entry_value_type: Any = entries[0] @@ -359,7 +624,10 @@ def _build_options_model( def get_controller_schema( target: type[Controller] | list[type[Controller]], + connection_classes: type[Connection] | list[type[Connection]] | None = None, ) -> dict[str, Any]: """Gets schema for given controller class(es) for serialisation.""" - options_model = _build_options_model(_normalise_classes(target)) + options_model = _build_options_model( + _normalise_classes(target), _normalise_connection_classes(connection_classes) + ) return options_model.model_json_schema() diff --git a/tests/assertable_controller.py b/tests/assertable_controller.py index 8299bff91..5c7993141 100644 --- a/tests/assertable_controller.py +++ b/tests/assertable_controller.py @@ -25,18 +25,12 @@ def __init__(self) -> None: self._sub_controllers.append(controller) self.add_sub_controller(f"SubController{index:02d}", controller) - initialised = False + built = False count = 0 - async def initialise(self) -> None: - await super().initialise() - self.initialised = True - - async def connect(self) -> None: - self._connected = True - - async def disconnect(self) -> None: - self._connected = False + async def build(self) -> None: + await super().build() + self.built = True @command() async def go(self): diff --git a/tests/demo/test_eiger.py b/tests/demo/test_eiger.py index 2b518c8bb..02ec50cf5 100644 --- a/tests/demo/test_eiger.py +++ b/tests/demo/test_eiger.py @@ -6,7 +6,9 @@ import pytest_asyncio from fastcs.attributes import AttrR, AttrRW -from fastcs.demo.eiger import UPDATE_PERIOD, EigerDetector +from fastcs.connections import Connections +from fastcs.controllers import ControllerRunner +from fastcs.demo.eiger import UPDATE_PERIOD, EigerConnection, EigerDetector from fastcs.demo.simulation.eiger import EigerParameter, create_eiger_sim_app from fastcs.util import ONCE @@ -14,15 +16,22 @@ SimState = dict[str, dict[str, EigerParameter]] +def _connections(app) -> Connections: + """The registry a `fastcs.yaml` would have built, pointed at the sim app.""" + return Connections( + {"eiger": EigerConnection(transport=httpx.ASGITransport(app=app))} + ) + + @pytest_asyncio.fixture async def _eiger(): app = create_eiger_sim_app() - controller = EigerDetector(transport=httpx.ASGITransport(app=app)) - await controller.connect() - await controller.initialise() - controller.post_initialise() + connections = _connections(app) + controller = EigerDetector(connections) + runner = ControllerRunner(controller, connections) + await runner.build() yield controller, app.state.sim - await controller.disconnect() + await runner.stop() @pytest_asyncio.fixture @@ -119,10 +128,10 @@ async def test_temperature_oscillation_seen_via_subscribe(): # the controller's temperature attribute, subscribing for updates. app = create_eiger_sim_app() async with app.router.lifespan_context(app): - controller = EigerDetector(transport=httpx.ASGITransport(app=app)) - await controller.connect() - await controller.initialise() - controller.post_initialise() + connections = _connections(app) + controller = EigerDetector(connections) + runner = ControllerRunner(controller, connections) + await runner.build() temperature = controller.attributes["temperature"] assert isinstance(temperature, AttrR) @@ -139,6 +148,6 @@ async def record(value: float) -> None: await temperature.poll() await asyncio.sleep(0.2) - await controller.disconnect() + await runner.stop() assert len(set(seen)) > 1, f"temperature did not change: {seen}" diff --git a/tests/demo/test_temperature_attr.py b/tests/demo/test_temperature_attr.py index bab260065..4ffb960f2 100644 --- a/tests/demo/test_temperature_attr.py +++ b/tests/demo/test_temperature_attr.py @@ -4,7 +4,7 @@ import pytest from fastcs.attributes import AttrW -from fastcs.connections import IPConnectionSettings +from fastcs.connections import Connections, IPConnection, IPConnectionSettings from fastcs.controllers import ControllerVector from fastcs.demo.temperature_attr import ( OnOffEnum, @@ -16,13 +16,12 @@ @pytest.fixture def controller() -> TemperatureController: - settings = TemperatureControllerSettings( - num_ramp_controllers=4, - ip_settings=IPConnectionSettings(ip="localhost", port=25565), + connections = Connections( + {"temperature": IPConnection(IPConnectionSettings(ip="localhost", port=25565))} + ) + return TemperatureController( + connections, TemperatureControllerSettings(num_ramp_controllers=4) ) - controller = TemperatureController(settings) - controller.post_initialise() - return controller @pytest.fixture diff --git a/tests/test_attributes.py b/tests/test_attributes.py index 0a0b0d061..16a7db717 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -400,7 +400,7 @@ class DemoParameterController(Controller): int_parameter: AttrRW float_parameter: AttrRW # hint to satisfy pyright - async def initialise(self): + async def build(self): self._connection = DummyConnection() await self._connection.connect() dtype_mapping = {"int": int, "float": float} @@ -454,7 +454,7 @@ async def setter(value, uri=uri): ) c = DemoParameterController() - await c.initialise() + await c.build() assert await c.ro_int_parameter.poll() == 10 assert await c.ro_int_parameter.poll() == 11 diff --git a/tests/test_connections.py b/tests/test_connections.py new file mode 100644 index 000000000..f540a1e75 --- /dev/null +++ b/tests/test_connections.py @@ -0,0 +1,266 @@ +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from fastcs.connections import ( + Connection, + Connections, + IPConnection, + IPConnectionSettings, + SerialConnection, + SerialConnectionSettings, + SimConnection, +) +from fastcs.connections.ip_connection import DisconnectedError, StreamConnection +from fastcs.connections.serial_connection import NotOpenedError + + +class OneConnection(Connection): + async def connect(self) -> None: ... + async def close(self) -> None: ... + + +class AnotherConnection(Connection): + async def connect(self) -> None: ... + async def close(self) -> None: ... + + +# Connections registry + + +def test_a_connection_is_claimed_by_name_with_its_type_asserted(): + connection = OneConnection() + registry = Connections({"one": connection}) + + assert registry.get("one", OneConnection) is connection + + +def test_claiming_a_name_that_was_not_declared_lists_the_ones_that_were(): + registry = Connections({"one": OneConnection(), "two": AnotherConnection()}) + + with pytest.raises(KeyError, match=r"No connection named 'three'") as exc: + registry.get("three", OneConnection) + + assert "'one', 'two'" in str(exc.value) + + +def test_claiming_a_name_with_the_wrong_type_says_both_types(): + registry = Connections({"one": AnotherConnection()}) + + with pytest.raises(TypeError, match="is AnotherConnection, but OneConnection"): + registry.get("one", OneConnection) + + +def test_a_registry_reports_what_was_never_claimed(): + registry = Connections({"used": OneConnection(), "spare": OneConnection()}) + + assert registry.unclaimed() == {"used", "spare"} + + registry.get("used", OneConnection) + + assert registry.unclaimed() == {"spare"} + + +def test_a_connection_is_named_by_identity_not_equality(): + """Two connections with matching settings are two connections.""" + first, second = OneConnection(), OneConnection() + registry = Connections({"first": first, "second": second}) + + assert registry.name_of(first) == "first" + assert registry.name_of(second) == "second" + assert registry.name_of(OneConnection()) is None + + +def test_a_registry_keeps_declaration_order(): + first, second = OneConnection(), AnotherConnection() + registry = Connections({"first": first, "second": second}) + + assert registry.values() == [first, second] + assert len(registry) == 2 + assert "first" in registry + assert "third" not in registry + assert repr(registry) == "Connections(['first', 'second'])" + + +# IPConnection + + +@pytest.mark.asyncio +async def test_ip_connect_opens_the_settings_it_was_given(): + connection = IPConnection(IPConnectionSettings(ip="192.0.2.1", port=1234)) + reader, writer = MagicMock(), MagicMock() + + with patch( + "asyncio.open_connection", AsyncMock(return_value=(reader, writer)) + ) as open_connection: + await connection.connect() + + open_connection.assert_awaited_once_with("192.0.2.1", 1234) + assert isinstance(connection._connection, StreamConnection) # noqa: SLF001 + + +@pytest.mark.asyncio +async def test_using_an_unopened_ip_connection_says_so(): + with pytest.raises(DisconnectedError, match="call connect"): + await IPConnection().send_command("ID?\r\n") + + +@pytest.mark.asyncio +async def test_a_command_that_hits_a_dead_socket_marks_the_link_down(): + connection = IPConnection() + stream = MagicMock() + stream.__aenter__ = AsyncMock(return_value=stream) + stream.__aexit__ = AsyncMock(return_value=False) + stream.send_message = AsyncMock(side_effect=ConnectionResetError) + connection._IPConnection__connection = stream # pyright: ignore[reportAttributeAccessIssue] + connection._set_connected() # noqa: SLF001 + + with pytest.raises(ConnectionResetError): + await connection.send_command("R=1\r\n") + + assert not connection.connected + + +@pytest.mark.asyncio +async def test_a_command_the_device_accepts_leaves_the_link_up(): + connection = IPConnection() + stream = MagicMock() + stream.__aenter__ = AsyncMock(return_value=stream) + stream.__aexit__ = AsyncMock(return_value=False) + stream.send_message = AsyncMock() + connection._IPConnection__connection = stream # pyright: ignore[reportAttributeAccessIssue] + connection._set_connected() # noqa: SLF001 + + await connection.send_command("R=1\r\n") + + stream.send_message.assert_awaited_once_with("R=1\r\n") + assert connection.connected + + +@pytest.mark.asyncio +async def test_stream_connection_reads_and_writes_lines(): + reader = asyncio.StreamReader() + reader.feed_data(b"ID=1\r\n") + writer = MagicMock() + writer.drain = AsyncMock() + writer.wait_closed = AsyncMock() + + stream = StreamConnection(reader, writer) + async with stream as held: + await held.send_message("ID?\r\n") + assert await held.receive_response() == "ID=1\r\n" + + writer.write.assert_called_once_with(b"ID?\r\n") + + await stream.close() + writer.close.assert_called_once() + + +# SerialConnection + + +@pytest.mark.asyncio +async def test_serial_connect_opens_the_settings_it_was_given(): + connection = SerialConnection( + SerialConnectionSettings(port="/dev/ttyS0", baud=9600) + ) + + with patch("aioserial.AioSerial") as aioserial: + await connection.connect() + + aioserial.assert_called_once_with(port="/dev/ttyS0", baudrate=9600) + + +@pytest.mark.asyncio +async def test_using_an_unopened_serial_connection_says_so(): + connection = SerialConnection(SerialConnectionSettings(port="/dev/ttyS0")) + + with pytest.raises(NotOpenedError, match="call connect"): + await connection.send_command(b"ID?\r\n") + + +@pytest.mark.asyncio +async def test_serial_round_trip_leaves_the_link_up(): + connection = SerialConnection(SerialConnectionSettings(port="/dev/ttyS0")) + stream = MagicMock() + stream.write_async = AsyncMock() + stream.read_async = AsyncMock(return_value=b"ID=1") + + with patch("aioserial.AioSerial", return_value=stream): + await connection.connect() + connection._set_connected() # noqa: SLF001 + + await connection.send_command(b"R=1\r\n") + assert await connection.send_query(b"ID?\r\n", 4) == b"ID=1" + assert connection.connected + + await connection.close() + stream.close.assert_called_once() + # Closing an already-closed link is tolerated - the runner does it before + # every reconnect attempt. + await connection.close() + + +@pytest.mark.asyncio +async def test_a_serial_port_that_goes_away_marks_the_link_down(): + connection = SerialConnection(SerialConnectionSettings(port="/dev/ttyS0")) + stream = MagicMock() + stream.write_async = AsyncMock(side_effect=OSError) + stream.read_async = AsyncMock(side_effect=OSError) + + with patch("aioserial.AioSerial", return_value=stream): + await connection.connect() + connection._set_connected() # noqa: SLF001 + + with pytest.raises(OSError): + await connection.send_command(b"R=1\r\n") + assert not connection.connected + + connection._set_connected() # noqa: SLF001 + stream.write_async = AsyncMock() + with pytest.raises(OSError): + await connection.send_query(b"ID?\r\n", 4) + assert not connection.connected + + +# SimConnection + + +@pytest.mark.asyncio +async def test_a_sim_connection_opens_and_closes_without_a_transport(): + """The pretending is all a driver writes: there is nothing here to fail.""" + + class SimDevice(SimConnection): + def __init__(self, **kwargs) -> None: + super().__init__(**kwargs) + self.position = 0 + + async def move(self, steps: int) -> None: + self.position += steps + + connection = SimDevice() + await connection.connect() + connection._set_connected() # noqa: SLF001 + + await connection.move(3) + assert connection.position == 3 + assert connection.connected + + await connection.close() + + +@pytest.mark.asyncio +async def test_a_sim_connection_is_a_sibling_of_the_real_transports(): + """Not a subclass of one: it would inherit a handle it never opens. + + It is a `Connection` like any other, so it takes the same reconnect settings + and is chosen by ``type:`` in the same place - even though its reconnect task + will idle forever. + """ + assert issubclass(SimConnection, Connection) + assert not issubclass(SimConnection, IPConnection | SerialConnection) + + connection = SimConnection.__new__(SimConnection) + Connection.__init__(connection, reconnect_period=2.0) + assert connection.reconnect_period == 2.0 diff --git a/tests/test_control_system.py b/tests/test_control_system.py index 19e3215e7..9397f7632 100644 --- a/tests/test_control_system.py +++ b/tests/test_control_system.py @@ -3,6 +3,7 @@ import pytest from fastcs.attributes import AttrR, NotPolled, Polled +from fastcs.connections import Connection, Connections from fastcs.control_system import FastCS from fastcs.controllers import Controller from fastcs.methods import Command, command @@ -30,7 +31,7 @@ class MyTestController(Controller): def __init__(self): super().__init__() - async def initialise(self): + async def build(self): async def do_nothing_dynamic() -> None: pass @@ -94,33 +95,68 @@ def __init__(self): assert controller.update_once.readback == 1 assert controller.update_never.readback == 0 - # One periodic scan task per distinct period, plus one reconnect watcher + # One periodic scan task per distinct period assert len(fastcs._runner._scan_coros) == 1 assert len(fastcs._runner._initial_coros) == 1 @pytest.mark.asyncio -async def test_controller_connect_disconnect(): - class MyTestController(Controller): - async def connect(self): - self.connect_called = True +async def test_serve_opens_and_closes_the_connection(): + """Opening and closing the link is the runner's job, not the controller's.""" + + class MyTestConnection(Connection): + def __init__(self): + super().__init__() + self.open = False - async def disconnect(self): - self.connect_called = False + async def connect(self) -> None: + self.open = True - controller = MyTestController() + async def close(self) -> None: + self.open = False + + class MyTestController(Controller): + def __init__(self, connection): + self.connection = connection + super().__init__() + + connection = MyTestConnection() + controller = MyTestController(connection) loop = asyncio.get_event_loop() - fastcs = FastCS(controller, [], loop) + fastcs = FastCS(controller, [], loop, Connections({"device": connection})) task = asyncio.create_task(fastcs.serve(interactive=False)) - # connect is called at the start of serve + # The runner opens every connection at the start of serve await asyncio.sleep(0.1) - assert controller.connect_called + assert connection.open + assert controller.connected task.cancel() - # disconnect is called at the end of serve + # ...and closes them at the end of it + await asyncio.sleep(0.1) + assert not connection.open + + +@pytest.mark.asyncio +async def test_a_fatal_runner_condition_comes_out_of_serve(): + """Not `sys.exit`: an embedded FastCS must be able to see this and decide. + + Reported rather than raised, because whatever hits it is a background task with + nothing awaiting it. + """ + + class MyTestController(Controller): + pass + + fastcs = FastCS(MyTestController(), [], asyncio.get_event_loop()) + + task = asyncio.create_task(fastcs.serve(interactive=False)) await asyncio.sleep(0.1) - assert not controller.connect_called + + fastcs._runner.fail(RuntimeError("the device is beyond saving")) + + with pytest.raises(RuntimeError, match="beyond saving"): + await asyncio.wait_for(task, timeout=5) diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py index f52cd9a8e..68679f820 100644 --- a/tests/test_controller_filler.py +++ b/tests/test_controller_filler.py @@ -54,14 +54,14 @@ class Declared(Controller): async def test_attributes_added_without_a_hint(): # `fastcs-PandABlocks`/`fastcs-secop`: the whole tree comes off the wire. class Dynamic(Controller): - async def initialise(self) -> None: + async def build(self) -> None: self.add_attribute("discovered", AttrR(int)) controller = Dynamic() assert controller.attributes == {} - await controller.initialise() + await controller.build() assert set(controller.attributes) == {"discovered"} controller.check_filled() diff --git a/tests/test_controller_runner.py b/tests/test_controller_runner.py index b60bb7a64..482829e7d 100644 --- a/tests/test_controller_runner.py +++ b/tests/test_controller_runner.py @@ -3,33 +3,51 @@ import pytest -from fastcs.attributes import AttrR +from fastcs.attributes import AttrR, Polled +from fastcs.connections import ( + DEFAULT_RECONNECT_ATTEMPTS, + DEFAULT_RECONNECT_PERIOD, + Connection, + Connections, +) from fastcs.controllers import Controller, ControllerRunner -from fastcs.controllers.runner import RECONNECT_PERIOD +from fastcs.controllers.runner import MAX_BUILD_PASSES from fastcs.methods import scan from fastcs.util import ONCE +class FakeConnection(Connection): + """A connection that opens when told to, and records what was asked of it.""" + + def __init__(self, **kwargs) -> None: + super().__init__(**kwargs) + self.fail_next: Exception | None = None + self.connects = 0 + self.closes = 0 + + async def connect(self) -> None: + self.connects += 1 + if self.fail_next is not None: + raise self.fail_next + + async def close(self) -> None: + self.closes += 1 + + class LifecycleController(Controller): """Records every lifecycle hook the runner is supposed to call.""" - def __init__(self): + def __init__(self, connection: Connection | None = None): + self.connection = connection super().__init__() self.events: list[str] = [] self.count = AttrR(int) - async def initialise(self): - self.events.append("initialise") - - def post_initialise(self): - self.events.append("post_initialise") - - async def connect(self): - self.events.append("connect") - await super().connect() + async def build(self): + self.events.append("build") - async def disconnect(self): - self.events.append("disconnect") + async def setup(self): + self.events.append("setup") @scan(ONCE) async def read_once(self): @@ -37,43 +55,74 @@ async def read_once(self): await self.count.update(self.count.readback + 1) +def runner_for(controllers, **connections: Connection) -> ControllerRunner: + """A runner over an explicit registry. + + Every connection a runner supervises is declared, never found: the registry is + the whole list, so a test says what it declared the same way ``fastcs.yaml`` + does. + """ + return ControllerRunner(controllers, Connections(dict(connections))) + + @pytest.mark.asyncio async def test_the_runner_drives_the_whole_lifecycle(): - controller = LifecycleController() - runner = ControllerRunner(controller) + connection = FakeConnection() + controller = LifecycleController(connection) + runner = runner_for(controller, only=connection) await runner.start() try: - assert controller.events == [ - "initialise", - "post_initialise", - "connect", - "initial", - ] + assert controller.events == ["build", "setup", "initial"] assert controller.count.readback == 1 + assert connection.connects == 1 + assert connection.connected finally: await runner.stop() - assert controller.events[-1] == "disconnect" + # Shutdown is a runner operation, not an author hook + assert connection.closes == 1 + + +@pytest.mark.asyncio +async def test_connections_open_before_anything_is_built(): + """A ``build`` runs against an open link, so it can ask the device questions.""" + connection = FakeConnection() + order: list[str] = [] + + class RecordingController(Controller): + def __init__(self): + self.connection = connection + super().__init__() + + async def build(self): + order.append(f"build(connected={connection.connected})") + + runner = runner_for(RecordingController(), only=connection) + await runner.build() + + assert order == ["build(connected=True)"] @pytest.mark.asyncio -async def test_setup_builds_the_apis_before_anything_connects(): - """A transport is wired to the APIs between setup and start.""" - controller = LifecycleController() - runner = ControllerRunner(controller) +async def test_build_builds_the_apis_before_anything_is_set_up(): + """A transport is wired to the APIs between build and start.""" + connection = FakeConnection() + controller = LifecycleController(connection) + runner = runner_for(controller, only=connection) - apis = await runner.setup() + apis = await runner.build() assert [api.path for api in apis] == [[]] assert "count" in apis[0].attributes - assert controller.events == ["initialise", "post_initialise"] + assert controller.events == ["build"] assert runner.controller_apis == apis @pytest.mark.asyncio -async def test_start_sets_up_when_setup_has_not_run(): - runner = ControllerRunner(LifecycleController()) +async def test_start_builds_when_build_has_not_run(): + connection = FakeConnection() + runner = runner_for(LifecycleController(connection), only=connection) await runner.start() try: @@ -84,8 +133,9 @@ async def test_start_sets_up_when_setup_has_not_run(): @pytest.mark.asyncio async def test_a_runner_takes_several_controllers(): - controllers = [LifecycleController(), LifecycleController()] - runner = ControllerRunner(controllers) + first, second = FakeConnection(), FakeConnection() + controllers = [LifecycleController(first), LifecycleController(second)] + runner = runner_for(controllers, first=first, second=second) await runner.start() try: @@ -96,90 +146,520 @@ async def test_a_runner_takes_several_controllers(): @pytest.mark.asyncio -async def test_stop_reports_a_failing_disconnect_without_raising(monkeypatch): - class UndisconnectableController(LifecycleController): - async def disconnect(self): - raise RuntimeError("no") +async def test_a_controller_with_no_connection_still_runs(): + """A soft controller that groups others has nothing to connect.""" + controller = LifecycleController(None) + runner = runner_for(controller) - logged: list[tuple[str, BaseException | None]] = [] + await runner.start() + try: + assert runner.connections == [] + assert controller.events == ["build", "setup", "initial"] + assert controller.connected + finally: + await runner.stop() - def record_exception(event, **kwargs): - # ``logger.exception`` is called from the ``except`` block, so the - # exception it is reporting is the one currently being handled. - logged.append((event, sys.exc_info()[1])) - monkeypatch.setattr("fastcs.controllers.runner.logger.exception", record_exception) +@pytest.mark.asyncio +async def test_setup_runs_once_the_whole_tree_is_built(): + """A parent's ``setup`` can read a child that only exists after ``build``.""" + order: list[str] = [] + + class Child(Controller): + async def build(self): + order.append("child build") + + async def setup(self): + order.append("child setup") + + class Parent(Controller): + async def build(self): + order.append("parent build") + self.add_sub_controller("CHILD", Child()) + + async def setup(self): + order.append("parent setup") - runner = ControllerRunner(UndisconnectableController()) + runner = runner_for(Parent()) await runner.start() + try: + assert order == ["parent build", "child build", "parent setup", "child setup"] + finally: + await runner.stop() - await runner.stop() - assert len(logged) == 1 - event, error = logged[0] - assert event == "Exception during disconnect" - assert isinstance(error, RuntimeError) - assert str(error) == "no" +@pytest.mark.asyncio +async def test_build_repeats_until_the_tree_stops_growing(): + class Tier(Controller): + def __init__(self, depth: int) -> None: + super().__init__() + self._depth = depth + + async def build(self): + if self._depth: + self.add_sub_controller("SUB", Tier(self._depth - 1)) + + runner = runner_for(Tier(3)) + await runner.build() + + controller = runner._controllers[0] + for _ in range(3): + controller = controller.sub_controllers["SUB"] # type: ignore[assignment] + assert controller.sub_controllers == {} + + +@pytest.mark.asyncio +async def test_a_tree_that_never_settles_is_caught(): + class Runaway(Controller): + async def build(self): + self.add_sub_controller("SUB", Runaway()) + + runner = runner_for(Runaway()) + + with pytest.raises(RuntimeError, match=f"{MAX_BUILD_PASSES} build passes"): + await runner.build() + + +@pytest.mark.asyncio +async def test_controllers_sharing_a_connection_are_one_connection(): + """Identity, not equality: the tree is not the unit of failure, the link is.""" + connection = FakeConnection() + + class Parent(Controller): + def __init__(self): + self.connection = connection + super().__init__() + self.add_sub_controller("A", LifecycleController(connection)) + self.add_sub_controller("B", LifecycleController(connection)) + + runner = runner_for(Parent(), shared=connection) + await runner.build() + + assert runner.connections == [connection] + assert connection.connects == 1 @pytest.mark.asyncio -async def test_the_runner_reconnects_a_controller_that_dropped_out(monkeypatch): - """Nothing else calls reconnect, so a paused controller would stay paused.""" - monkeypatch.setattr("fastcs.controllers.runner.RECONNECT_PERIOD", 0.01) +async def test_a_connection_created_during_build_is_rejected(): + """It would never be opened, and so never reconnected either.""" - class DroppingController(LifecycleController): - reconnects = 0 + class LateConnector(Controller): + async def build(self): + child = LifecycleController(FakeConnection()) + self.add_sub_controller("LATE", child) - async def reconnect(self): - self.reconnects += 1 - await super().reconnect() + with pytest.raises(RuntimeError, match="did not open"): + await runner_for(LateConnector()).build() - controller = DroppingController() - runner = ControllerRunner(controller) + +@pytest.mark.asyncio +async def test_a_declared_but_unclaimed_connection_is_warned_about(monkeypatch): + warnings: list[dict] = [] + monkeypatch.setattr( + "fastcs.controllers.runner.logger.warning", + lambda event, **kwargs: warnings.append({"event": event, **kwargs}), + ) + + claimed = FakeConnection() + connections = Connections({"used": claimed, "spare": FakeConnection()}) + connections.get("used", FakeConnection) + + runner = ControllerRunner(LifecycleController(claimed), connections) await runner.start() try: - assert controller.connected + assert [w["connection"] for w in warnings if "never used" in w["event"]] == [ + "spare" + ] + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_a_connection_nothing_polls_is_warned_about(monkeypatch): + warnings: list[dict] = [] + monkeypatch.setattr( + "fastcs.controllers.runner.logger.warning", + lambda event, **kwargs: warnings.append({"event": event, **kwargs}), + ) + + class OnDemandOnly(Controller): + def __init__(self, connection): + self.connection = connection + super().__init__() + + on_demand = FakeConnection() + runner = runner_for(OnDemandOnly(on_demand), only=on_demand) + await runner.start() + try: + assert any("no polled attribute" in w["event"] for w in warnings) + finally: + await runner.stop() + + warnings.clear() + + class Polling(Controller): + def __init__(self, connection): + self.connection = connection + super().__init__() + self.value = AttrR(int, getter=Polled(self._get, period=0.2)) + + async def _get(self) -> int: + return 1 + + polled = FakeConnection() + runner = runner_for(Polling(polled), only=polled) + await runner.start() + try: + assert not any("no polled attribute" in w["event"] for w in warnings) + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_the_runner_reconnects_a_connection_that_dropped_out(): + """The connection's own IO marks it down; one task per connection brings it back.""" + connection = FakeConnection(reconnect_period=0.01) + runner = runner_for(LifecycleController(connection), only=connection) + await runner.start() + try: + assert connection.connected - # What a scan task does when its callback raises - controller._connected = False + # What a connection's IO does when its transport fails + connection.set_disconnected() + await connection.wait_up() + assert connection.connected + assert connection.connects == 2 + # Closed before the reconnect attempt, tolerating an already-closed link + assert connection.closes == 1 + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_a_failing_reconnect_keeps_trying_then_gives_up(): + connection = FakeConnection(reconnect_period=0.001, reconnect_attempts=3) + runner = runner_for(LifecycleController(connection), only=connection) + await runner.start() + try: + connection.fail_next = RuntimeError("still down") + connection.set_disconnected() + + state = runner._state[connection] + await asyncio.wait_for(state.exhausted.wait(), timeout=2) + + assert state.attempts == 3 + assert not connection.connected + + # Terminal until the process restarts: no further attempts + attempts_at_exhaustion = connection.connects await asyncio.sleep(0.05) + assert connection.connects == attempts_at_exhaustion + finally: + await runner.stop() - assert controller.reconnects >= 1 - assert controller.connected + +@pytest.mark.asyncio +async def test_a_clean_reconnect_restores_the_retry_budget(): + connection = FakeConnection(reconnect_period=0.001, reconnect_attempts=1000) + runner = runner_for(LifecycleController(connection), only=connection) + await runner.start() + try: + connection.fail_next = RuntimeError("down") + connection.set_disconnected() + await asyncio.sleep(0.02) + assert runner._state[connection].attempts > 0 + + connection.fail_next = None + await asyncio.wait_for(connection.wait_up(), timeout=2) + + assert runner._state[connection].attempts == 0 finally: await runner.stop() @pytest.mark.asyncio -async def test_a_failing_reconnect_does_not_stop_the_runner(monkeypatch): - monkeypatch.setattr("fastcs.controllers.runner.RECONNECT_PERIOD", 0.01) +async def test_a_dependent_waits_rather_than_spending_its_budget(): + """No attempt means no increment, so the budget freezes while waiting.""" + base = FakeConnection(reconnect_period=0.001, reconnect_attempts=1000) + layered = FakeConnection(depends_on=base, reconnect_period=0.001) + + runner = runner_for( + [LifecycleController(base), LifecycleController(layered)], + base=base, + layered=layered, + ) + await runner.start() + try: + base.fail_next = RuntimeError("down") + base.set_disconnected() + layered.set_disconnected() - class UnreconnectableController(LifecycleController): - attempts = 0 + await asyncio.sleep(0.05) + + # The dependent has not attempted at all while its dependency is down + assert runner._state[layered].attempts == 0 + assert layered.connects == 1 - async def reconnect(self): - self.attempts += 1 - raise RuntimeError("still down") + base.fail_next = None + await asyncio.wait_for(layered.wait_up(), timeout=2) + finally: + await runner.stop() - controller = UnreconnectableController() - runner = ControllerRunner(controller) + +@pytest.mark.asyncio +async def test_a_dependent_is_released_when_its_dependency_gives_up(monkeypatch): + """Released rather than left hanging, so it stalls loudly.""" + errors: list[dict] = [] + monkeypatch.setattr( + "fastcs.controllers.runner.logger.error", + lambda event, **kwargs: errors.append({"event": event, **kwargs}), + ) + + base = FakeConnection(reconnect_period=0.001, reconnect_attempts=1) + layered = FakeConnection(depends_on=base, reconnect_period=0.001) + + runner = runner_for( + [LifecycleController(base), LifecycleController(layered)], + base=base, + layered=layered, + ) await runner.start() try: - controller._connected = False + base.fail_next = RuntimeError("down for good") + base.set_disconnected() + layered.set_disconnected() + + await asyncio.sleep(0.2) + + gave_up = [e for e in errors if e["event"] == "Giving up"] + assert gave_up and gave_up[0]["connection"] == "base" + # The give-up message names what it takes down with it + assert gave_up[0]["blocks"] == ["layered"] + + stalled = [e for e in errors if e["event"].startswith("Stalled")] + assert stalled and stalled[0]["connection"] == "layered" + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_a_dependent_waits_for_every_dependency(): + """All of them must be up: a connection layered over two links is no more + usable with one of them than with neither.""" + first = FakeConnection(reconnect_period=0.001, reconnect_attempts=1000) + second = FakeConnection(reconnect_period=0.001, reconnect_attempts=1000) + layered = FakeConnection(depends_on=[first, second], reconnect_period=0.001) + + runner = runner_for( + [ + LifecycleController(first), + LifecycleController(second), + LifecycleController(layered), + ], + first=first, + second=second, + layered=layered, + ) + await runner.start() + try: + for connection in (first, second): + connection.fail_next = RuntimeError("down") + connection.set_disconnected() + layered.set_disconnected() + + # Only one of the two comes back + first.fail_next = None + await asyncio.wait_for(first.wait_up(), timeout=2) await asyncio.sleep(0.05) - # It keeps trying rather than dying on the first failure - assert controller.attempts > 1 - assert not controller.connected + assert runner._state[layered].attempts == 0 + assert not layered.connected + + second.fail_next = None + await asyncio.wait_for(layered.wait_up(), timeout=2) + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_any_dependency_giving_up_stalls_the_dependent(monkeypatch): + """One that has given up is enough to make the dependent unusable, however + healthy the others are.""" + errors: list[dict] = [] + monkeypatch.setattr( + "fastcs.controllers.runner.logger.error", + lambda event, **kwargs: errors.append({"event": event, **kwargs}), + ) + + healthy = FakeConnection(reconnect_period=0.001, reconnect_attempts=1000) + doomed = FakeConnection(reconnect_period=0.001, reconnect_attempts=1) + layered = FakeConnection(depends_on=[healthy, doomed], reconnect_period=0.001) + + runner = runner_for( + [ + LifecycleController(healthy), + LifecycleController(doomed), + LifecycleController(layered), + ], + healthy=healthy, + doomed=doomed, + layered=layered, + ) + await runner.start() + try: + doomed.fail_next = RuntimeError("down for good") + doomed.set_disconnected() + layered.set_disconnected() + + await asyncio.sleep(0.2) + + stalled = [e for e in errors if e["event"].startswith("Stalled")] + assert stalled and stalled[0]["connection"] == "layered" + assert stalled[0]["dependencies"] == ["doomed"] + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_connections_are_opened_in_dependency_order(): + """A connection layered over another must not be opened before it, however + the two were declared.""" + opened: list[str] = [] + + class Recorded(FakeConnection): + def __init__(self, name: str, **kwargs) -> None: + super().__init__(**kwargs) + self.name = name + + async def connect(self) -> None: + opened.append(self.name) + await super().connect() + + base = Recorded("base") + layered = Recorded("layered", depends_on=base) + + # Declared the wrong way round on purpose + runner = runner_for( + [LifecycleController(layered), LifecycleController(base)], + layered=layered, + base=base, + ) + await runner.build() + try: + assert opened == ["base", "layered"] + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_a_dependency_cycle_is_caught_at_startup(): + first = FakeConnection() + second = FakeConnection(depends_on=first) + first.depends_on = [second] + + runner = runner_for( + [LifecycleController(first), LifecycleController(second)], + first=first, + second=second, + ) + + with pytest.raises(ValueError, match="Cycle in connection dependencies"): + await runner.build() + + +@pytest.mark.asyncio +async def test_scans_are_gated_on_the_connection(): + connection = FakeConnection(reconnect_period=0.001) + + class Scanning(Controller): + def __init__(self): + self.connection = connection + super().__init__() + self.scans = 0 + + @scan(0.001) + async def tick(self): + self.scans += 1 + + controller = Scanning() + runner = runner_for(controller, only=connection) + await runner.start() + try: + await asyncio.sleep(0.02) + assert controller.scans > 0 + + connection.fail_next = RuntimeError("down") + connection.set_disconnected() + await asyncio.sleep(0.02) + + paused_at = controller.scans + await asyncio.sleep(0.02) + assert controller.scans == paused_at finally: await runner.stop() +@pytest.mark.asyncio +async def test_stop_closes_connections_in_reverse_declaration_order(): + closed: list[str] = [] + + class Recording(FakeConnection): + def __init__(self, name: str, **kwargs): + super().__init__(**kwargs) + self.name = name + + async def close(self) -> None: + await super().close() + closed.append(self.name) + + base = Recording("base") + layered = Recording("layered", depends_on=base) + + runner = runner_for( + [LifecycleController(base), LifecycleController(layered)], + base=base, + layered=layered, + ) + await runner.start() + await runner.stop() + + assert closed == ["layered", "base"] + + +@pytest.mark.asyncio +async def test_stop_reports_a_failing_close_without_raising(monkeypatch): + class UncloseableConnection(FakeConnection): + async def close(self) -> None: + raise RuntimeError("no") + + logged: list[tuple[str, BaseException | None]] = [] + + def record_exception(event, **kwargs): + # ``logger.exception`` is called from the ``except`` block, so the + # exception it is reporting is the one currently being handled. + logged.append((event, sys.exc_info()[1])) + + monkeypatch.setattr("fastcs.controllers.runner.logger.exception", record_exception) + + uncloseable = UncloseableConnection() + runner = runner_for(LifecycleController(uncloseable), only=uncloseable) + await runner.start() + + await runner.stop() + + assert len(logged) == 1 + event, error = logged[0] + assert event == "Exception while closing connection" + assert isinstance(error, RuntimeError) + assert str(error) == "no" + + @pytest.mark.asyncio async def test_stop_cancels_the_tasks(): - controller = LifecycleController() - runner = ControllerRunner(controller) + connection = FakeConnection() + controller = LifecycleController(connection) + runner = runner_for(controller, only=connection) await runner.start() tasks = set(runner._tasks) assert tasks @@ -191,5 +671,112 @@ async def test_stop_cancels_the_tasks(): assert not runner._tasks -def test_reconnect_period_is_a_second_by_default(): - assert RECONNECT_PERIOD == 1.0 +def test_the_framework_connection_defaults(): + assert DEFAULT_RECONNECT_PERIOD == 1.0 + assert DEFAULT_RECONNECT_ATTEMPTS == 10 + + connection = FakeConnection() + assert connection.reconnect_period == DEFAULT_RECONNECT_PERIOD + assert connection.reconnect_attempts == DEFAULT_RECONNECT_ATTEMPTS + + +def test_a_class_default_sits_between_the_framework_and_the_constructor(): + class Patient(FakeConnection): + reconnect_period = 5.0 + reconnect_attempts = 60 + + assert Patient().reconnect_period == 5.0 + assert Patient().reconnect_attempts == 60 + assert Patient(reconnect_period=0.5).reconnect_period == 0.5 + assert Patient(reconnect_attempts=2).reconnect_attempts == 2 + + +@pytest.mark.asyncio +async def test_a_failed_build_closes_what_it_opened(): + """Startup aborts, and no task exists yet for a later `stop` to clean up.""" + connection = FakeConnection() + + class Unbuildable(Controller): + def __init__(self): + self.connection = connection + super().__init__() + + async def build(self): + raise RuntimeError("cannot build") + + runner = runner_for(Unbuildable(), only=connection) + + with pytest.raises(RuntimeError, match="cannot build"): + await runner.build() + + assert connection.closes == 1 + + +@pytest.mark.asyncio +async def test_a_failed_setup_closes_what_it_opened(): + connection = FakeConnection() + + class Unsetuppable(Controller): + def __init__(self): + self.connection = connection + super().__init__() + + async def setup(self): + raise RuntimeError("cannot set up") + + runner = runner_for(Unsetuppable(), only=connection) + + with pytest.raises(RuntimeError, match="cannot set up"): + await runner.start() + + assert connection.closes == 1 + + +@pytest.mark.asyncio +async def test_a_dependency_the_runner_does_not_supervise_is_rejected(): + """It would never be opened, so the dependent could never be attempted.""" + unsupervised = FakeConnection() + layered = FakeConnection(depends_on=unsupervised) + + runner = runner_for(LifecycleController(layered), layered=layered) + + with pytest.raises(ValueError, match="does not supervise"): + await runner.build() + + +@pytest.mark.asyncio +async def test_a_declared_connection_is_opened_even_with_no_controller_holding_it(): + """The declared list is the list: the runner never looks in the tree for one.""" + declared = FakeConnection() + + runner = runner_for(LifecycleController(None), spare=declared) + await runner.build() + try: + assert runner.connections == [declared] + assert declared.connects == 1 + finally: + await runner.stop() + + +@pytest.mark.asyncio +async def test_several_registries_are_supervised_together(): + """One registry per top-level entry, because role names are local to an entry.""" + first = FakeConnection() + second = FakeConnection() + + runner = ControllerRunner( + [LifecycleController(first), LifecycleController(second)], + [Connections({"device": first}), Connections({"device": second})], + ) + await runner.build() + try: + assert runner.connections == [first, second] + finally: + await runner.stop() + + +def test_a_connection_no_registry_names_falls_back_to_its_class(): + """A log line is never the place to raise, so naming has a fallback.""" + runner = runner_for(LifecycleController(None)) + + assert runner._name_of(FakeConnection()) == "FakeConnection" diff --git a/tests/test_controllers.py b/tests/test_controllers.py index fb8575c21..9c8da337d 100644 --- a/tests/test_controllers.py +++ b/tests/test_controllers.py @@ -4,6 +4,7 @@ import pytest from fastcs.attributes import AttrR, AttrRW, AttrW, Polled +from fastcs.connections import Connection from fastcs.controllers import Controller, ControllerVector from fastcs.methods import Command, Scan, command, scan @@ -374,26 +375,69 @@ async def scan_nothing(self): @pytest.mark.asyncio -async def test_scan_exception_sets_disconnected_and_reconnect_resumes(): +async def test_a_raising_scan_is_logged_and_retried(): + """A scan does not decide the connection is down - the connection's IO does.""" + calls = 0 + class MyTestController(Controller): @scan(0.01) async def failing_scan(self): + nonlocal calls + calls += 1 raise RuntimeError("scan error") controller = MyTestController() - controller.post_initialise() _, scan_coros, _ = controller.create_api_and_tasks() - controller._connected = True task = asyncio.create_task(scan_coros[0]()) - - # Wait long enough for the scan to run and raise, setting _connected = False await asyncio.sleep(0.1) - assert not controller._connected - # Trigger reconnect - _connected resumes scan tasks - await controller.reconnect() - assert controller._connected + assert calls > 1 + assert controller.connected # no connection to be down + + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + +@pytest.mark.asyncio +async def test_scans_wait_while_the_connection_is_down(): + class MyTestConnection(Connection): + async def connect(self) -> None: ... + async def close(self) -> None: ... + + connection = MyTestConnection() + calls = 0 + + class MyTestController(Controller): + def __init__(self): + self.connection = connection + super().__init__() + + @scan(0.01) + async def counting_scan(self): + nonlocal calls + calls += 1 + + controller = MyTestController() + _, scan_coros, _ = controller.create_api_and_tasks() + + task = asyncio.create_task(scan_coros[0]()) + + # Starts life down, so nothing runs until the framework marks it up + await asyncio.sleep(0.05) + assert calls == 0 + assert not controller.connected + + connection._set_connected() + await asyncio.sleep(0.05) + assert calls > 0 + + connection.set_disconnected() + await asyncio.sleep(0.02) + paused_at = calls + await asyncio.sleep(0.05) + assert calls == paused_at task.cancel() with pytest.raises(asyncio.CancelledError): diff --git a/tests/test_http_connection.py b/tests/test_http_connection.py new file mode 100644 index 000000000..2f55c3f8b --- /dev/null +++ b/tests/test_http_connection.py @@ -0,0 +1,129 @@ +import httpx +import pytest +import pytest_asyncio + +from fastcs.connections import HTTPConnection, HTTPConnectionSettings +from fastcs.connections.ip_connection import DisconnectedError + + +def _handler(request: httpx.Request) -> httpx.Response: + """A device that answers a couple of paths and complains about the rest.""" + if request.url.path == "/value": + return httpx.Response(200, json={"value": 1.5}) + if request.url.path == "/frame": + return httpx.Response(200, content=b"\x00\x01\x02") + if request.url.path == "/set": + return httpx.Response(200, json={"applied": True}) + if request.url.path == "/nothing": + return httpx.Response(204) + return httpx.Response(404, json={"error": "no such parameter"}) + + +class FakeDevice(HTTPConnection): + """Points the connection at an in-process handler rather than a socket.""" + + def __init__(self, handler=_handler, **kwargs) -> None: + super().__init__(**kwargs) + self._transport = httpx.MockTransport(handler) + + +@pytest_asyncio.fixture +async def device(): + connection = FakeDevice() + await connection.connect() + yield connection + await connection.close() + + +def test_settings_build_the_base_url(): + settings = HTTPConnectionSettings(host="detector", port=8080, scheme="https") + + assert settings.base_url == "https://detector:8080" + + +def test_the_defaults_are_a_local_http_device(): + assert HTTPConnectionSettings().base_url == "http://127.0.0.1:80" + + +@pytest.mark.asyncio +async def test_using_it_before_it_is_open_says_so(): + """The same failure as any other connection: not open is not a device fault.""" + with pytest.raises(DisconnectedError, match="connect"): + await FakeDevice().get("/value") + + +@pytest.mark.asyncio +async def test_get_returns_the_parsed_body(device: FakeDevice): + assert await device.get("/value") == {"value": 1.5} + + +@pytest.mark.asyncio +async def test_get_bytes_returns_the_raw_body(device: FakeDevice): + """Frame and file data is not JSON.""" + assert await device.get_bytes("/frame") == b"\x00\x01\x02" + + +@pytest.mark.asyncio +async def test_put_returns_the_body_when_there_is_one(device: FakeDevice): + assert await device.put("/set", {"value": 2.0}) == {"applied": True} + + +@pytest.mark.asyncio +async def test_put_returns_none_when_the_device_answers_with_nothing( + device: FakeDevice, +): + assert await device.put("/nothing", 1) is None + + +@pytest.mark.asyncio +async def test_an_error_status_is_a_device_complaint_not_a_dead_link( + device: FakeDevice, +): + """A 404 is the device rejecting one parameter, so the link stays up.""" + device._set_connected() # noqa: SLF001 + + with pytest.raises(httpx.HTTPStatusError): + await device.get("/missing") + + assert device.connected + + +@pytest.mark.asyncio +async def test_a_transport_failure_marks_the_connection_down(): + def refuse(request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("connection refused", request=request) + + connection = FakeDevice(refuse) + await connection.connect() + connection._set_connected() # noqa: SLF001 + + with pytest.raises(httpx.ConnectError): + await connection.get("/value") + + assert not connection.connected + + +@pytest.mark.asyncio +async def test_closing_twice_is_allowed(device: FakeDevice): + """`close` is called before every reconnect attempt, on whatever state.""" + await device.close() + await device.close() + + with pytest.raises(DisconnectedError): + await device.get("/value") + + +@pytest.mark.asyncio +async def test_a_subclass_reshapes_the_response_without_touching_health(): + """The Eiger case: the envelope is the device's convention, not HTTP's.""" + + class Unwrapping(FakeDevice): + async def get(self, path: str): + return (await super().get(path))["value"] + + connection = Unwrapping() + await connection.connect() + try: + assert await connection.get("/value") == 1.5 + finally: + await connection.close() diff --git a/tests/test_ip_connection.py b/tests/test_ip_connection.py index 3770174f1..3a50eac2a 100644 --- a/tests/test_ip_connection.py +++ b/tests/test_ip_connection.py @@ -2,7 +2,7 @@ import pytest -from fastcs.connections.ip_connection import IPConnection +from fastcs.connections.ip_connection import DisconnectedError, IPConnection @pytest.fixture @@ -50,3 +50,38 @@ async def test_close_connected_and_connection_reset(connection): await conn.close() assert conn._IPConnection__connection is None + + +@pytest.mark.asyncio +async def test_a_peer_that_closes_instead_of_answering_marks_the_link_down(): + """``readline`` returns b"" at EOF, which is a dead link, not an empty reply.""" + conn = IPConnection() + mock_stream = MagicMock() + mock_stream.__aenter__ = AsyncMock(return_value=mock_stream) + mock_stream.__aexit__ = AsyncMock(return_value=False) + mock_stream.send_message = AsyncMock() + mock_stream.receive_response = AsyncMock(return_value="") + conn._IPConnection__connection = mock_stream # pyright: ignore[reportAttributeAccessIssue] + conn._set_connected() + + with pytest.raises(DisconnectedError): + await conn.send_query("ID?\r\n") + + # Without this the caller just gets "", fails to parse it, and retries forever + # while the reconnect task stays idle. + assert not conn.connected + + +@pytest.mark.asyncio +async def test_a_real_response_leaves_the_link_up(): + conn = IPConnection() + mock_stream = MagicMock() + mock_stream.__aenter__ = AsyncMock(return_value=mock_stream) + mock_stream.__aexit__ = AsyncMock(return_value=False) + mock_stream.send_message = AsyncMock() + mock_stream.receive_response = AsyncMock(return_value="ID=1\r\n") + conn._IPConnection__connection = mock_stream # pyright: ignore[reportAttributeAccessIssue] + conn._set_connected() + + assert await conn.send_query("ID?\r\n") == "ID=1\r\n" + assert conn.connected diff --git a/tests/test_launch.py b/tests/test_launch.py index 987c6bff8..749469987 100644 --- a/tests/test_launch.py +++ b/tests/test_launch.py @@ -11,11 +11,13 @@ from fastcs import __version__ from fastcs.attributes import AttrR +from fastcs.connections import Connection, Connections, HTTPConnection from fastcs.control_system import FastCS from fastcs.controllers import Controller from fastcs.exceptions import LaunchError from fastcs.launch import ( _build_options_model, + _instantiate_controllers, _launch, get_controller_schema, launch, @@ -67,6 +69,52 @@ def __init__(self, arg: SomeConfig) -> None: super().__init__() +@dataclass +class LinkSettings: + host: str + port: int = 22 + + +class FakeConnection(Connection): + def __init__(self, settings: LinkSettings, **kwargs) -> None: + super().__init__(**kwargs) + self.settings = settings + + async def connect(self) -> None: ... + + async def close(self) -> None: ... + + +class OtherConnection(Connection): + type_name: ClassVar[str] = "other-connection" + + def __init__(self, label: str = "unlabelled", **kwargs) -> None: + super().__init__(**kwargs) + self.label = label + + async def connect(self) -> None: ... + + async def close(self) -> None: ... + + +class NeedsConnections(Controller): + """The common shape: a registry, and nothing else.""" + + def __init__(self, connections: Connections) -> None: + super().__init__() + self.claimed = connections.get("link", FakeConnection) + self.registry = connections + + +class NeedsBoth(Controller): + """`connections` alongside an options object - the two-argument case.""" + + def __init__(self, connections: Connections, arg: SomeConfig) -> None: + super().__init__() + self.registry = connections + self.arg = arg + + runner = CliRunner() @@ -320,3 +368,369 @@ def test_multi_controller_run_reaches_fastcs(mocker: MockerFixture, tmp_path): controllers_arg = init_spy.call_args.args[1] assert [c.path[0] for c in controllers_arg] == ["one", "two"] assert [type(c) for c in controllers_arg] == [IsHinted, OtherHinted] + + +# `connections:` in the config, and injection + + +def _build(controllers: list[dict], classes=None, connections=None) -> list[Controller]: + """Validate a `controllers:` list and instantiate it, as ``run`` does.""" + return _build_with_registries(controllers, classes, connections)[0] + + +def _build_with_registries( + controllers: list[dict], classes=None, connections=None +) -> tuple[list[Controller], list[Connections]]: + """As `_build`, but also the registries the launcher hands the runner.""" + options_model = _build_options_model( + classes or [NeedsConnections], connections or [FakeConnection] + ) + instance = options_model.model_validate( + {"controllers": controllers, "transport": [{"rest": {}}]} + ) + return _instantiate_controllers(_controllers(instance)) + + +def test_connections_are_declared_per_entry(): + """The key is the *role* the driver asks for; the entry identifies the + instance. Both entries claim "link" and get different objects - which a + single global block could not express.""" + first, second = _build( + [ + { + "id": "PITCH", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "192.168.0.1"}, + } + }, + }, + { + "id": "YAW", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "192.168.0.2", "port": 23}, + "reconnect_period": 5.0, + } + }, + }, + ] + ) + + assert isinstance(first, NeedsConnections) and isinstance(second, NeedsConnections) + assert first.claimed is not second.claimed + assert first.claimed.settings == LinkSettings(host="192.168.0.1", port=22) + assert second.claimed.settings == LinkSettings(host="192.168.0.2", port=23) + # Forwarded to `Connection` through the connection's own **kwargs + assert second.claimed.reconnect_period == 5.0 + + +def test_connections_alongside_an_options_object(): + """`connections` does not count towards the argument limit, so a controller + may take it *and* an options object.""" + (controller,) = _build( + [ + { + "id": "x", + "type": "tests.NeedsBoth", + "name": "a-name", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + } + }, + } + ], + classes=[NeedsBoth], + ) + + assert isinstance(controller, NeedsBoth) + assert controller.arg == SomeConfig(name="a-name") + assert len(controller.registry) == 1 + + +@pytest.mark.parametrize("declared", ["ssh", ["ssh"]]) +def test_depends_on_takes_a_name_or_a_list(declared): + (controller,) = _build( + [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + "depends_on": declared, + }, + "ssh": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + }, + }, + } + ] + ) + + assert isinstance(controller, NeedsConnections) + ssh = controller.registry.get("ssh", FakeConnection) + assert controller.claimed.depends_on == [ssh] + + +def test_depends_on_resolves_several_names(): + (controller,) = _build( + [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": { + "ssh": {"type": "tests.FakeConnection", "settings": {"host": "h"}}, + "status": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + }, + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + "depends_on": ["ssh", "status"], + }, + }, + } + ] + ) + + assert isinstance(controller, NeedsConnections) + assert [type(c).__name__ for c in controller.claimed.depends_on] == [ + "FakeConnection", + "FakeConnection", + ] + assert controller.claimed.depends_on == [ + controller.registry.get("ssh", FakeConnection), + controller.registry.get("status", FakeConnection), + ] + + +def test_unknown_depends_on_name_lists_the_declared_roles(): + with pytest.raises(LaunchError) as error: + _build( + [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + "depends_on": "typo", + } + }, + } + ] + ) + + assert "depends on 'typo', which is not declared" in str(error.value) + assert "Declared: ['link']" in str(error.value) + + +def test_depends_on_cycle_is_a_config_error(): + """Caught while the roles still have names, rather than deadlocking in the + reconnect tasks at runtime.""" + with pytest.raises(LaunchError, match="Cycle in `depends_on`"): + _build( + [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + "depends_on": "ssh", + }, + "ssh": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + "depends_on": "link", + }, + }, + } + ] + ) + + +def test_connections_for_a_controller_that_cannot_receive_them(): + with pytest.raises(LaunchError, match="no `connections` argument"): + _build( + [ + { + "id": "x", + "type": "tests.IsHinted", + "name": "n", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + } + }, + } + ], + classes=[IsHinted], + ) + + +def test_connections_is_a_reserved_options_field(): + """Reserved whether or not any Connection classes are registered, so that + registering one later cannot collide with an existing driver.""" + + @dataclass + class Colliding: + connections: str + + class Collides(Controller): + def __init__(self, arg: Colliding) -> None: + super().__init__() + + with pytest.raises(LaunchError, match="'connections' field"): + _build_options_model([Collides]) + + +def test_connection_type_discriminates_within_the_entry(): + (controller,) = _build( + [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": { + "link": { + "type": "tests.FakeConnection", + "settings": {"host": "h"}, + }, + "other": {"type": "other-connection", "label": "labelled"}, + }, + } + ], + connections=[FakeConnection, OtherConnection], + ) + + assert isinstance(controller, NeedsConnections) + assert controller.registry.get("other", OtherConnection).label == "labelled" + + +def test_unknown_connection_type_rejected(): + options_model = _build_options_model([NeedsConnections], [FakeConnection]) + with pytest.raises(ValidationError): + options_model.model_validate( + { + "controllers": [ + { + "id": "x", + "type": "tests.NeedsConnections", + "connections": {"link": {"type": "not.AConnection"}}, + } + ], + "transport": [{"rest": {}}], + } + ) + + +def test_no_connections_block_without_registered_classes(): + """Nothing could be declared, so the key is not in the schema at all.""" + schema = get_controller_schema(NeedsConnections) + entry = schema["$defs"]["NeedsConnectionsEntry"] + assert "connections" not in entry["properties"] + + +def test_a_single_connection_class_need_not_be_a_list(): + schema = get_controller_schema(NeedsConnections, [FakeConnection]) + assert "FakeConnectionConfig" in schema["$defs"] + + +def test_a_connection_argument_without_a_type_hint(): + class Unhinted(Connection): + def __init__(self, settings) -> None: + super().__init__() + + async def connect(self) -> None: ... + + async def close(self) -> None: ... + + with pytest.raises(LaunchError, match="Add a typehint for `settings`"): + _build_options_model([NeedsConnections], [Unhinted]) + + +def test_a_connection_taking_star_args(): + class Starred(Connection): + def __init__(self, *settings: str) -> None: + super().__init__() + + async def connect(self) -> None: ... + + async def close(self) -> None: ... + + with pytest.raises(LaunchError, match=r"`\*settings` cannot be expressed"): + _build_options_model([NeedsConnections], [Starred]) + + +def test_a_connection_argument_colliding_with_a_framework_key(): + class Colliding(Connection): + def __init__(self, type: str) -> None: # noqa: A002 + super().__init__() + + async def connect(self) -> None: ... + + async def close(self) -> None: ... + + with pytest.raises(LaunchError, match="collides with a launch-framework key"): + _build_options_model([NeedsConnections], [Colliding]) + + +def test_the_framework_http_connection_is_usable_from_config(): + """A REST driver names `fastcs.HTTPConnection` and writes no connection at all.""" + + class NeedsHTTP(Controller): + def __init__(self, connections: Connections) -> None: + super().__init__() + self.connection = connections.get("link", HTTPConnection) + + controllers = _build( + [ + { + "id": "OD", + "type": "tests.NeedsHTTP", + "connections": { + "link": { + "type": "fastcs.HTTPConnection", + "settings": {"host": "odin", "port": 8888}, + "reconnect_period": 5.0, + } + }, + } + ], + classes=[NeedsHTTP], + connections=[HTTPConnection], + ) + + connection = controllers[0].connection + assert isinstance(connection, HTTPConnection) + assert connection._settings.base_url == "http://odin:8888" + assert connection.reconnect_period == 5.0 + + +def test_connections_block_in_the_schema(): + schema = get_controller_schema(NeedsConnections, FakeConnection) + entry = schema["$defs"]["NeedsConnectionsEntry"] + assert entry["properties"]["connections"]["additionalProperties"] == { + "$ref": "#/$defs/FakeConnectionConfig" + } + + connection = schema["$defs"]["FakeConnectionConfig"] + assert connection["properties"]["type"]["const"] == "tests.FakeConnection" + # Forwarded `**kwargs` stand in for `Connection`'s own arguments + assert "reconnect_period" in connection["properties"] + assert "reconnect_attempts" in connection["properties"] + # Resolved after the block is built, so it is names here rather than objects + assert "depends_on" in connection["properties"] diff --git a/tests/test_multi_controller.py b/tests/test_multi_controller.py index 7b561773f..591d2afbc 100644 --- a/tests/test_multi_controller.py +++ b/tests/test_multi_controller.py @@ -10,6 +10,7 @@ from pytest_mock import MockerFixture from fastcs.attributes import AttrR +from fastcs.connections import Connection, Connections from fastcs.control_system import FastCS from fastcs.controllers import Controller from fastcs.transports.epics import EpicsDocsOptions, EpicsGUIOptions @@ -299,28 +300,38 @@ class names, so ``DEV-1`` and ``DEV_1`` would silently override each other in assert "'DEV_1'" in message +class _LifecycleConnection(Connection): + """Records whether the runner opened and closed the link.""" + + def __init__(self): + super().__init__() + self.open = False + + async def connect(self) -> None: + self.open = True + + async def close(self) -> None: + self.open = False + + class _LifecycleController(Controller): """Records lifecycle hook calls for end-to-end assertions.""" + connection: _LifecycleConnection + foo: AttrR[int] def __init__(self): + self.connection = _LifecycleConnection() super().__init__() - self.connect_called = False - self.initialised = False - self.post_initialised = False - - async def initialise(self): - self.initialised = True + self.built = False + self.set_up = False - def post_initialise(self): - self.post_initialised = True + async def build(self): + self.built = True - async def connect(self): - self.connect_called = True - - async def disconnect(self): - self.connect_called = False + async def setup(self): + self.set_up = True class _OtherLifecycleController(_LifecycleController): @@ -341,15 +352,25 @@ async def test_fastcs_serves_two_controllers_end_to_end(mocker: MockerFixture): # app directly through TestClient. mocker.patch.object(RestTransport, "serve", new=lambda self: asyncio.sleep(3600)) - fastcs = FastCS([a, b], [transport], asyncio.get_event_loop()) + # One registry per entry, as the launcher builds them: each controller + # declares its own link under the same local role name. + fastcs = FastCS( + [a, b], + [transport], + asyncio.get_event_loop(), + [ + Connections({"device": a.connection}), + Connections({"device": b.connection}), + ], + ) task = asyncio.create_task(fastcs.serve(interactive=False)) try: await asyncio.sleep(0.1) for controller in (a, b): - assert controller.initialised - assert controller.post_initialised - assert controller.connect_called + assert controller.built + assert controller.set_up + assert controller.connection.open with TestClient(transport._server._app) as client: assert client.get("/alpha/foo").status_code == 200 @@ -369,4 +390,4 @@ async def test_fastcs_serves_two_controllers_end_to_end(mocker: MockerFixture): pass for controller in (a, b): - assert not controller.connect_called + assert not controller.connection.open diff --git a/tests/transports/epics/pva/test_p4p.py b/tests/transports/epics/pva/test_p4p.py index ff63015c8..e89f61f22 100644 --- a/tests/transports/epics/pva/test_p4p.py +++ b/tests/transports/epics/pva/test_p4p.py @@ -712,8 +712,7 @@ async def get_a(self) -> int: controller = SeedController() controller.set_path([str(uuid4())]) - await controller.initialise() - controller.post_initialise() + await controller.build() controller_api, _, initial_coros = controller.create_api_and_tasks() attribute = controller_api.attributes["a"]