-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Alicat BASIS devices #121
base: master
Are you sure you want to change the base?
Conversation
(only commenting on this aspect right now; I'll review the rest later). OK, great - that avoided most of the formatting issues from Notepad++. The remainder are because we run
I fixed the lint issues by pushing 22a4518 |
Got it, I'll start using ruff as well. Let me know how the rest of the review goes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed a bit, but still haven't made it to the main code yet. This should be reworked to avoid importing an entire module for one line.
import asyncio | ||
from typing import Any, ClassVar | ||
|
||
import nest_asyncio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complicated library is cute, but overkill just for one async
call in the constructor.
|
||
loop = asyncio.get_event_loop() | ||
tasks = asyncio.gather(_is_basis()) | ||
loop.run_until_complete(tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is necessary. It might make sense to put detection of Basis responses in the main driver and throw errors there.
Anyway, if it is necessary it can be done with asyncio.ensure_future
to get around the constructor not being able to use await
. I'm pretty sure that will finish before e.g. any reads are made. If not, it could then be await
ed by is_connected
since await
ing an already-completed task basically does nothing.
Is this making sense, or should I give an example or different explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alex, apologies for the late response. It has been a very busy week...
That's true (and I assume if anyone is importing the BASIS part of this library then they already have a BASIS device). I'll go ahead and mark that for deletion in the code. Let me know if there's anything else you think could be improved (I'm sure there is!) and I'll put everything into one pull request. Thank you again for being so patient!
The Alicat BASIS devices have many changes from the standard devices (the only closed loop variable available is mass flow, there is no 'D' term in the PID tuning, the data frame is different, etc). This updated driver allows most of the functionality expected from the standard device on the BASIS devices.
This has been tested on actual hardware and works. I've also slightly updated the example code in the readme, since that was for a TCP connection and most of our users use serial and were getting confused.
Let me know if there's anything I should change. I did use a more modern IDE this time so we shouldn't be seeing any issues with CR/LF etc.