numat / alicat

Python driver and command line tool for Alicat mass flow controllers.
GNU General Public License v2.0
21 stars 27 forks source link

unexpected behavior from is_connected method after 0.5.0 release #58

Closed Briley645 closed 1 year ago

Briley645 commented 1 year ago

Hello! I've been using this package inside my own automation system, and I wrote a script to find all of my connected MFCs addresses. The latest update broke my script at first through the name change between 'address' and 'unit'. I fixed that, but I'm getting unexpected behavior in the is_connected method. I'm posting a screenshot of the results using 0.4.0 and 0.5.0 where the only thing i'm changing in the script is unit/address. It seems like is_connected is no longer returning False for incorrect conditions? I tried looking through the commit history but it doesn't seem that the method code has changed since 0.4.0 besides the asynchio additions. I'm not sure why that would break this. Do you have thoughts? image

alexrudd2 commented 1 year ago

Hello, @Briley645. I'm glad this driver has been of use to you.

I've never used the is_connected() method so I don't notice breakage. Sorry!

The problem is that I changed that method to be async in cb6fdb3. That's why you're getting the error coroutine 'FlowMeter.is_connected' was never awaited'.
It has to be called with an await e.g. if await FlowController.is_connected([...])

alexrudd2 commented 1 year ago

I think your best option is to rewrite your connection tester script to run with asyncio.run() so it can use await.

import asyncio.  # <=== new import

    [...]
    print('Beginning search[...]'

    async def search():  # <=== new async function
        for port in comports:
            [...]
             if await FlowController.is_connected([...])  # <== new await to call the async coroutine
    [...]
    asyncio.run(search()) # <== new run() to start the "event loop"

Does this make sense?

Of course, you can always stay on 0.4.0 for the time being. Is there any particular feature you needed that became available in 0.5.0?

Briley645 commented 1 year ago

I see, it seems that my first if statement may have never actually evaluated since I didn't use the "await" command. I just installed my package on a new lab computer, and I hadn't pinned the alicat version in my requirements file so it installed with 0.5.0 and caused some issues.

Now I'm realizing that it seems all method calls would need to include the "await" command now? Thats a bit of a major change. I was already implementing multithreading by just creating a QT worker thread and making method calls to my MFCs within that. I think the best solution is to pin 0.4.0 and avoid changing everything to be compatible with asyncio.

Thanks for getting back!

alexrudd2 commented 1 year ago

Now I'm realizing that it seems all method calls would need to include the "await" command now? Thats a bit of a major change.

Essentially, yes. I only use the async code and can't maintain sync versions; sorry! That's why I listed it as a breaking change: https://github.com/numat/alicat/blob/95aafff59035205efa89ec603d55c83e367c4b3e/README.md?plain=1#L143-L146

It's theoretically possible to wrap all the coroutines with ensure_future and then use callbacks, but I don't have a good example of how to use that.

You may experience the following bug https://github.com/numat/alicat/issues/36, which was introduced in 0.4.1. It's fixed by https://github.com/numat/alicat/pull/37/commits/ceac13791a82aeece61115ffbac1a377846642b6 which is in 0.5.0. If this is really problematic I can probably release 0.4.2 to backport just that fix.

Briley645 commented 1 year ago

I actually did get that bug immediately in 0.4.1 and switched to 0.4.0. I saw in the commits it was fixed in 0.5.0, but I haven't had any issues in 0.4.0. I'm not sure if there are any release note somewhere between the different versions of what features have been changing. If there were any major updates a 0.4.2 could be cool, but it doesn't seem necessary from my end right now.

alexrudd2 commented 1 year ago

I actually did get that bug immediately in 0.4.1 and switched to 0.4.0. I saw in the commits it was fixed in 0.5.0, but I haven't had any issues in 0.4.0. I'm not sure if there are any release note somewhere between the different versions of what features have been changing. If there were any major updates a 0.4.2 could be cool, but it doesn't seem necessary from my end right now.

Ok, as long as you're happy on 0.4.0 I'm going to close this issue. Thanks, and sorry for the disruption!