Beckhoff / ADS

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

Inegration of GetDeviceAdsVariables and add IAdsVariable type #170

Closed Capacites closed 1 year ago

Capacites commented 1 year ago

GetDeviceAdsVariables : this function list all disponible variable (Name and type of variable) of the automate

IAdsVariable : Interface wich can regroupe all AdsVariable in vector or map and use them without typing

pbruenn commented 1 year ago

First of all thanks for the PR, I like the feature. However, this PR needs some changes to get merged. The parsing function doesn't validate the input. bytesRead is never used to check if our calculated pointers are out of bounds. I think this is related to the old school macros PADSSYMBOLNAME, PADSSYMBOLTYPE. I know they are in the examples on Beckhoff Infosys. However, this is not a copy of Beckhoff Infosys. We want to do better. And for me better means correct parsing. E.g. Check if pAdsSymbolEntry + nameLength is out of bounds. And drop these old school defines (and typedefs).

But this is only a detail. What I am missing is the bigger picture. What is the usecase for this new function? Please, describe that in the commit messages. Adding a short demonstration to our existing example would be a great bonus.

I have an idea, what you are trying to do. But I struggle with the precise usecase, which is important to find an elegant solution for to parsing problem.

Capacites commented 1 year ago

Thank you for reply! We will create a method to replace the macro.

We use this lib in our Ros node project here. In your node config file we declare the GVL variable we wish to publish. The ros node find the GVL and build with factory the adsvariable with the good type and store it in the global list.

pbruenn commented 1 year ago

So at the moment you use it for double only? Or do you use double in ROS only and convert it to int for example? What I want to find out is how we could make these AdsVariables typesafe but generic.

Capacites commented 1 year ago

We use primary type with automate to libads (bool, uint, int ,double,float..) and we cast in double in ROS. We are aware that IADSVariable works only in primary type ,not in generic type.

If you use a AdsVariable and you add in your map, you must cast it in AdsVariable before you "operator =" get or "operator T()" method.

We don't have a simple solution for a generic IADSVariable.

Capacites commented 1 year ago

The fix is under the test.

Capacites commented 1 year ago

We have finish our test. you can review the code and merge it.

Capacites commented 1 year ago

@pbruenn : do you have made some test to valid our request?

pbruenn commented 1 year ago

Well, my remarks from my first comment still stand. I still don't liked the parsing code and I am not convinced the approach to put AdsVariables into a std::map or vector is a good idea. You send additional patches, but they are hard to review, if they are not rebased and logically squashed together. I had not time to write a CONTRIBUTING file, but will do this in the near future.

A few weeks ago, I started my own approach on plc symbol parsing and today decided to push it public: https://github.com/Beckhoff/ADS/tree/patrickbr/ads-symbol-upload-info Feel free to send feedback, but be aware that this is only a side project and I still have to try some things to find a nice way of handling different types in a single map/vector. My idea is basically to read all the plc info and keep that map of "SymbolEntries". When I want to access some variable, I would do that by name so I look up the entry by name and get all the info I need to read and write to the PLC variable directly with indexgroup and offset. The idea is to have functions like SymbolEntry::WriteFrom(T new value) and implement a switch case inside for conversion to TwinCAT types). This way it would be easy for "third party" users off AdsLib to write their own Wrapper objects around SymbolEntry. My idea is to have something like:


const auto symbolMap = SymbolAccess::FetchSymbolEntries()

const auto value = symbolMap["MAIN.myVar"].Read<uint16_t>();
symbolMap["MAIN.anotherVar"].Write(std::string>{"new value");
pbruenn commented 1 year ago

We merged an alternative implementation. See commit ee023f239976e1835dd4819c6f2fa026a15e417b and its parents