Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use std::function for callback #203

Open
ccvca opened this issue Jun 2, 2023 · 1 comment · May be fixed by #204
Open

Use std::function for callback #203

ccvca opened this issue Jun 2, 2023 · 1 comment · May be fixed by #204

Comments

@ccvca
Copy link
Contributor

ccvca commented Jun 2, 2023

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:

struct AdsNotification {

ADS/AdsLib/AdsDevice.cpp

Lines 83 to 87 in 3824918

AdsHandle AdsDevice::GetHandle(const uint32_t indexGroup,
const uint32_t indexOffset,
const AdsNotificationAttrib& notificationAttributes,
PAdsNotificationFuncEx callback,
const uint32_t hUser) const

long AdsSyncAddDeviceNotificationReqEx(long port,
const AmsAddr* pAddr,
uint32_t indexGroup,
uint32_t indexOffset,
const AdsNotificationAttrib* pAttrib,
PAdsNotificationFuncEx pFunc,
uint32_t hUser,
uint32_t* pNotification)

struct Notification {

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.

@pbruenn
Copy link
Member

pbruenn commented Jun 2, 2023

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

@ccvca ccvca linked a pull request Jun 13, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants