Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
521 stars 197 forks source link

netbios,exception,and multithread #240

Open Wudel opened 6 days ago

Wudel commented 6 days ago

Dear ADS Library Team,

I hope this message finds you well. I have been experimenting with the ADS library and have encountered a few issues that I would greatly appreciate your guidance on.

  1. NetBIOS Protocol Requirement: It seems that the platform using this library requires the NetBIOS protocol to establish a connection; otherwise, the connection might be terminated by the remote PLC (such as TwinCAT). In a Linux environment, this implies the need for Samba. Could you confirm if this is indeed necessary?
  2. Handling SIGPIPE Signals: During testing, I noticed that when the socket is broken, a SIGPIPE signal may be thrown, causing the program to crash. Would it be advisable to use signal(SIGPIPE, SIG_IGN) to ignore this signal? Or are there other recommended approaches to handle this situation?
  3. https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsRouter.cpp#L82 Exception Handling in AmsRouter.cpp: While reviewing the code, I observed that the exception is thrown using throw e instead of simply throw. It seems that using throw would be preferable as it preserves the original exception type, allowing it to be caught as an adsException or runtime_error. Could you please give some advice on this?
  4. multiThread: It appears that when multiple adsDevice instances are created using same remote ip and amsid, they share a single AmsConnection, which uses one TCP socket. The differentiation between these devices is based on their local ports, although these are not like traditional TCP ports. When I arrange one adsDevice per thread in multithreads, simultaneous requests from different devices seem to interfere with each other, possibly due to the shared sendto function. Can you confirm whether they will indeed affect each other? https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/AdsDevice.h#L85 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/AdsDevice.cpp#L201 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AdsLib.cpp#L106 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AdsLib.cpp#L255 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsRouter.cpp#L204 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsConnection.cpp#L141 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsConnection.cpp#L126 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/Sockets.cpp#L224
pbruenn commented 6 days ago

Dear ADS Library Team,

I hope this message finds you well. I have been experimenting with the ADS library and have encountered a few issues that I would greatly appreciate your guidance on.

1. NetBIOS Protocol Requirement: It seems that the platform using this library requires the NetBIOS protocol to establish a connection; otherwise, the connection might be terminated by the remote PLC (such as TwinCAT). In a Linux environment, this implies the need for Samba. Could you confirm if this is indeed necessary?

I don't understand why NetBIOS should be required? You either have to use IP addresses or any kind of name resolution, but DNS should be just fine.

2. Handling SIGPIPE Signals: During testing, I noticed that when the socket is broken, a SIGPIPE signal may be thrown, causing the program to crash. Would it be advisable to use signal(SIGPIPE, SIG_IGN) to ignore this signal? Or are there other recommended approaches to handle this situation?

I have no better approach for this.

3. https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsRouter.cpp#L82

   Exception Handling in AmsRouter.cpp: While reviewing the code, I observed that the exception is thrown using throw e instead of simply throw. It seems that using throw would be preferable as it preserves the original exception type, allowing it to be caught as an adsException or runtime_error. Could you please give some advice on this?

you are probably right. This should be a rethrow instead of copy and throw.

4. multiThread: It appears that when multiple adsDevice instances are created using same remote ip and amsid, they share a single AmsConnection, which uses one TCP socket. The differentiation between these devices is based on their local ports, although these are not like traditional TCP ports. When I arrange one adsDevice per thread in multithreads, simultaneous requests from different devices seem to interfere with each other, possibly due to the shared sendto function. Can you confirm whether they will indeed affect each other?

Yes, this is fundamental ADS design. TwinCAT will only accept a single TCP connection from the same IP. https://github.com/Beckhoff/ADS/issues/201#issuecomment-1544167471 https://github.com/Beckhoff/ADS/issues/49#issuecomment-330198577 https://github.com/Beckhoff/ADS/issues/72#issuecomment-406172875 https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/AdsDevice.h#L85

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/AdsDevice.cpp#L201

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AdsLib.cpp#L106

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AdsLib.cpp#L255

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsRouter.cpp#L204

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsConnection.cpp#L141

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/standalone/AmsConnection.cpp#L126

   https://github.com/Beckhoff/ADS/blob/20d4a67970a6b954b32f6e51caf622da26adc13c/AdsLib/Sockets.cpp#L224
Wudel commented 6 days ago

https://github.com/Beckhoff/ADS/issues/86#issuecomment-486708069 Thank you very much. As mentioned in the link, regarding the fourth question: if we use different AdsDevice instances (each with a different port) in different threads, each AdsDevice will depend on the same socket connection. Should we use a mutex or another synchronization method to prevent these AdsDevice instances from sending adsRequest simultaneously?

Wudel commented 5 days ago

https://github.com/user-attachments/assets/dd49555f-f6f6-4d14-9b0d-7b42251cf704

For the first question, I am confused about how NetBIOS influences the program. In fact, there are no references to NetBIOS in the source code of the "main" program. NetBIOS runs as a service system and has no direct relationship with the "main" program. This makes the situation quite puzzling.

企业微信截图_17328495449045

I tried to capture the network traffic using Wireshark and found that the PLC conducts a name query via NBNS once when we attempt to connect to the PLC. This may be a design feature of the PLC and seems to have no relationship with the ADS library. The remote PLC is running TwinCAT 2, with the CPU being CX1100-0002 and the Ethernet module being CX1020-0011. The errorcode is 1861. I hope this information will be helpful to others.

IMG_3948

pbruenn commented 5 days ago

#86 (comment) Thank you very much. As mentioned in the link, regarding the fourth question: if we use different AdsDevice instances (each with a different port) in different threads, each AdsDevice will depend on the same socket connection. Should we use a mutex or another synchronization method to prevent these AdsDevice instances from sending adsRequest simultaneously?

No, it should be safe to use AdsDevices in parallel without external synchronization. This is our testcase for this: https://github.com/Beckhoff/ADS/blob/master/AdsLibOOITest/main.cpp#L497-L510