Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
502 stars 194 forks source link

handling multiple nics #47

Closed dom-white closed 7 years ago

dom-white commented 7 years ago

Hi, I noticed that there is no support for the computer running your library to have multiple network interfaces, each with ads machines on.

For example two networks cards, each with an ADS machine on.

In the router there is the concept of 'local address' and this is populated with the ip address used by the first connection's 'own ip'. The write function then uses this as the 'src address' when sending all further commands,

If a new connection is made it is assumed that the src ip address is the same, however if this is done through a different network interface then the write command sets the 'src address' incorrectly and the transaction fails.

I have implemented the following patch as a workaround

diff --git a/AdsLib/AmsConnection.h b/AdsLib/AmsConnection.h
index 6efc72c..540769b 100644
--- a/AdsLib/AmsConnection.h
+++ b/AdsLib/AmsConnection.h
@@ -80,10 +80,10 @@ struct AmsConnection : AmsProxy {
     template<class T> long AdsRequest(AmsRequest& request, uint32_t tmms)
     {
         AmsAddr srcAddr;
-        const auto status = router.GetLocalAddress(request.port, &srcAddr);
-        if (status) {
-            return status;
-        }
+        AmsNetId netId = AmsNetId {ownIp};
+        memcpy(&srcAddr.netId, &netId, sizeof(netId));
+        srcAddr.port = request.port;
+
         AmsResponse* response = Write(request.frame, request.destAddr, srcAddr, request.cmdId);
         if (response) {
             if (response->Wait(tmms)) {

This utilises the fact that each connection object knows it own src ip, and so it uses that rather than calling the router's GetLocalAddress method. What this does not take into account is there are external API's for getting and setting the local address, and these would still just affect the variable localAddress stored in the router object (which this workaround bypasses). I am not sure why the external API's to get and set the local address are required, but I would like to get a fix in that does not alter any of these external API's.

Another way to solve this (and keep the get and set method's valid), is to keep a 'map' of src addresses and use the portnumber as a key. This would require some API changes though, e.g to store each src address in the Router's AddRoute method, the AddRoute method would need the portnumber passed in, so the API for AdsAddRoute would need this extra portnumber parameter. Also the get and set local address methods, would also need to pass in a portnumber too.

Please can you let me know what you think the best solution would be, as I am keen to get this support into your great library, rather than hacking a patch into my version locally

Thanks, Dominic

pbruenn commented 7 years ago

Hi Dominic,

I have two uses cases in mind and I'm not sure, which you mean:

  1. You run one AdsLib application on a machine with two NICs which are both connected to different TwinCAT machines/networks.
  2. You run two (or more) AdsLib applications on your machine and want to address these applications with different AdsNetIds

Which one describes your setup?

For the first case, you should be fine by choosing just one static AmsNetId for your AdsLib implementation, apply it with AdsSetLocalAddress() and add routes for that NetId on your TwinCAT machines. Commit c1bfedd25988468ac0f46e9cf0fff4d4be3ffa52 introduced AdsSetLocalAddress() and it was directly intended to support this use case. With more than one IP you can't be sure which will be taken to derive the NetId and you end up in a scenario like you described.

The second use case is not supported by current AdsLib. It would require to implement a central instance on your host machine, which accepts connections from local applications and multiplex their ADS communication to other TwinCAT hosts. That central instance would become a full message router [1], which requires to implement a platform independent system service/daemon, ADS routing tables, NetId assignment and TCP connection sharing.

Regards, Patrick [1] https://infosys.beckhoff.de/content/1033/tcadscommon/html/tcadscommon_intro.htm

dom-white commented 7 years ago

Hi Patrick, Thank you for your feedback, sorry if I was not clear. It is use case number 1 that I am interested in.

I am not sure how the AdsSetLocalAddress() can help, as I have two local addresses, and this API sets a single instance of a local address in what is effectively a singleton Router object.

Do you mean to say call the AdsSetLocalAddress() prior to attempting a read on a TwinCat device on a different network, so e.g. an example based on your example

    static const AmsNetId remoteNetIdNic1 { 192, 168, 0, 231, 1, 1 };
    static const AmsNetId remoteNetIdNic2 { 172, 16, 0, 231, 1, 1 };

    static const AmsNetId myNetIdNic1 { 192, 168, 0, 241, 1, 1 };
    static const AmsNetId myNetIdNic2 { 172, 16, 0, 241, 1, 1 };

    static const char remoteIpV4Nic1[] = "ads-server-viaNic1";
    static const char remoteIpV4Nic2[] = "ads-server-viaNic2";

    // add 1st route
    if (AdsAddRoute(remoteNetIdNic1, remoteIpV4Nic1)) {
        out << "Adding ADS route failed, did you specified valid addresses?\n";
        return;
    }
    // and create a port for it
    const long portNic1 = AdsPortOpenEx();
    if (!port) {
        out << "Open ADS port failed\n";
        return;
    }
    // add 2nd route on 2nd nic
    if (AdsAddRoute(remoteNetIdNic2, remoteIpV4Nic2)) {
        out << "Adding ADS route failed, did you specified valid addresses?\n";
        return;
    }
    // and add port for it
    const long portNic2 = AdsPortOpenEx();
    if (!port) {
        out << "Open ADS port failed\n";
        return;
    }

    const AmsAddr remoteNic1 { remoteNetIdNic1, AMSPORT_R0_PLC_TC3 };
    const AmsAddr remoteNic2 { remoteNetIdNic2, AMSPORT_R0_PLC_TC3 };

    uint32_t bytesRead;
    uint32_t buffer;

    // set local address to that of my nic 1 card before reading via from nic 1
    AdsSetLocalAddress(myNetIdNic1);

    // read 4 bytes from address 0x4020 from TwinCat device on Nic 1
    const long status = AdsSyncReadReqEx2(portNic1, &remoteNic1, 0x4020, 0, sizeof(buffer), &buffer, &bytesRead);
    if (status) {
        out << "ADS read failed with: " << std::dec << status << '\n';
        return;
    }
    out << "ADS read " << std::dec << bytesRead << " bytes, value: 0x" << std::hex << buffer << '\n';

    // set local address to that of my nic 2 card before reading data via nic 2
    AdsSetLocalAddress(myNetIdNic2);

    // read 4 bytes from address 0x4020 from TwinCat device on Nic 2
    const long status = AdsSyncReadReqEx2(portNic2, &remoteNic2, 0x4020, 0, sizeof(buffer), &buffer, &bytesRead);
    if (status) {
        out << "ADS read failed with: " << std::dec << status << '\n';
        return;
    }
    out << "ADS read " << std::dec << bytesRead << " bytes, value: 0x" << std::hex << buffer << '\n';

The comment in the c1bfedd commit would lead me to think this was wrong though SetLocalAddress() should not be called when an AmsPort is already open.

However if the localAddress is not altered, then the 2nd read will fail as the request packet sent will have header information with the wrong local ip address in it. In this case the AdsRequest returns ADSERR_CLIENT_SYNCTIMEOUT

The change that I suggested in my first post means each request is sent out with the correct local address set and therefore all requests work whatever network that are routing through

pbruenn commented 7 years ago

Why do you need two different local addresses? SetLocalAddress() in AdsLib is intended to be the equivalent to the AmsNetId of the TwinCAT router on your TwinCAT system: ams_router

The relation: NetId == IP + ".1.1" is not a requirement by ADS.

Just use the same NetId on both TwinCAT remotes, when adding the routes for 192.168.0.241 and 172.16.0.241.

Of course, if the router on the devices your application wants to connect to, is not under your control, we are in trouble. I hope that's not your intend to use two different local addresses.

dom-white commented 7 years ago

Hi Patrick, Ok, I see what you are saying. It is because the TwinCAT devices have their route back to me set as an ams address based on the ip address of the nic of my computer that they are coming in over. But you are saying in effect both of the TwinCAT devices could have an identical ams address for me.

Ok, so this has highlighted not an issue in your code but an issue in my understanding :-)

Thank you very much for the explanation, I will try and get the TwinCat devices set up correctly, and actively set my computer's ams address (via AdsSetLocalAddress) to an ams address I have chosen for my computer

pbruenn commented 7 years ago

I'm glad I could help. If you run in any other limitation or bug, don't hesitate to write another issue.