Beckhoff / ADS

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

ADS function Delay #21

Closed Satheesh-satz closed 8 years ago

Satheesh-satz commented 8 years ago

I am seeing delay of two minutes with either Deleting the Device Notification with the call to “AdsSyncDelDeviceNotificationReqEx” or with Closing the port with a call “AdsPortCloseEx”.

In both the cases I am seeing it tries to erase the notification in a loop and it takes approximately 2 minutes every time. And this happen when the PLC connection is disconnected manually from the Ethernet port.

void AmsPort::Close() { std::lock_guardstd::mutex lock(mutex);

auto it = std::begin(notifications);
while (it != std::end(notifications)) {
    it->second->Erase(it->first, tmms);
    it = notifications.erase(it);
}
port = 0;

}

bool AmsPort::DelNotification(const AmsAddr& ams, uint32_t hNotify) { std::lock_guardstd::mutex lock(mutex); for (auto it = notifications.begin(); it != notifications.end(); ++it) { if (it->first == hNotify) { if (std::ref(it->second->conn.second) == ams) { it->second->Erase(hNotify, tmms); notifications.erase(it); return true; } } } return false; }

Could you please help me solve this delay of 2 minutes.

pbruenn commented 8 years ago

Sorry I cannot reproduce this issue. When I run the notificationExample() from example.cpp, pulling the network cable while receiving notifications and then hit ENTER to terminate the program. I get a 5 seconds delay (default port timeout) and a graceful shutdown. Can you describe your setup in more detail?

Satheesh-satz commented 8 years ago

Hi Patric,

And this happens only when i add some event notification and currently i am having 20 events registered with ADS.

pbruenn commented 8 years ago

Okay now I see the problem. When you try to delete many notifications the timeouts will add and you will end up with a 20 * 5 sec delay. I see two possibilities:

  1. If you already know the connection is lost, or you don't care about the DelNotification responses from your master, you could set the timeout to 0, just before you call AdsSyncDelDeviceNotificationReqEx() or AdsPortCloseEx().
  2. For AdsPortCloseEx() this might make sense in general, so you could patch AmsPort:Close() directly: ` void AmsPort::Close() {
std::lock_guard<std::mutex> lock(mutex);
auto it = std::begin(notifications);
while (it != std::end(notifications)) {
    it->second->Erase(it->first, 0 /* was 'tmms' */);
    it = notifications.erase(it);
}
port = 0;

} `

I am not sure 2. is a good idea for all use cases, so I don't want to do that in upstream.

Besides: While investigating your issue, I stumbled upon ADS errors were not correctly propagated via DelNotification(). Commit 319d79414c82898034405b741f6549be1d212f1e is addressing this. With this commit AdsSyncDelDeviceNotificationReqEx() should give a return value of 1861 (ADS timeout elapsed) for your case. You could than uses this information to conditionally SetTimeout(0) and close the port.