search

pyvisa / pyvisa-py

A pure python PyVISA backend
https://pyvisa-py.readthedocs.io
MIT License
282 stars 120 forks source link

replace the call to serial.Serial with serial.serial_for_url #386

Closed LongnoseRob closed 1 year ago

LongnoseRob commented 1 year ago

to support loop:// and other url-types supported in pyserial (>3.0)

tested with the follwing code on python 3.11.3, pyserial 3.5:

import pyvisa
rm = pyvisa.ResourceManager("@py")
abc = rm.open_resource("ASRLloop://::INSTR")
abc.timeout = 3000
abc.read_termination = "\r"
abc.write_termination = "\r\n"
abc.write("Test1234567890")
print(abc.read())
MatthieuDartiailh commented 1 year ago

Could add an entry to the docs ? Otherwise this looks good since we require pyserial >= 3.0 and this feature was introduced in 2.5

LongnoseRob commented 1 year ago

Could add an entry to the docs ? Otherwise this looks good since we require pyserial >= 3.0 and this feature was introduced in 2.5

Added some documentation, hope this is what you were thinking about.

What I am not sure about is how to add a test :thinking:

MatthieuDartiailh commented 1 year ago

Thanks for the doc update I believe it will do. My only remaining concern is the behavior one gets if pyvisa attempts to read/write an attribute which is not supported for a URL. I did not dive deep enough in pyserial docs to have a clear sense of what would happen then.

As for testing since you can talk to the loop back you could mock a resource no ?

LongnoseRob commented 1 year ago

Thanks for the doc update I believe it will do. My only remaining concern is the behavior one gets if pyvisa attempts to read/write an attribute which is not supported for a URL. I did not dive deep enough in pyserial docs to have a clear sense of what would happen then.

If it try an assert_trigger() on a loop:// resource, this causes a NotImplementedError If I try a read_stb(), this causes a VisaIOError: VI_ERROR_NSUP_OPER (-1073807257): The given session or object reference does not support this operation.

As for testing since you can talk to the loop back you could mock a resource no ?

WIll work on that next

LongnoseRob commented 1 year ago

added a unit test, but it seems i cannot trigger the test(s) from the cli on two systems with python 3.11.3 or 3.11.4:

~/builds/pyvisa-py$ python -m unittest pyvisa_py/testsuite/test_serial.py                                                                           |

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
MatthieuDartiailh commented 1 year ago

I may have switched to pytest at one point I will have to double check.

LongnoseRob commented 1 year ago

I may have switched to pytest at one point I will have to double check.

pytest seems to work (and also pointed out a deprecatuion issue..

MatthieuDartiailh commented 1 year ago

CIs are not good. You may need to skip the new tests in some cases (note I did not check the exact failure).

LongnoseRob commented 1 year ago

CIs are not good. You may need to skip the new tests in some cases (note I did not check the exact failure).

The CI failed due to me using .append instead of directly setting the value. Could you trigger a new CI-run?

LongnoseRob commented 1 year ago

Could it be that in the CI enviromnent no pyserial is available and therefore the test fails?

Proof:

testsuite/test_serial.py F [100%]

================================================================================================================= FAILURES ================================================================================================================== __ TestSerial.test_serial ___

self = <pyvisa_py.testsuite.test_serial.TestSerial object at 0x7fbf0e7fbdd0>

def test_serial(self):
    """Test loop://"""
    msg = b"Test01234567890"

    available = ["loop://"]
    expected = []
    exp_missing = []
    missing = {}

    rm = ResourceManager("@py")
    try:
        dut = rm.open_resource("ASRLloop://::INSTR")
        print("opened")
        dut.timeout = 3000
        dut.read_termination = "\r\n"
        dut.write_termination = "\r\n"
        dut.write(str(msg))
        ret_val = dut.read()
        if str(msg) == ret_val:
            expected = ["loop://"]

    except Exception:
        exp_missing = ["loop://"]
  assert sorted(available) == sorted(expected)

E AssertionError: assert ['loop://'] == [] E Left contains one more item: 'loop://' E Use -v to get more diff

testsuite/test_serial.py:39: AssertionError

- install pyserial

(ptest) [robby@octoberrain pyvisa_py]$ pip list Package Version Editable project location

coverage 7.2.7 iniconfig 2.0.0 packaging 23.1 pip 22.3.1 pluggy 1.2.0 pyserial 3.5 <---- pytest 7.4.0 pytest-cov 4.1.0 pytest-cover 3.0.0 pytest-coverage 0.0 PyVISA 1.13.1.dev31+g071fa6d /home/robby/builds/pyvisa PyVISA-py 0.7.1.dev8+g5e0e5d6 /home/robby/builds/pyvisa-py setuptools 65.5.0 typing_extensions 4.7.1

- run test again:

(ptest) [robby@octoberrain pyvisa_py]$ pytest testsuite/test_serial.py ============================================================================================================ test session starts ============================================================================================================ platform linux -- Python 3.11.3, pytest-7.4.0, pluggy-1.2.0 rootdir: /home/robby/builds/pyvisa-py configfile: pyproject.toml plugins: cov-4.1.0 collected 1 item

testsuite/test_serial.py . [100%]



--> We need to extend the CI env to include pyserial for the tests
Can I do this in this PR or should this be done in anotherway?
MatthieuDartiailh commented 1 year ago

I am fine with you adding to the CI env but I would also like to see the test behind a conditional skip if pyserial is not available.

LongnoseRob commented 1 year ago

I am fine with you adding to the CI env but I would also like to see the test behind a conditional skip if pyserial is not available.

codecov-commenter commented 1 year ago

Codecov Report

Merging #386 (cdd8d32) into main (c33f4f2) will decrease coverage by 0.03%. Report is 1 commits behind head on main. The diff coverage is 18.51%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   23.05%   23.02%   -0.03%     
==========================================
  Files          22       23       +1     
  Lines        3292     3318      +26     
  Branches      456      457       +1     
==========================================
+ Hits          759      764       +5     
- Misses       2516     2536      +20     
- Partials       17       18       +1
Flag Coverage Δ
unittests 23.02% <18.51%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
pyvisa_py/serial.py 5.71% <0.00%> (+0.02%) :arrow_up:
pyvisa_py/testsuite/test_serial.py 19.23% <19.23%> (ø)

... and 1 file with indirect coverage changes

LongnoseRob commented 1 year ago

Thanks for the review.