Skip to content
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

Enhancement Request: Set ramp rate #127

Open
patr1ck opened this issue Jun 14, 2024 · 18 comments
Open

Enhancement Request: Set ramp rate #127

patr1ck opened this issue Jun 14, 2024 · 18 comments

Comments

@patr1ck
Copy link

patr1ck commented Jun 14, 2024

Alicat MFCs allow you to set a ramp rate on device – it would be great if we could set that through the python API as well.

Thanks!

@alexrudd2
Copy link
Member

See also #108

@alexrudd2
Copy link
Member

Relevant commands from the Alicat serial primer:
image
image

image
image

@alexrudd2
Copy link
Member

alexrudd2 commented Jun 18, 2024

@patr1ck Thanks for the enhancement suggestion. PR is welcome of course. I may be able to add some functionality here, but no longer work with Alicats regularly.

Regarding your use case:

  • Are you setting the ramp_ratemax_ramp and unit_time?
  • Do you need to configure the various ramp settings via the API? (ramp_up, ramp_down, zero_ramp, and power_up_ramp)
  • Are you comfortable testing code directly from a git branch, or only via pip install?

@patr1ck
Copy link
Author

patr1ck commented Jun 19, 2024

Hey Alex, Thanks so much for the response! I super appreciate the all work done and shared on this module so far – it's extremely helpful.

To answer your questions:

  • We would like to set both max_ramp (I think that's what you mean?) and unit_time
  • Of these options, we only really need ramp_up, ramp_down, zero_ramp. power_up_ramp would be nice but is not really needed.
  • I'm comfortable testing from a branch.

We've worked around this API not existing by adding simple ramping in our automation, but it would be awesome to rely on the device to do it instead of our own code. I probably don't have time to dig into the serial connection code and add it myself, but would be happy to test if it becomes available in the future.

@alexrudd2
Copy link
Member

After a bit of testing I realized that I do not have access to a device with firmware new enough for ramping (!). So I'll have to work a bit 'blind'

For the time being I have a small fix for #108 in https://github.com/alexrudd2/alicat/tree/ramp Would you mind testing that branch?

To know how this is being used (and for my own curiosity), do you mind sharing your organization and use case?

@patr1ck
Copy link
Author

patr1ck commented Jun 28, 2024

Wow, awesome, thank you Alex! I'll test this on Monday when our system is available.

I work for Ammobia and we use various Alicat devices in our experimental systems. We use Pyopticon instead of LabView to do a lot of our automation of those systems, and have written some Alicat widgets for Pyopticon which leverage this library. There's nothing proprietary about them, so we plan to publish them as soon as I can clean them up into a neat package. (They are mostly just glue between this library and Pyopticon anyways.)

@patr1ck
Copy link
Author

patr1ck commented Jul 1, 2024

I've tested the branch and set_ramp_config seems to work just fine on device! I'm not able to call get_ramp_config (as seen in the tests) because it looks like that doesn't exist in the branch, but maybe I'm not looking in the right place?

@alexrudd2
Copy link
Member

I've tested the branch and set_ramp_config seems to work just fine on device! I'm not able to call get_ramp_config (as seen in the tests) because it looks like that doesn't exist in the branch, but maybe I'm not looking in the right place?

That's good news! I didn't write get_ramp_config yet. :)

I work for Ammobia and we use various Alicat devices in our experimental systems

Cool tech! Don't forget to get the upgraded seals for pure NH3 streams. If you need a Python programmer with experience with laboratory and SCADA systems (in Central timezone), please contact me at alex <*AT> ruddick.tech.

@alexrudd2
Copy link
Member

Can you try get_ramp_config now?

Also can you let me know if the tests run against a real device? Just change the lines below.

# from alicat.driver import FlowController
from alicat.mock import FlowController
ADDRESS = '/dev/tty.usbserial-FTCJ5EK9'

from alicat.driver import FlowController

ADDRESS = '{YOUR SERIAL PORT}`

@patr1ck
Copy link
Author

patr1ck commented Jul 3, 2024

Sorry I've been a bit slower in testing this, the one system we have to do this on has been in use, but I hope to do it tomorrow.

@patr1ck
Copy link
Author

patr1ck commented Jul 11, 2024

Hey Alex, sorry this took so long, the long weekend kind of messed up our schedule. I tested it today and get_ramp_config on our device fails at driver.py:591. If I set_trace() there, it looks like values is ['1', '0'].

Separately, I had some trouble getting the tests to run due to our non-standard environment (Windows, anaconda, etc). I'm going to work on that and get back to you when I have a chance to fix that.

@alexrudd2
Copy link
Member

alexrudd2 commented Jul 11, 2024

Can you tell me what the Alicat returns? Either the Python line from line 586, or just the response from a serial terminal.

You can file other issues for Windows and/or Anaconda. No experience with (Ana)conda; sorry. Note that pytest-xdist isn't necessary (it only parallelizes tests to save time), so you could uninstall it on the off chance it's causing problems with conda's 'asyncio'

The code should be fine on Windows; it's been straightforward on similar drivers I can add a runner in the CI to check and already runs in CI :)

Most likely the problem is the serial port drivers, which Windows usually f@$# up. It's best to manually install drivers for your serial adapter chip (I recommend chips from FTDI). Sometimes the baud/parity are hardcoded in Device Manager, or it uses an unexpected COM port.

@alexrudd2
Copy link
Member

alexrudd2 commented Jul 11, 2024

Another option is to offload the serial onto an Ethernet/serial adapter. I'd recommend USConverters.com - although they're just white-labeled from Taiwan/China, the support and docs are very helpful. Their nicer models allow you to write serial commands using a web interface (don't forget to add CR). This also lets you poll from, say, a Linux server in a rack somewhere.

@patr1ck
Copy link
Author

patr1ck commented Jul 12, 2024

The system we want this for is in near constant use, so I found another MFC I was able to appropriate for my own use/testing.

Answer to your first question:

patrick@Patrick-Ammobia alicat % python test_alicat.py
> /Users/patrick/code/alicat/alicat/driver.py(589)get_ramp_config()
-> if not line or self.unit not in line:
(Pdb) line
'A 0 0 0 0'

Here's the full output of the tests:

     patrick@Patrick-Ammobia alicat % pytest tests
============================================= test session starts ==============================================
platform darwin -- Python 3.12.2, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/patrick/code/alicat
configfile: setup.cfg
plugins: cov-5.0.0, asyncio-0.23.7, xdist-3.6.1
asyncio: mode=Mode.AUTO
collected 12 items                                                                                             

tests/test_driver.py .F.......FFF                                                                        [100%]

=================================================== FAILURES ===================================================
______________________________________________ test_driver_cli[B] ______________________________________________

capsys = <_pytest.capture.CaptureFixture object at 0x1027cc800>, unit = 'B'

    @pytest.mark.parametrize('unit', ['A', 'B'])
    @mock.patch('alicat.FlowController', FlowController)
    def test_driver_cli(capsys, unit):
        """Confirm the commandline interface works with different unit IDs."""
>       command_line([ADDRESS, '--unit', unit])

tests/test_driver.py:19: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
alicat/__init__.py:88: in command_line
    asyncio.run(get())
../../.pyenv/versions/3.12.2/lib/python3.12/asyncio/runners.py:194: in run
    return runner.run(main)
../../.pyenv/versions/3.12.2/lib/python3.12/asyncio/runners.py:118: in run
    return self._loop.run_until_complete(task)
../../.pyenv/versions/3.12.2/lib/python3.12/asyncio/base_events.py:685: in run_until_complete
    return future.result()
alicat/__init__.py:71: in get
    state = await flow_controller.get()
alicat/driver.py:352: in get
    state = await super().get()
alicat/driver.py:125: in get
    line = await self._write_and_read(command)
alicat/driver.py:332: in _write_and_read
    await self._init_task
alicat/driver.py:318: in _init_control_point
    self.control_point = await self._get_control_point()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alicat.driver.FlowController object at 0x1027ce480>

    async def _get_control_point(self) -> str:
        """Get the control point, and save to internal variable."""
        command = f'{self.unit}R122'
        line = await self._write_and_read(command)
        if not line:
>           raise OSError("Could not read control point.")
E           OSError: Could not read control point.

alicat/driver.py:531: OSError
__________________________________________ test_ramp_config[config0] ___________________________________________

config = {'down': False, 'power': False, 'up': True, 'zero': True}

    @pytest.mark.parametrize('config', [
        {'up': True, 'down': False, 'zero': True, 'power': False},
        {'up': True, 'down': True, 'zero': False, 'power': True},
        {'up': False, 'down': False, 'zero': False, 'power': False},
        ])
    async def test_ramp_config(config):
        """Confirm changing the ramping configuration works."""
        async with FlowController(ADDRESS) as device:
            await device.set_ramp_config(config)
>           result = await device.get_ramp_config()

tests/test_driver.py:77: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alicat.driver.FlowController object at 0x101b42510>

    async def get_ramp_config(self) -> dict[str, bool]:
        """Get the setpoint ramp settings (firmware 10v05).
    
        `up`: whether the controller ramps when increasing the setpoint,
        `down`: whether the controller ramps when decreasing the setpoint,
                (this includes setpoints below 0 on bidirectional devices),
        `zero`: whether the controller ramps when establishing a zero setpoint,
        `power`: whether the controller ramps when using a power-up setpoint
        """
        command = f"{self.unit}LSRC"
        line = await self._write_and_read(command)
        if not line or self.unit not in line:
            raise OSError("Could not read ramp config.")
        values = line[6:].split(' ')
        if len(values) != 4:
>           raise OSError("Could not read ramp config.")
E           OSError: Could not read ramp config.

alicat/driver.py:592: OSError
__________________________________________ test_ramp_config[config1] ___________________________________________

config = {'down': True, 'power': True, 'up': True, 'zero': False}

    @pytest.mark.parametrize('config', [
        {'up': True, 'down': False, 'zero': True, 'power': False},
        {'up': True, 'down': True, 'zero': False, 'power': True},
        {'up': False, 'down': False, 'zero': False, 'power': False},
        ])
    async def test_ramp_config(config):
        """Confirm changing the ramping configuration works."""
        async with FlowController(ADDRESS) as device:
            await device.set_ramp_config(config)
>           result = await device.get_ramp_config()

tests/test_driver.py:77: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alicat.driver.FlowController object at 0x1027cd6a0>

    async def get_ramp_config(self) -> dict[str, bool]:
        """Get the setpoint ramp settings (firmware 10v05).
    
        `up`: whether the controller ramps when increasing the setpoint,
        `down`: whether the controller ramps when decreasing the setpoint,
                (this includes setpoints below 0 on bidirectional devices),
        `zero`: whether the controller ramps when establishing a zero setpoint,
        `power`: whether the controller ramps when using a power-up setpoint
        """
        command = f"{self.unit}LSRC"
        line = await self._write_and_read(command)
        if not line or self.unit not in line:
            raise OSError("Could not read ramp config.")
        values = line[6:].split(' ')
        if len(values) != 4:
>           raise OSError("Could not read ramp config.")
E           OSError: Could not read ramp config.

alicat/driver.py:592: OSError
__________________________________________ test_ramp_config[config2] ___________________________________________

config = {'down': False, 'power': False, 'up': False, 'zero': False}

    @pytest.mark.parametrize('config', [
        {'up': True, 'down': False, 'zero': True, 'power': False},
        {'up': True, 'down': True, 'zero': False, 'power': True},
        {'up': False, 'down': False, 'zero': False, 'power': False},
        ])
    async def test_ramp_config(config):
        """Confirm changing the ramping configuration works."""
        async with FlowController(ADDRESS) as device:
            await device.set_ramp_config(config)
>           result = await device.get_ramp_config()

tests/test_driver.py:77: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alicat.driver.FlowController object at 0x1027cfa40>

    async def get_ramp_config(self) -> dict[str, bool]:
        """Get the setpoint ramp settings (firmware 10v05).
    
        `up`: whether the controller ramps when increasing the setpoint,
        `down`: whether the controller ramps when decreasing the setpoint,
                (this includes setpoints below 0 on bidirectional devices),
        `zero`: whether the controller ramps when establishing a zero setpoint,
        `power`: whether the controller ramps when using a power-up setpoint
        """
        command = f"{self.unit}LSRC"
        line = await self._write_and_read(command)
        if not line or self.unit not in line:
            raise OSError("Could not read ramp config.")
        values = line[6:].split(' ')
        if len(values) != 4:
>           raise OSError("Could not read ramp config.")
E           OSError: Could not read ramp config.

alicat/driver.py:592: OSError

---------- coverage: platform darwin, python 3.12.2-final-0 ----------
Name                 Stmts   Miss  Cover
----------------------------------------
alicat/__init__.py      55     18    67%
alicat/driver.py       285    149    48%
alicat/mock.py          46     46     0%
alicat/util.py         141     67    52%
----------------------------------------
TOTAL                  527    280    47%

=========================================== short test summary info ============================================
FAILED tests/test_driver.py::test_driver_cli[B] - OSError: Could not read control point.
FAILED tests/test_driver.py::test_ramp_config[config0] - OSError: Could not read ramp config.
FAILED tests/test_driver.py::test_ramp_config[config1] - OSError: Could not read ramp config.
FAILED tests/test_driver.py::test_ramp_config[config2] - OSError: Could not read ramp config.
========================================= 4 failed, 8 passed in 8.09s ==========================================
patrick@Patrick-Ammobia alicat % 

@alexrudd2
Copy link
Member

Thanks! Hmm, I think this line:

values = line[6:].split(' ')

should be changed to:

values = line[2:].split(' ')

@patr1ck
Copy link
Author

patr1ck commented Jul 16, 2024

Nice, that seems to fix get_ramp_config():

patrick@Patrick-Ammobia alicat % python ./test_alicat.py 
{'up': True, 'down': True, 'zero': True, 'power': True}

@alexrudd2
Copy link
Member

Nice, that seems to fix get_ramp_config():

patrick@Patrick-Ammobia alicat % python ./test_alicat.py 
{'up': True, 'down': True, 'zero': True, 'power': True}

OK, great. That fix is incorporated in 0.6.3.

Now that we have the configuration working, I'll think about what the best way to set the ramp rates is. I think doing things incrementally is my best approach without the physical hardware. Sorry it takes longer.

Here's the full output of the tests:

That's good. There are essentially two failing tests.
(1) test_driver_cli[B] is safe to ignore since the device you've got attached has an address of A - it would pass if you changed to B. (I forgot to tell you to comment out that parameterize when checking against only one device).
(2) test_ramp_config[configXXX] should be fixed with the full implementation of get_ramp_config(). Can you confirm?

@patr1ck
Copy link
Author

patr1ck commented Jul 16, 2024

Gotcha, I removed 'B' and hit another error: the tests were failing because the dictionary being returned from get_ramp wasn't matching what was sent through set_ramp. I looked into it a bit more (because I could see the values changing on my device so it was clear set_ramp was working) and noticed a simple read error in get_ramp. Around L593 it should be:

return {
            'up': bool(int(values[0])),
            'down': bool(int(values[1])),
            'zero': bool(int(values[2])),
            'power': bool(int(values[3])),
        }

Now all tests pass :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants