search

Beckhoff / ADS

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

Standalone DLL with C interface #156

Closed cowo78 closed 2 years ago

cowo78 commented 2 years ago

Plain C functions in the library should be accessible using C interface (no name mangling). Also some files were missing in CMakeLists.txt. Tested on win64/gcc11/cmake

pbruenn commented 2 years ago

Hi @cowo78, thanks for the PR. I have a few questions, what is your use case for C ABI/API? Do you need it for other language bindings like pyads?

As mentioned in https://github.com/Beckhoff/ADS/issues/127, pure C is out of scope for this project. That's why we dropped the ifdefs you want to add back.

cowo78 commented 2 years ago

Yes, I currently use it with pyads. Broadly speaking I don't see the point in having a C-style interface (based on pure functions) being mangled by the C++ compiler. The TcAdsDll is compiled without name mangling so why having the same features with different symbols?

pbruenn commented 2 years ago

The history is: TcAdsDll was first so we began to implement AdsLib with the same interface as TcAdsDll. But soon that C API proofed to be very inefficient to use (code redundancy). So we advanced the AdsLib(AdsLibOOI) to be easier usable from C++. At that point the similarity to TcAdsDll allowed us to use that modern interface (AdsLibOOI) on top of TcAdsDll, too. However, having this pure C API on bottom of AdsLib is very limiting. Just look how we convert error codes to exceptions and back to error codes. The future plan is to drop the ancient interface. This would simplify AdsLib and might improve the performance, too. However, at that point we will lose the ability to use our more modern interface with TcAdsDll. In the end that's the conflict, which stopped the progress to get rid of the C API earlier. Well, I have to admit there wasn't much innovation in the past, but bringing back C compatibility will not help in that regard.

cowo78 commented 2 years ago

IMHO as long as there is a plain C interface it should be usable. I agree it's mainly a legacy burden so by all means go ahead and remove it but since it is still there now my point is to make it usable. Again that's only my opinion so if it does not fit into project scope just say so and close the PR.

pbruenn commented 2 years ago

I have now slept on it and I think we just don't have a C API anymore. If only the extern "C" declarations were needed I would say let's add them without #ifdef __cplusplus. But it is not that simple. Who checks if the C API works at all? How do we prevent regressions there? I can't and don't want to support that. Apart from that two things are mixed up in the PR. The changes to CMakeLists.txt could be interesting independently from the C API. So if cmake users want to merge that, I am fine to merge those changes independently.

cowo78 commented 2 years ago

All right. But this, to me, sounds like that the functions based API should just be dropped. extern C or not the feature is the same and if it is not tested/required anymore why don't you drop it?

I will submit a different PR with the cmake-related changes only.

Feel free to close this PR.

pbruenn commented 2 years ago

If you mean this defines https://github.com/Beckhoff/ADS/blob/master/AdsLib/AdsLib.h#L203-L205 you might be right we should just drop them. However the simple functions couldn't just be dropped as we still use them in the modern interface. We just don't test them from pure C and outside the context of this library.