Handfeger commented 3 years ago

This fixes #285

I ran the tests and believe that other installations should not be affected (but not sure about windows). I am not sure if it is possible to write a test for this as you need to wait 5 minutes inside a docker container to experience this issue. I am happy to provide a test if anybody has an idea how it could be tested.

Another option to make this a option that can be accessed via highlevel function. I have found no way to access the sockets from outside. Maybe another option would be to expose the socket somehow to highlevel access points.

isort complaines about 2 files I did not touch: /docs/source/conf.py and /pyvisa_py/testsuite/keysight_assisted_tests/test_tcpip_resources.py. I did not fix this to not pollute this pull request.

[x] Closes #285
[x] Executed black . && isort -c . && flake8 with no errors
[x] The change is fully covered by automated unit tests
[x] Documented in docs/ as appropriate
[x] Added an entry to the CHANGES file

MatthieuDartiailh commented 3 years ago

The VISA specification mentions that VI_ATTR_TCPIP_KEEPALIVE should be supported by TCPIP::SOCKET and HiSLIP resources (a special case of INSTR), when enabled keep alive packets are used. However nothing is said for VXI-11 (the conventional TCPIP::INSTR).

Currently PyVISA-py has no support for HiSLIP so let's not worry about that. For SOCKET it would be straightforward to port your solution to using the proper attribute. For INSTR, it may be worth providing a global flag in pyvisa-py to allow to opt-in that feature. How does that sound ?

Let me know if you need any guidance.

MatthieuDartiailh commented 3 years ago

ping @Handfeger

Handfeger commented 3 years ago

Sorry I'm currently learning for exams, so do not expect an answer before the middle of the month.

MatthieuDartiailh commented 3 years ago

No problem.

Handfeger commented 3 years ago

Currently PyVISA-py has no support for HiSLIP so let's not worry about that. For SOCKET it would be straightforward to port your solution to using the proper attribute. For INSTR, it may be worth providing a global flag in pyvisa-py to allow to opt-in that feature. How does that sound ?

Let me know if you need any guidance.

As our instruments do not open a port for socket connection, I fear that the opt-in via global flag is our only option. Where would you define this global option? You mean setting a variable in the global scope and accessing that with pyvisa-py? That seems a bit hacky to me. Or would you set a global option in the session?

MatthieuDartiailh commented 3 years ago

It would not be perfect but, if we want to avoid global variable at the module level (that could live under tcpip.py should we go that route nonetheless in the end), we could have that parameter lives on the Library object (defined in highlevel.py). However the library is never passed to the session so we need to either change that (in Session.init), or do the work in the library object which feels a bit weird since it is very specific to a session.

So I suggest:

having a new attribute on the VisaLibrary object vxi11_use_keepalive
passing the library to session in their __init__ (be careful to alter all of them)
apply your fix depending on the value of vxi11_use_keepalive, and store the attribute value on the session in case people need to inspect it later on
implement the proper solution for SOCKET (ie your fix but based on VI_ATTR_TCPIP_KEEPALIVE rather than vxi11_use_keepalive)

How does that sound ?

Handfeger commented 3 years ago

Looking into the handling of attributes on the highlevel API I'm wondering if the introduction of a custom attribute would also work. This would mean to add checking for the custom attributes in set_attribute and get_attribute. But this way we could access the session and with that the socket of the instrument we want to activate keepalive packets on.

MatthieuDartiailh commented 3 years ago

This is also an option but will require to define a new constant, since it does not exist at the VISA level. If you prefer that approach it is fine by me.

Handfeger commented 3 years ago

The approach I took was to overwrite the VI_ATTR_TCPIP_KEEPALIVE attribute to also be available for tcpip sessions of the INSTR type. This is done in the new file attributes.py which is imported in sessions.py. Because of the architecture of PyVisa attributes, importing is sufficient for overwriting it. After that all I did was to implement the attribute on session level in tcpip.py.

Are you okay with these decisions?

Below you see the test from inside the docker container. The line rs = pyvisa.ResourceManager('@py') is missing. Code Test

MatthieuDartiailh commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build failed:

Check code formatting

codecov-commenter commented 3 years ago

Codecov Report

Merging #292 (916f6da) into main (40d4396) will decrease coverage by 16.55%. The diff coverage is 41.66%.

:exclamation: Current head 916f6da differs from pull request most recent head fcfb458. Consider uploading reports for the commit fcfb458 to get more accurate results

@@             Coverage Diff             @@
##             main     #292       +/-   ##
===========================================
- Coverage   38.77%   22.21%   -16.56%     
===========================================
  Files          19       20        +1     
  Lines        2561     2597       +36     
  Branches      343      347        +4     
===========================================
- Hits          993      577      -416     
- Misses       1511     2013      +502     
+ Partials       57        7       -50

Flag	Coverage Δ
unittest	`?`
unittests	`22.17% <41.66%> (+0.27%)`	:arrow_up:

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

Impacted Files	Coverage Δ
pyvisa_py/sessions.py	`40.50% <ø> (-26.50%)`	:arrow_down:
pyvisa_py/testsuite/__init__.py	`46.15% <ø> (ø)`
...e/keysight_assisted_tests/test_resource_manager.py	`100.00% <ø> (ø)`
pyvisa_py/tcpip.py	`18.24% <6.66%> (-49.91%)`	:arrow_down:
...te/keysight_assisted_tests/test_tcpip_resources.py	`82.92% <30.00%> (-17.08%)`	:arrow_down:
pyvisa_py/__init__.py	`91.66% <100.00%> (+0.75%)`	:arrow_up:
pyvisa_py/attributes.py	`100.00% <100.00%> (ø)`
pyvisa_py/protocols/vxi11.py	`43.75% <0.00%> (-33.34%)`	:arrow_down:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 40d4396...fcfb458. Read the comment docs.

Handfeger commented 3 years ago

I'm happy to add documentation but wanted to check back if the code is fine first.

I also have a few further questions to get this pull request ready:

Should I add an FAQ about broken pipes or is there some troubleshooting guide I missed?
Where do you believe more documentation is needed?
~~Do you think I should write a test for this feature? How would I go about that?~~
flake8 is complaining about importing attributes without using it, but the import is needed so __init_subclass__() is executed once on this attribute to overwrite it. How do I handle this?

MatthieuDartiailh commented 3 years ago

Regarding the documentation, you could add en entry under the FAQ of pyvisa-py. It is not ideal but since there is already en entry discussing use inside a VM it would work. We could also have an entry under Pyvisa documentation as another PR but mentionning the fact that the VXI11 part is pyvisa-py specific.

Regarding flake8 you can add # noqa: followed by the error code to ignore.

Once this is done I will give bors another try to exercise your test.

Handfeger commented 3 years ago

Out of curiosity would you have the bandwidth to test using Keysight IO Library suite (which is available for linux) in your docker image to see if the connection drops ? I am curious to know if they enabled the keepalive by default since it is not discussed as part of the vxi11 standard. If they do we should do the same.

I am pretty sure this is not the standard option as we tried many different Visa systems and always ran into this particular problem. But to be honest I don't believe we tried the Keysight lib. So I might do that on the weekend.

I finished the documentation now so feel free to test run it all.

MatthieuDartiailh commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build failed:

pyvisa.pyvisa-py.keysight-assisted

MatthieuDartiailh commented 3 years ago

Looks like the bot is down... I will look at it tomorrow.

Handfeger commented 3 years ago

Inheriting is in and everything still works.

MatthieuDartiailh commented 3 years ago

Not sure you can see it but here is the log of the keysight bot https://dev.azure.com/pyvisa/pyvisa-py/_build/results?buildId=192&view=logs&j=6b902995-b73d-5f5c-66fd-a7f66c857d2c&t=a1c457be-0fbe-5525-b6b0-ac3cf58f8d6c&l=135.

Basically your test do not pass, because self.instr is not the pyvisa-py internal session object but the normal pyvisa resource object. You will need something like self.instr.visalib.sessions[self.instr.session] to obtain the object on which keepalive exists.

Handfeger commented 3 years ago

Is there any way to test my function locally? pytest doesn't seem to test this particular function for me and I don't want to waste your time with failing code.

MatthieuDartiailh commented 3 years ago

In order for those test to run you need to set an env var (see the __init__.py of the keysight assisted folder of pyvisa). If I spell it from memory I will get it wrong. You will also see the test IP.

MatthieuDartiailh commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build failed:

pyvisa.pyvisa-py.keysight-assisted

MatthieuDartiailh commented 3 years ago

I happen to have some serious issue on the bot at the moment and may need to swap out a computer so I do not know I will be able to have it back online. If you can get the test to pass locally, great, otherwise we can merge as is and open a new issue about the test and I will look into it when the bot is back online.

Handfeger commented 3 years ago

We are currently running an experiment on the hardware so the earliest point would be end of the week, so maybe we open a new issue for the test.

MatthieuDartiailh commented 3 years ago

bors try

MatthieuDartiailh commented 3 years ago

Apparently windows believes it managed to fix a disk issue on the bot, so I will try to run the bot one last time and if it fails for something unrelated to this I will merge and open an issue.

bors[bot] commented 3 years ago

try

Build failed:

pyvisa.pyvisa-py.keysight-assisted

MatthieuDartiailh commented 3 years ago

The build failed but that does not look related to your work. I will try to have a look nonetheless but I will do my best to merge shortly.

Handfeger commented 3 years ago

We currently have a workaround so there is not that much time pressure on this issue.

MatthieuDartiailh commented 2 years ago

Since I have had no time to look into the bot issue I will merge as is. Thanks for your work @Handfeger an sorry for the delay.

pyvisa / pyvisa-py

Fixing TCP/IP droping in from Dockercontainers #292

try

Codecov Report

try

try

try