yaq-project / yaq-python

Repository for yaq core python packages.
https://yaq.fyi
GNU Lesser General Public License v3.0
5 stars 5 forks source link

Use `aioserial` instead of `aserial` #69

Open kadykov opened 1 year ago

kadykov commented 1 year ago

Probably, it would be better to use existing projects like aioserial to handle asyncio operations with serial ports...

ksunden commented 1 year ago

I know when I implemented the original aserial module (which was briefly its own package entirely) I tried a couple "async serial" implementations and found all of them at that time (which was a couple years ago now, so things may have changed in the mean time) to not quite work like I'd wanted.

In particular, I think the following patterns which are needed for some (though not all) hardware interfaces were not easy to support :

aserial is a really light wrapper around serial.Serial and is not much of a maintenance burden. As such, I'm not too interested in removing the functionality nor reworking existing daemons that are working. That said, if it works, new work can proceed to use aioserial, I have no issue with that.

kadykov commented 1 year ago

Thank you for your answer. I have nothing against aserial, and I think that awrite_then_read is quite useful. I just think that it is better to concentrate the efforts on already existing solutions to have at least one reliable tool.

Would it be better to make aserial a separate project so the people can use it without yaq?

I see only two issues with aserial: 1) Hardcoded 10 ms delay await asyncio.sleep(0.01) could be too much for certain equipment. 2) timeout=0 could break the initialization that requires reading in __init__ like this:

class DaemonName(HasPosition, IsDaemon):
    _kind = "DaemonName"

    def __init__(self, name: str, config: Dict[str, Any], config_filepath: Path):
        super().__init__(name, config, config_filepath)
        eol = b"\x03"
        self._ser = aserial.ASerial(
            self._config["serial_port"], baudrate=self._config["baud_rate"], eol=eol
        )
        self._ser.write(b"GET READY!")
        # self._ser.timeout = 0.1
        while True:
            sleep(0.001)
            self._ser.write(b"ARE YOU READY?")
            if self._ser.read_until(expected=eol) != b"YES, I'M READY":
                break
        # self._ser.timeout = 0
untzag commented 1 year ago

By the way, aioserial uses threads :vomiting_face:

untzag commented 1 year ago

@kadykov for your timeout question, what about the following pattern?

class DaemonName(HasPosition, IsDaemon):
    _kind = "DaemonName"

    def __init__(self, name: str, config: Dict[str, Any], config_filepath: Path):
        super().__init__(name, config, config_filepath)
        eol = b"\x03"
        self._ser = aserial.ASerial(
            self._config["serial_port"], baudrate=self._config["baud_rate"], eol=eol
        )
        self._ready = False
        self._loop.create_task(self._ensure_ready())

    async def _ensure_ready(self):
        while True:
            response = await self._ser.awrite_then_readline(b"GET READY!")
            if response == b"YES, I'M READY":
                self._ready = True
                break
            else:
                await asyncio.sleep(1)

We do something similar for yaqd-new-era-continuous-nextgen

https://github.com/yaq-project/yaqd-new-era/blob/main/yaqd_new_era/_new_era_continuous_nextgen.py#L39

untzag commented 1 year ago

@kadykov I think if you can find an existing package that implements awrite then read functionality we would be happy to deprecate aserial.

Or perhaps you could contribute that functionality to an existing package.

Always happy to push functionality as far upstream as possible!