pyvisa / pyvisa-py

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

TCPIP timeout handling improvement #113

Closed skrchnavy closed 6 years ago

skrchnavy commented 6 years ago

Improvement of #107

Code is simplified in calculation when timeout elapsed. (removed variable now)

poll interval calculation is improved.

Note: Poll interval is necessary on windows, because select is not 'broken' by signal. so waiting in select for example 10 seconds would block program for 10 seconds until signal (CTRL+C) processed. \ On linux there is no such issue with .

poll_ interval is calculated as 50% of previous poll interval (min is 50ms). \ Previous constant 10ms poll interval could increase CPU utilization.

When merged, #61 can be closed

MatthieuDartiailh commented 6 years ago

LGTM. Can I assume you checked this works as expected ?

skrchnavy commented 6 years ago

yes, i tested it on windows 7 and works as described.

With 10s timeout, it waited 2secs then ~1 sec, then ~0.5 sec, etc. and finished after 123 iterations, table shows time missing to timeout (measured after select unblocked)

8.0
6.99799990654
6.4960000515
6.24300003052
6.11599993706
6.05099987984
5.99900007248
5.94799995422
5.89700007439
==== CUT ====
0.148000001907
0.095999956131
0.0439999103546
-0.00799989700317

Last value shows that function finshed 8ms later then planed.

With 3s timeout, measurement is:

1.5
0.748000144958
0.371000051498
0.180999994278
0.085000038147
0.0329999923706
-0.0190000534058

finished 19ms after planed timeout.

MatthieuDartiailh commented 6 years ago

Thinking a bit more about this, I would like to see a small modification to provide a better support of short timeout (sometimes useful in debugging). Could you add a min_select_timeout = max(min(select_timeout/10, 50), 1) , and use it instead of 50, such that we get something closer to the expected timeout for timeout in the range 1 to 50 ms.

To fix the conflicts please rebase on master rather than merging it makes for a cleaner history.

MatthieuDartiailh commented 6 years ago

There are still some conflicts that needs to be solved, once it is done I will merge.

skrchnavy commented 6 years ago

I was not able locally rebase changes so I did the merge. I will do final change in calculation of select_timeout and min_select_timeout and then you can review / merge

skrchnavy commented 6 years ago

ready for final review. measurments (diff is difference between real timeout and expected timeout, it shall be <1% from expeted timeout):

MatthieuDartiailh commented 6 years ago

I just added a tiny bit more docs (in case somebody in the future think about reverting to only use select timeout). If you are happy with the change, you can trigger bors

bors delegate+

bors[bot] commented 6 years ago

:v: skrchnavy can now approve this pull request

skrchnavy commented 6 years ago

Agreed and approved

MatthieuDartiailh commented 6 years ago

bors r+

bors[bot] commented 6 years ago

Build succeeded