Convert adapters to use asyncio instead of the deprecated asynchat/asyncore

[kovacskristof]

Convert adapters to use asyncio instead of the deprecated asynchat/asyncore

Closes #320

[anderslindho]

Writing here since I was requested to have a look via e-mail.

This looks pretty nice (assuming it passes tests once the workflow is started), but I would personally like to first see test coverage for the modbus adapter, and maybe also for the "EPICS" adapter. And I think the EPICS adapter maybe has to be replaced to fully get rid of the legacy python asyncio interface - see #335.

[Tom-Willemsen]

The ruff CI failure is due to us using a central ruff.toml for all our python code. But I think to make it easier for people to contribute, I'll make this a project-local one shortly so that lewis stays more self-contained.

The pyright CI failure is because pyright doesn't quite understand the @has_log decorator. In other places, we forward-declare just the type of log, i.e.:

def __init__(...):
    self.log: logging.Logger
    ...

---

I will try to have a more detailed look through the changes and a functional test with some of our ISIS emulators over the coming days.

[Tom-Willemsen]

This looks directionally good - have left a few comments in the code about some more detailed code-level points.

[Tom-Willemsen]

This unsolicited_reply function is public API which can be called directly from emulators - see here for an example of an emulator that does this to emulate a device which sends a status packet once per second, unprompted.

This change from sync to async is then a breaking change for downstream emulators, as it would now need to be awaited. So we need to either:

• Find a way to wrap / keep the existing sync API to add an 'unsolicited message' to the queue
• Provide a different way to solve the same problem (sending unprompted messages from an emulator), and write release notes / a migration guide describing how to migrate an emulator which currently uses unsolicited_reply (like the one above) to the new scheme.

I think the current approach was always a bit of a hack, and we don't do this in very many downstream emulators, so I am quite open to changing the approach as long as the capability to send asynchronous messages remains.

[balintpeceli]

We haven't realized that this was a public API, thanks for pointing this out!

We tried to strategically keep the changes at the minimum and prefer backwards-compatibility over asyncio "modernity". The full adapter internals could be restructured to adopt asyncio concepts more naturally, but we intentionally avoided that. We would prefer to tackle that later in scope of a subsequent PR if really wanted.

Following that strategy we could have a look at the WriteTransport.write API and try to free the entire write-path from awaiting.

Could you please give us some instructions how to properly test this behavior with the appointed emulator or any other alternatives?

[Tom-Willemsen]

We tried to strategically keep the changes at the minimum and prefer backwards-compatibility over asyncio "modernity".

Yes, I think this is the right general approach. We'd certainly prefer to avoid large changes required on the emulator side for most emulators. However I am open to changes specifically to the "unsolicited command" mechanism on the emulator side, as we only have <5 emulators which actually use this.

To test with the emulator I linked:

• Check out the EPICS-deviceEmulator repository (mine is in c:\instrument\apps\epics\support\deviceemulator\master
• Run lewis with something like:

c:\Instrument\Apps\Python3\python.exe -m lewis -k lewis_emulators mecfrf -p "stream: {bind_address: 127.0.0.1, port: 57677}" -a c:\Instrument\Apps\EPICS\support\DeviceEmulator\master\

• With the emulator running, telnet to 127.0.0.1:57677; you should get a message including the bytes DATA once per second, without you sending anything. The rest of the message is in a horrible binary format for that device unfortunately.

[Tom-Willemsen]

Why do we need a create_task here if we are immediately awaiting it?

[balintpeceli]

Although in this case it's almost the same, we propose to create a task here for the following reasons:

• this way it clearly documents what tasks we want to schedule,
• asyncio docs warns us that "Unlike tasks, awaiting a coroutine does not hand control back to the event loop!" - which might cause troubles in the future if potentially new tasks will have to be scheduled

[Tom-Willemsen]

I am still not sure I fully understand, but can you try to write the explanation in a code comment (for future readers) and then I will think harder about whether this makes sense.

[Tom-Willemsen]

This doesn't work for devices which use an empty in_terminator, which should treat a timeout as a valid input termination.

Traceback (most recent call last):
  File "C:\Instrument\dev\lewis\lewis\adapters\stream.py", line 183, in handle_accept
    await handler.handle_client()
  File "C:\Instrument\dev\lewis\lewis\adapters\stream.py", line 52, in handle_client
    msg = await self._reader.readuntil(self._in_terminator)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Instrument\Apps\Python3\Lib\asyncio\streams.py", line 610, in readuntil
    raise ValueError('Separator should be at least one-byte string')
ValueError: Separator should be at least one-byte string

[balintpeceli]

This use case was indeed overlooked. The fix is on the way.

[Tom-Willemsen]

rstrip specifies a set of characters to be removed. So rstrip("\r\n") removes any trailing \r or \n characters, not just a "\r\n" sequence. You may want removesuffix instead.