Closed GullinBustin closed 4 months ago
@bnjmnp I added comments in the main changes of this pull request. A big percentage of the changes are only a copy of SOEM files. The real changes are to add Py_BEGIN_ALLOW_THREADS
and Py_END_ALLOW_THREADS
to the blocking functions and replace the files in setup.py.
@GullinBustin: Thank you for sharing your ideas.
What is the issue with SDO communication being slow? Cyclic PDO communication is actually the more time critical part of EtherCAT.
Did you check if your changes might have a negative effect on PDO cycles running in parallel?
Also, did you consider patching my fork of the SOEM repo (submodule) in the same way I already patched it? This would be a more clean way of introducing changes to the C code?
Consider the case when pople introduced fixes or improvements to the main SOEM repo. It is much nicer to apply the patches with the forked SOEM submodule method, than manually updating the local copies of osal.c
and nicdrv.c
.
@bnjmnp Thanks for your reply.
I didn't notice that the submodule points to a fork of your own instead of the original SOEM repo. Definitely, that changes must be done in the SOEM repository.
About effects on the PDO communications, it should not affect, but I will do better testing.
What is the issue with SDO communication being slow? Cyclic PDO communication is actually the more time critical part of EtherCAT.
About this question, the problem is not SDO communications are slow, the problem is that all the communications by EtherCAT (PDO too) block Python threads. In my case, it is a problem because I have a program with a GUI and the SDOs block the GUI thread. I want to do something similar to what the Python socket module does, where threads are unblocked when it waits for socket response: https://github.com/python/cpython/blob/main/Modules/socketmodule.c#L967. But in this case, doing it is not trivial.
In conclusion, I will close this PR because these changes should be done in SOEM repository, but I will study it more before creating the new PR because I'm not completely happy with the current solution. Add sleep works for me, but it could be problematic depending on the devices.
In Python, when 2 or more threads are created they can't work in parallel because of GIL. Some functions in Python (such as sleep) allow running another thread during its execution. In PySOEM, sdo_read and sdo_write (for example) await the slave response, but this wait does not unblock Python threads.
This pull request solves this problem for windows by modifying the osal.c and nicdrv.c files and unblock Python threads in the blocking functions, as
osal_usleep
. Fix the problem onosal_usleep
is easy, but forecx_waitinframe_red
the only way is to add anosal_usleep
call to reduce the number of iterations and allow running other threads meanwhile.Tests
I made a manual test to verify SDO reads do not block the threads. The test has a main thread and a secondary thread:
The result of the new implementation:
The secondary thread is hardly affected by the SDO reads.
The result of the old implementation:
The secondary thread is very affected by the SDO reads. When the SDO reads loop ends, the secondary thread works like in the new implementation case.
Test code: