pyvisa / pyvisa-py

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

fix: allow custom hislip sub_address #359

Closed alxthm closed 1 year ago

alxthm commented 1 year ago
MatthieuDartiailh commented 1 year ago

Your fix looks fine to me but I cannot test locally beyond the default case. Did you test on an instrument making use of sub addresses ?

MatthieuDartiailh commented 1 year ago

You will need to fix the formatting issue before this can be merged.

codecov-commenter commented 1 year ago

Codecov Report

Merging #359 (6ddb546) into main (8a0753b) will decrease coverage by 0.01%. The diff coverage is 0.00%.

:exclamation: Current head 6ddb546 differs from pull request most recent head 12a39f6. Consider uploading reports for the commit 12a39f6 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
- Coverage   23.13%   23.12%   -0.01%     
==========================================
  Files          22       22              
  Lines        3277     3278       +1     
  Branches      450      450              
==========================================
  Hits          758      758              
- Misses       2501     2503       +2     
+ Partials       18       17       -1     
Flag Coverage Δ
unittests 23.12% <0.00%> (-0.01%) :arrow_down:

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

Impacted Files Coverage Δ
pyvisa_py/protocols/hislip.py 27.33% <0.00%> (ø)
pyvisa_py/tcpip.py 20.55% <0.00%> (-0.04%) :arrow_down:

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

alxthm commented 1 year ago

Thanks for the feedback!

Yes, this has been tested on a different sub address, with and without specifying the port in the resource name:

import pyvisa
rm = pyvisa.ResourceManager('@py')
dev = rm.open_resource("TCPIP0::pxi-chassis-01::hislip_PXI0_CHASSIS1_SLOT3_INDEX0::INSTR")
# OR
dev = rm.open_resource("TCPIP0::pxi-chassis-01::hislip_PXI0_CHASSIS1_SLOT3_INDEX0,4880::INSTR")
dev.query('*IDN?')

Regarding the formatting issue, it seems to me flake8 is failing on something unrelated to this change. The same issue already appears in the last release commit for instance. If you wish, I can add a commit that simply ignores the warning (# noqa: C901, which seems fair to me in this case), but maybe this would be better as a separate fix?

MatthieuDartiailh commented 1 year ago

Sorry for the long delay. I properly fixed the flake8 issue in #362 and fixed a couple of other issues. Could you rebase your work and quickly retest ? Once it is done I will merge quickly.

alxthm commented 1 year ago

Done! I can still open resources like before. Thanks for fixing these other issues :)

MatthieuDartiailh commented 1 year ago

Could you add an entry to the changelog ?

MatthieuDartiailh commented 1 year ago

Thanks