Pull request for Alicat Python Library #15

marinapalese · 2020-05-15T17:52:35Z

Several functions were added to the flowmeter and flowcontroller classes. The functions added include most serial override commands, and well as a way to create and write gas mixes to the Alicat (introduced in R22 5v firmware and onward).

Override Commands Added: Volumetric & pressure tare, reset totalizer, valve hold and cancel valve hold, lock/unlock display.
set_gas() was edited to accommodate numerical and string inputs. This will give access the full gas list, and is the best method to set a gas mix that has been created. I did not make any efforts add additional gases to self.gases, since there's at least 60 that would need to be added and they are not often used.
I also changed the method of checking if the gas was set correctly by directly reading register 46. This register contains the gas in bits 1-256 (Air is zero), and zero dead band in bits 512-32768. By subtracting out the enabled high bits the gas can be read accurately. The current method has the possibility of incorrectly raising errors if there is any status code on the device. Directly checking register 46 also works well with both a numerical and string gas input.
In _set_setpoint in the FlowController class, current was change from float(line.split()[-2]) to float(line.split()[5]). Checking from the back, again, has the possibility of incorrectly calling an error if there is a status code on the device. The setpoint will always be index 5 from the front on a controller, so there is no issue. This method was not employed in set_gas() because if the device has a totalizer, the totalized flow will appear before the gas.

patrickfuller · 2020-05-15T18:31:05Z

Thanks for the PR! Reviewing will be a fun Friday afternoon distraction so I'll get you something back later today.

Regarding your comments on registers, I've always felt that this driver in general would be better if we moved away from using the space-separated API and relied more heavily on registers. That is, I like the path you're going down there and would be very open to more of it.

Thanks again!

patrickfuller

This is really cool! I have a lot of little comments that I hope you find educational, and I'm happy to discuss more if needed.

Hope this helps!

patrickfuller · 2020-05-15T19:06:23Z

README.md

@@ -80,10 +80,48 @@ You can also set the gas type and flow rate / pressure.

 ```python
 flow_controller.set_gas('N2')
+flow_controller.set_gas(8)         # Optionally set a gas by it's number; find the full gas table on page 52 of the Alicat manual.


Remove apostrophe - "its" - and move out from a code comment to the README itself. Something like "gases can be set by either their name or their index number" before this code block.

patrickfuller · 2020-05-15T19:09:26Z

README.md

 flow_controller.set_flow_rate(1.0)
 flow_controller.set_pressure(20)
 ```

+For firmwave 5v and greater, create and set gas mixes using COMPOSER software loaded into the device. Mixes can contain up to five gases, and are stored in gas indices 236-255. 


ware, not wave.

Is there a way to read the firmware version through the serial commands? We could use this on init to set features and raise errors if someone tries to use an unsupported function.

Nevermind, you already do this and it's great.

patrickfuller · 2020-05-15T19:09:38Z

README.md

+flow_controller.create_mix(mix_no=236, name="Mix1", gas1="N2", percent1=50, gas2="Ar", percent2=50)
+flow_controller.set_gas(236)
+flow_controller.delete_mix(236)
+```


This is really cool!!

patrickfuller · 2020-05-15T19:10:35Z

README.md

+flow_controller.write_PID_P(4000)
+flow_controller.write_PID_D(10)
+flow_controller.write_PID_I(4000)
+print(flow_controller.read_PID())


This is also cool! I've been meaning to add PID tweaking to this code since day 1, so it's great to see it here.

alicat/serial.py

patrickfuller · 2020-05-15T19:40:49Z

alicat/serial.py

+                  ' {g5}\r'.format(addr=self.address, shortName=name, mixNumber=mix_no, p1=percent1,
+                                     g1=self.gases.index(gas1), p2=percent2, g2=self.gases.index(gas2),
+                                     p3=percent3 if gas3 is not None else "", g3=self.gases.index(gas3) if gas3 is not None else "", p4=percent4 if gas4 is not None else "",
+                                     g4=gas4 if gas4 is not None else "", p5=percent5 if gas5 is not None else "", g5=self.gases.index(gas5) if gas5 is not None else "")


This could also be cleaner and made to not trail so many spaces.

gas_list = ' '.join(' '.join(self.gases.index(gas), percent) for gas, percent in gases.items()) command = ' '.join(self.address, name, mix_no, gas_list) + '\r'

Double join read a little awkward but it avoids you repeating yourself. Could replace the inner join with a small format string as well to try helping readability.

patrickfuller · 2020-05-15T19:42:33Z

alicat/serial.py

+        command = '{addr}$$C\r'.format(addr=self.address)
+        self._write_and_read(command, retries)
+
+    def read_PID(self, retries=2):


You might get yelled at by pycodestyle for caps in the function name. The general rule is code style supersedes acronym capitalization, so it could push for read_pid instead.

Personally, I think it's fine either way.

patrickfuller · 2020-05-15T19:44:29Z

alicat/serial.py

+        if spl[3] == '2':
+            loop_type = 'PD2I'
+        elif spl[3] == '1' or spl[3] == '0':
+            loop_type = 'PD/PDF'


Nitpick here. I'd write as

loop_type = ['PD', 'PDF', 'PD2I'].index(int(spl[3]))

This way, you'll get better errors when spl[3] is malformed.

Readability here. Do in two lines even if one would work.

loop_num = int(spl[3]) loop_type = ['PD/PDF', 'PD/PDF', 'PD2I'].index(loop_num)

patrickfuller · 2020-05-15T19:46:42Z

alicat/serial.py

+        if looptype == 'PD/PDF':
+            command = '{addr}$$w85=1\r'.format(addr=self.address)
+        elif looptype == 'PD2I':
+            command = '{addr}$$w85=2\r'.format(addr=self.address)


Same here.

command = '{addr}$$w85={loop_num}\r'.format( addr=self.address, loop_num=['', 'PD/PDF', 'PD2I'].index(looptype) )

patrickfuller · 2020-05-15T19:47:57Z

alicat/tcp.py

+        else:
+            num = False
+
+        if num is True:


Same comments all apply here.

I'm thinking about rewriting this so we don't have to repeat ourselves in the code, but unfortunately should copy it all over for now.

marinapalese · 2020-05-18T19:47:41Z

This is really cool! I have a lot of little comments that I hope you find educational, and I'm happy to discuss more if needed.

Hope this helps!

Thank you for the feedback! I will work on making the changes as time allows. Thanks again!

marinapalese · 2022-01-03T22:06:17Z

Hi Patrick! I'm sorry this took so long.... some things happened when COVID started really affecting us and I wasn't allowed to work on this for a long time. I've dusted this off and made all the changes you requested now that I've finally been able to work and this and got all my other projects out of the way.

If you wouldn't mind taking another look, I'd really appreciate it. Happy new year!

patrickfuller · 2022-01-04T17:50:27Z

@marinapalese Happy new year and no worries! It's been a wild couple of years for all of us.

I'll review ASAP. Also - any chance you're able to test this driver on an array of MFCs? (We could probably string something together at our company but I figured you might have access to a nicer test setup!)

patrickfuller

@marinapalese hope this helps! I'm being extra nitpicky in the hope of showing off the "pythonic" way of doing things. None of my comments really matter if we can test and show this works on a few live devices, but I'm still hoping you find it helpful to read through!

patrickfuller · 2022-01-04T17:54:12Z

alicat/serial.py

            del values[-1]
-
+        print(values)


Delete print statement.

If you want to print something out in production code, I recommend the logging module.

import logging logger = logging.getLogger('alicat')

logger.debug(f'Read values: {values}')

This allows the end user to turn the print statements on and off depending on what they're looking for.

This all said, I don't think that's the intent of this print statement. If not, it should just be deleted.

Ahh, I was just trying to quickly check something here and forgot to delete that. Thank you for telling me about the logging module regardless!

patrickfuller · 2022-01-04T17:55:02Z

alicat/serial.py

+
+        if number != reg46_gasbit:
+            raise IOError("Cannot set gas.")
+        pass


Delete pass

patrickfuller · 2022-01-04T17:55:41Z

alicat/serial.py

+        else:
+            return self._set_gas_name(gas, retries)
+
+    def _set_gas_number(self, number, retries):


Recommend adding a docstring to say what this is doing. Something like:

"""Set flow controller gas type by number. See the Alicat manual for usage. """

patrickfuller · 2022-01-04T17:56:30Z

alicat/serial.py

+        reg46 = self._write_and_read('{addr}$$R46\r'.format(
+            addr=self.address
+        ), retries)
+        reg46_gasbit = int(reg46.split()[-1]) & 0b0000000111111111


patrickfuller · 2022-01-04T17:57:44Z

alicat/serial.py

+            raise IOError("Cannot set gas.")
+        pass
+
+    def _set_gas_name(self, name, retries):


Docstring here too. Something like

"""Set gas type by name. See supported gases in `FlowController.gases`. """

patrickfuller · 2022-01-04T18:05:03Z

alicat/serial.py

+    def lock(self, retries=2):
+        """Lock the display."""
+        self._test_controller_open()
+        command = '{addr}$$L\r'.format(addr=self.address)
+        self._write_and_read(command, retries)
+
+    def unlock(self, retries=2):
+        """Unlock the display."""
+        self._test_controller_open()
+        command = '{addr}$$U\r'.format(addr=self.address)
+        self._write_and_read(command, retries)
+
+    def tare_pressure(self, retries=2):
+        """Tare the pressure."""
+        self._test_controller_open()
+
+        command = '{addr}$$PC\r'.format(addr=self.address)
+        line = self._write_and_read(command, retries)
+
+        if line == '?':
+            raise IOError("Unable to tare pressure.")
+
+    def tare_volumetric(self, retries=2):
+        """Tare volumetric flow."""
+        self._test_controller_open()
+        command = '{addr}$$V\r'.format(addr=self.address)
+        line = self._write_and_read(command, retries)
+
+        if line == '?':
+            raise IOError("Unable to tare flow.")


These are great features. Thanks for adding!

patrickfuller · 2022-01-04T18:06:14Z

alicat/serial.py

+        if any([self.control_point is not None and
+                self.control_point == 'abs pressure',
+                self.control_point is not None and
+                self.control_point == 'gauge pressure',
+                self.control_point is not None and
+                self.control_point == 'diff pressure']):


Hm, how about

if self.control_point in ['abs pressure', 'gauge pressure', 'diff pressure']:

Good point! For some reason I remember trying this and having trouble with it. Just changed it and no issues with any device.

patrickfuller · 2022-01-04T18:09:19Z

alicat/serial.py

+        if any([self.control_point is not None and
+                self.control_point == 'mass flow',
+                self.control_point is not None and
+                self.control_point == 'vol flow']):


if self.control_point in ['mass flow', 'vol flow']:

patrickfuller · 2022-01-04T18:10:14Z

alicat/serial.py

+        value = p_value
+        command = '{addr}$$w21={v}\r'.format(addr=self.address, v=value)


You could save a line on this and just do:

command = '{addr}$$w21={v}\r'.format(addr=self.address, v=p_value)

Also applies to below functions.

patrickfuller · 2022-01-04T18:11:51Z

alicat/tcp.py

@@ -14,8 +14,8 @@
 class FlowMeter(object):


This is on me to fix. Having to copy-paste the same code twice is super messy. Let's focus on getting the serial.py file working and then I'll commit to putting time into refactoring so you don't need to repeat your code.

Definitely. I really appreciate you putting in the time for refactoring. Also, I will admit that I do not have a setup for testing a TCP connection so only serial.py is thoroughly tested by me.

alexrudd2 · 2022-01-04T19:07:30Z

alicat/__init__.py

@@ -19,7 +19,8 @@ def command_line():
                        "to read devices routed through a converter.")
    parser.add_argument('--address', '-a', default='A', type=str, help="The "
                        "device address, A-D. Should only be used if multiple "


This was there all along, but might as well fix it now. I believe they go A-Z not A-D

Yup it is A-Z! I changed it.

marinapalese · 2022-01-04T19:23:11Z

@marinapalese Happy new year and no worries! It's been a wild couple of years for all of us.

I'll review ASAP. Also - any chance you're able to test this driver on an array of MFCs? (We could probably string something together at our company but I figured you might have access to a nicer test setup!)

I have a box of controllers with all our main firmware except 4v - can't remember when that was "borrowed". Initially tested all the code for each of the firmware except that one, and then I've been testing the refactored version with our new 9v firmware.

marinapalese · 2022-01-04T20:01:16Z

@marinapalese hope this helps! I'm being extra nitpicky in the hope of showing off the "pythonic" way of doing things. None of my comments really matter if we can test and show this works on a few live devices, but I'm still hoping you find it helpful to read through!

Thank you so much! I can't express enough how much I appreciate you taking the time to show me the pythonic way. I did go through and make all the changes you recommended, as well as test them on some of my devices.

@marinapalese

See #15. Thanks @marinapalese!

patrickfuller · 2022-01-05T03:53:01Z

I made a few tweaks and uploaded to PyPI.

@marinapalese - can you confirm it works by updating your driver (pip install --upgrade alicat) and testing on your MFCs?

@alexrudd2 - I deleted the TCP driver and added a note that we can re-add it in if needed. If we end up needing it, we'll want to follow #16 to separate out the transport (serial, TCP, etc) and protocol (the strings the Alicat expects) so we're not repeating ourselves.

Thanks again!

alexrudd2 · 2022-01-05T04:37:57Z

@alexrudd2 - I deleted the TCP driver and added a note that we can re-add it in if needed. If we end up needing it, we'll want to follow #16 to separate out the transport (serial, TCP, etc) and protocol (the strings the Alicat expects) so we're not repeating ourselves.

Thanks again!

?!? The TCP driver was the only one I ever used! It was powering the pilot-lab. "Fortunately" we've now killed that controller and replaced it with kilo-lab which uses a P2000 to talk to the Alicats. So now we don't actually use this driver at all in production.

patrickfuller · 2022-01-05T04:50:02Z

Great! I left a comment in telling people to let us know if they're using the TCP driver. It's a couple hours of refactor if we want to add it back in right. Not a big deal but not a priority if it's going unused.

marinapalese · 2022-01-05T23:31:12Z

I made a few tweaks and uploaded to PyPI.

@marinapalese - can you confirm it works by updating your driver (pip install --upgrade alicat) and testing on your MFCs?

@alexrudd2 - I deleted the TCP driver and added a note that we can re-add it in if needed. If we end up needing it, we'll want to follow #16 to separate out the transport (serial, TCP, etc) and protocol (the strings the Alicat expects) so we're not repeating ourselves.

Thanks again!

@patrickfuller I checked it out and everything works with some small things.

README.md lists the function get_PID() when it is actually get_pid(), causing it not to work. It works fine once I call the actual function.
This was probably there before, but README.md also says "Tare volumetric hold." instead of "Tare volumetric flow." while describing other features.
All versions of the firmware work great, except GP. I have mixed feelings about this since it's 10 years old now and these devices aren't circulating much. The loop type can't be changed (and register 85 doesn't exist yet) so that messes get_pid(), though set_pid() can be used if the loop isn't specified. The totalizer command is missing the dollar signs ('{addr}T\r' instead of '{addr}$$T\r' so it doesn't like this. Only GP actually uses the dollar signs, though everything else is backwards compatible. And lastly, the {addr}VE command also wasn't around yet, so GP isn't recognized to stop it from continue to create_mix() function. {addr}??m8 would work, but only for GP which is annoying.

patrickfuller · 2022-01-06T18:24:07Z

@marinapalese thanks for reviewing! I updated the README to handle the first two points.

Your third point is related to #14 and could be worth thinking through further. I'd recommend moving the conversation to that thread but here's a brief overview:

Ideally, FlowController would ask for the connected device's version (and maybe other properties like max flow) on initialization. The class would then select from a series of subclasses (ie. FlowControllerGP) that implement all the features available for that specific device version. This is more complicated but provides back-compatibility and modern device features without compromising on either. There are many patterns (mixins w/ protocol vs. transport subclasses, yaml config file containing protocols, etc) that could be tried out based on how unique the handling needs to be.

Also ideally, we'd read/write the registers directly for all functions. Not only does this open up more features (as you've already shown in this PR!) but it allows greater accuracy for getting/setting flow rates on more modern MFCs.

This is a full library rewrite... but this is a small library so it's not out of the question. What would be needed is 1. a detailed list of all Alicat MFC versions and their communication protocols and 2. a good hardware test system with all supported MFCs that we could build a real test suite against.

If it's something you're interested in leading or helping out with, let me know and I'd be happy to find a way to support.

Marina Palese and others added 9 commits March 27, 2020 15:31

alicat/serial.py

16b8bbb

alicat/serial.py

02543a4

alicat/__init__.py

73c7eb6

alicat/serial.py

8e45ec1

alicat/serial.py

ba4824a

alicat/serial.py

205bae3

Update README.md

e622141

alicat/serial.py

fe3006a

Merge branch 'master' of https://github.com/marinapalese/alicat

600de27

patrickfuller suggested changes May 15, 2020

View reviewed changes

Marina Palese and others added 7 commits November 4, 2021 16:15

serial.py

a53d044

added support for pressure

c184e3c

added support for pressure devices

35cb352

added pressure support

d4a4a0e

Merge branch 'numat:master' into master

ce11b04

Updated install

590e69d

alicat/serial.py

dcbf2e3

patrickfuller suggested changes Jan 4, 2022

View reviewed changes

alexrudd2 reviewed Jan 4, 2022

View reviewed changes

marinapalese added 3 commits January 4, 2022 12:45

alicat/serial.py

d486169

alicat/serial.py

3d791c1

alicat/__init__.py

26adb66

patrickfuller merged commit e623b74 into numat:master Jan 5, 2022

patrickfuller added a commit that referenced this pull request Jan 5, 2022

Bump version to add new features.

9f5c067

See #15. Thanks @marinapalese!

patrickfuller mentioned this pull request Jan 5, 2022

Where is tare flow ? #24

Closed

alexrudd2 mentioned this pull request Jan 6, 2022

Handle very old (GP) firmware more robustly #27

Open

alexrudd2 mentioned this pull request Apr 4, 2022

"OSError: Could not read from flow controller" when setting setpoints on GP firmware #28

Open

		value = p_value
		command = '{addr}$$w21={v}\r'.format(addr=self.address, v=value)

Pull request for Alicat Python Library #15

Pull request for Alicat Python Library #15

Conversation

marinapalese commented May 15, 2020 • edited by patrickfuller Loading

patrickfuller commented May 15, 2020

patrickfuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfuller May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marinapalese commented May 18, 2020

marinapalese commented Jan 3, 2022

patrickfuller commented Jan 4, 2022

patrickfuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marinapalese commented Jan 4, 2022

marinapalese commented Jan 4, 2022

patrickfuller commented Jan 5, 2022

alexrudd2 commented Jan 5, 2022

patrickfuller commented Jan 5, 2022

marinapalese commented Jan 5, 2022

patrickfuller commented Jan 6, 2022

marinapalese commented May 15, 2020 •

edited by patrickfuller

Loading

patrickfuller May 15, 2020 •

edited

Loading