DiamondLightSource / tickit

Event-based hardware simulation framework
Apache License 2.0
6 stars 0 forks source link

Adapter rework for Zebra #174

Closed abbiemery closed 1 year ago

abbiemery commented 1 year ago

This is a possible change to the way adapters work within tickit. This attempts unifies the adapters in a composed way by seperating out the IO and the device specific handling logic. The idea is that you use an AdapterContainer which has both an adapter (the part that handles the messages) and the io which does the rest.

The AdapterContainer acts as the previous adapters did and contains the run_forever method. Now though, this calls a setup method in the io in which the io takes the specific adapter that has been provided. The adapter and io are failry coupled given the io needs a specific adapter type to work, eg. class HttpIo(AdapterIo[HttpAdapter]):. However i was hoping this change provides a clearer divide in developer and user implemented code.

In the http example:

class HttpIo(AdapterIo[HttpAdapter]):
class MyDeviceSpecificAdapter(HttpAdapter):

myAdapterContainer.io = HttpIo
myAdapterContainer.adapter = MyDeviceSpecificAdapter

As before users would need to write specific adapters for their devices but this should simplifiy their lives a bit as all they now need is the device and device specific methods. For example the http adapter on an iobox:

class IoBoxHttpAdapter(HttpAdapter):
    """An adapter for an IoBox that allows reads and writes via REST calls"""

    io_box: IoBoxDevice

    def __init__(self, io_box: IoBoxDevice) -> None:
        self.io_box = io_box

    @HttpEndpoint.put("/memory/{address}", interrupt=True)
    async def write_to_address(self, request: web.Request) -> web.Response:
        address = request.match_info["address"]
        new_value = (await request.json())["value"]
        self.io_box.write(address, new_value)
        return web.json_response({address: new_value})

    @HttpEndpoint.get("/memory/{address}")
    async def read_from_address(self, request: web.Request) -> web.Response:
        address = request.match_info["address"]
        value = self.io_box.read(address)
        return web.json_response({address: value})

Then the component config:

@pydantic.v1.dataclasses.dataclass
class ExampleHttpDevice(ComponentConfig):
    """Example HTTP device."""

    host: str = "localhost"
    port: int = 8080

    def __call__(self) -> Component:  # noqa: D102
        device = IoBoxDevice()
        adapters = [
            AdapterContainer(
                IoBoxHttpAdapter(device),
                HttpIo(
                    self.host,
                    self.port,
                ),
            )
        ]
        return DeviceSimulation(
            name=self.name,
            device=device,
            adapters=adapters,
        )

Most importantly, and the motivation for this change, is there is no longer a limitation on what an adapter may apply to. This allows us to write an adapter which inspect the components in a system simulation. I put a quick example one I made in the example folder which used a tcpio and commandadapter.

class BaseSystemSimulationAdapter:
    """A common base adapter for system simulation adapters.

    They should be able to use any interpreter available.
    """

    _components: Dict[ComponentID, Component]
    _wiring: Union[Wiring, InverseWiring]

    def setup_adapter(
        self,
        components: Dict[ComponentID, Component],
        wiring: Union[Wiring, InverseWiring],
    ) -> None:
        self._components = components
        self._wiring = wiring

class SystemSimulationAdapter(BaseSystemSimulationAdapter, CommandInterpreter):
    """Network adapter for a generic system simulation.

    Network adapter for a generic system simulation using a command interpreter. This
    Can be used to query the system simulation component for a list of the
    ComponentID's for the components in the system and given a specific ID, the details
    of that component.
    """

    _byte_format: ByteFormat = ByteFormat(b"%b\r\n")

    @RegexCommand(r"ids", False, "utf-8")
    async def get_component_ids(self) -> bytes:
        """Returns a list of ids for all the components in the system simulation."""
        return str(self._components.keys()).encode("utf-8")

    ...

I'd really appreciate opinions on the idea before I commit to it and rewrite half the test suite. I'm particularly not certain on:

  1. The naming. It doesn't quite fit for me but i'm lacking the oversight and vocabulary to find something that feels right.
  2. The epics adapter, I don't think i really divided the io and adapter code well here, it all felt rather intertwined.
  3. The wrapper: I haven't figured out how to change these up yet, so they don't work. We COULD keep them wrapping or we could investigate the path of using a new specification(like regex) and maybe chain them there.
codecov[bot] commented 1 year ago

Codecov Report

Merging #174 (b47a673) into master (bd673a8) will decrease coverage by 0.58%. The diff coverage is 93.82%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   95.21%   94.64%   -0.58%     
==========================================
  Files          43       41       -2     
  Lines        1295     1306      +11     
==========================================
+ Hits         1233     1236       +3     
- Misses         62       70       +8     
Files Changed Coverage Δ
...rc/tickit/adapters/specifications/regex_command.py 96.15% <ø> (ø)
src/tickit/adapters/utils.py 100.00% <ø> (ø)
src/tickit/adapters/tcp.py 82.60% <65.00%> (ø)
src/tickit/core/components/system_simulation.py 94.11% <75.00%> (-3.61%) :arrow_down:
src/tickit/adapters/io/zeromq_push_io.py 84.93% <90.00%> (ø)
src/tickit/adapters/zmq.py 90.90% <90.90%> (ø)
src/tickit/core/adapter.py 91.30% <93.33%> (-0.37%) :arrow_down:
src/tickit/adapters/io/tcp_io.py 97.67% <97.67%> (ø)
src/tickit/adapters/epics.py 76.78% <100.00%> (ø)
src/tickit/adapters/http.py 100.00% <100.00%> (ø)
... and 7 more