pbruenn
commented
1 year ago Yes, you are not the first (#142) and I agree a std::function<> interface would be much nicer. Actually this is on my todo list for quite some time, but I had no time to look into this. Important is, that we don't break existing users. So it might be easier to add a new wrapper for this. If you have a good idea, go for it. But be warned I will only merge a clean solution.
EDIT: Beware you can not change AdsSyncAddDeviceNotificationReqEx as that is part of the legacy "TcAdsLib" interface [1]. We have to keep it in sync to keep the possibility to link against upstream TwinCAT TcAdsDll.dll.
[1] https://infosys.beckhoff.com/content/1033/tcadsdll2/12444732299.html
As this library already requires C++ 14 , using std::function (https://en.cppreference.com/w/cpp/utility/functional/function) for the notifications should improve the usability a lot (e.g. bind to class members, use custom handler types, ...). As a function pointer can be bound to an std::function, too, this would not break existing code.
This would require changes in these places: https://github.com/Beckhoff/ADS/blob/3824918ebbcaefa4cfa398da149822f37e9d7f6f/AdsLib/AdsNotificationOOI.h#L14 https://github.com/Beckhoff/ADS/blob/3824918ebbcaefa4cfa398da149822f37e9d7f6f/AdsLib/AdsDevice.cpp#L83-L87 https://github.com/Beckhoff/ADS/blob/3824918ebbcaefa4cfa398da149822f37e9d7f6f/AdsLib/standalone/AdsLib.cpp#L261-L268 https://github.com/Beckhoff/ADS/blob/3824918ebbcaefa4cfa398da149822f37e9d7f6f/AdsLib/AdsNotification.h#L15
PS: I'm aware that this is a feature request, but I just want to get a general opinion about this, before I potentially work on this.