dev-ooi: Usage of AdsRoute not intuitive

[mwiarda]

Hi, I just read through the example code. Love the easy way to establish a route using the AdsRoute class. However, if you forget about it you just get the TargetMachineNotFound error code when you try to create an AdsVariable for the first time. I believe that a good API would prevent me from making a request without having a route.

Currently trying out some ideas and options. But haven't found a good one yet :-( Best one so far goes along the lines of replacing the AmsAddr with AdsRoute in the AdsVariable<> constructor.

AdsLocalPort is better, but I would love the handling to be internal and not pass it every time I do something. So if you request a variable for the first time a port is opened.

Regards Michael

[pbruenn]

Yes I think there is no easy or correct solution to this issue. If we make the API more fool proof, we introduce more implicit limitations (# of variables, routes, singletons). In my opinion the root of this problem is this ultra flexible AdsPort inherited by TcAdsDll. Where you just allocate some local resources for notification handling. But on each AdsRead/WriteRequest you are free to change your destination [NetId, port]. Making the AdsLocalPort handling automatic, would require one more static global singleton, which I would like to prevent. I understand the annoyance to pass it to each AdsVariables construction but with a macro like we discussed in #37 that should go away. If we sacrifice the ultra flexibility of AdsLocalPort we could move the route and remote addr handling into that object. This would reduce the AdsVariable parameters and give us auto handled routes.

[mwiarda]

If we sacrifice the ultra flexibility of AdsLocalPort we could move the route and remote addr handling into that object. This would reduce the AdsVariable parameters and give us auto handled routes.

I'll have a look at that later.

[mwiarda]

How about calling it AdsRoute? It should be obvious for the user that you need a route to communicate with the PLC.

auto route = AdsRoute(ipV4, netId, taskPort, symbolPort);
auto var = AdsVariable(route, symbolName);
auto var2 = AdsVariable(route, indexGroup, indexOffset);

[pbruenn]

Sounds good to me.

[mwiarda]

I pushed a new iteration of the ooi to my branch. It contains lots of changes. Its still far from perfect. The whole TaskPort and SymbolPort thing is way too messed up. Notifications seem to work only if you have a handle to a symbol on my system. Not with indexGroup and indexOffset. Is that normal?

Have a look at the change if you got the time. I will need some more time to think about it. Unfortunatly, I have a lot on my table right now.

[pbruenn]

Same here ;-). But today I finally took the time to have a look. First of all thanks for sharing and please don't get frustrated by the amount of critics, I am just a very pedantic person, but I really appreciate, that we are working together on this interface!

Again I learned something: enum ADSSTATE : uint16_t

But now the critics: It's very hard for me to follow your changes, as you put the 'split into multiple files' and general code changes in the same commit. Please split that into multiple commits next time, that would make it much easier for me (and I guess for others following us silently ;-).

Personally I don't like these getter functions for constant members like: const std::string AdsDevice::GetName() const I would make them just public and remove the redundant lines of code. Even this small functions might introduce unwanted sideeffects. Or is a constant copy of that string really what you want?

Another constant copy, do you really mean: const AdsDeviceState AdsDevice::GetState() or const AdsDeviceState& AdsDevice::GetState() or AdsDeviceState AdsDevice::GetState()

I would replace:

class AdsDevice {
public:

with

struct AdsDevice {

just to save a line ;-)

What do you think of: void AdsDevice::SetState(const AdsDeviceState state)

I merged your changes to dev-ooi, please take a look and let me know if I missed something. I reorganized the source tree to clean up the redundant versions of our new OOI files. The idea is to have a new static library AdsLibOOI on top of AdsLib (or even TcAdsDll). As the number of include files increased dramatically I changed the 'copy lib headers to example dir' with 'add lib dir as include path' for the example project. To increase the testing of AdsLibOOI I added a copy of AdsLibTest and ported the testcases to OOI. At least until I reached the notification tests. It seems to me you stopped there, too. I will continue to work on the Notifications, but thought this would be a good break to push my changes online to share with you. I think we shouldn't make AdsNotification depend on an AdsVariable. I believe the usecase is to either have a AdsVariable or an AdsNotification, but not both at the same time. What was your idea by adding a AdsVariable to the notification?

[mwiarda]

Don't worry, I knew you'd say that and you are right. The chaos happened when I started reorganizing the whole thing and touching everything at once.

To your changes: 1) Agreed. I'm not a friend of useless getters and setters myself. 2) I really meant const AdsDeviceState AdsDevice::GetState(). The function gets the state from the device and then returns it (const because a change would have no meaning for the real device state). I know that creates a temporary copy. However, I didn't want to return a reference to prevent lifetime issues (reference outlives object) and to explicitly say "this does not automatically update". 3) Agreed 4) Agreed, what was the adsState for anyway? The documentation doesn't say anything afaik. 5) I will have a look at the structure later. 6) I guess creating an AdsNotification with just one parameter was tempting :-) I don't mind separating that.

What really bothers me is the two ports numbers needed for accessing variables and notifications depending on wether they were created using the symbolname or indexgroup and indexoffset. Do notifications even work for indexgroup and indexoffset pairs? I always got "device does not support this service".

[pbruenn]

1. I would replace the two port numbers with two different AdsRoute instances. So If you want to use a symbolName pass the symbolRoute to your AdsVariable, if you use Index then the other indexRoute.
2. Notifications should work for both symbols and index. See my latest commit's to have an example in the notification tests of AdsLibOOITest.
3. Right now, the last issue, I am working on is the concurrent access to AdsAddRoute/AdsDelRoute. As we want to make AdsRoute fool proof, we have to allow the exact same route shared by different AdsRoute instances. So we can not just call AdsDelRoute in the destructor of AdsRoute, but add some reference counting.

[mwiarda]

1. It would be much nicer if the user would not have to know about which route to use.
2. I'll check that.
3. That was my initial thought as well. I have not tested that properly but I did some quick checks and sharing routes worked perfectly. The router does some reference counting and handled all cases I threw at it.

[pbruenn]

1. But the target port number always depends on your target. If you implement your own AdsServer and want to connect to him you would have to pass his port number. I think that's the case in your setup, as I can use the same port number for access by symbol and index.
3. create two instances of AdsRoute with the same parameters, use parallel threads, then delete one of the instances while the other thread is waiting for a Notification. That should crash as the AmsConnection is freed before the response returned. At least TestAdsPerformance::testManyNotifications crashed deterministic even with only 2 threads and one notification per thread.

[mwiarda]

1. It depends not only on your target but also how the variable is defined and where it is allocated. In the test project the test array is allocated in the marker area (%MB0). That works fine. I'm not sure if it still works for %Q*. It definitely doesn't work for C++ data areas.
2. Ok. I'll test that later.

On Wed, Oct 26, 2016 at 12:12 PM, Patrick Brünn notifications@github.com wrote:

1. But the target port number always depends on your target. If you implement your own AdsServer and want to connect to him you would have to pass his port number. I think that's the case in your setup, as I can use the same port number for access by symbol and index.
2. create two instances of AdsRoute with the same parameters, use parallel threads, then delete one of the instances while the other thread is waiting for a Notification. That should crash as the AmsConnection is freed before the response returned. At least TestAdsPerformance:: testManyNotifications crashed deterministic even with only 2 threads and one notification per thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Beckhoff/ADS/issues/40#issuecomment-256305630, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFLxjRrSgBOODkxWNEYhNSSRzLtoI9ks5q3yeagaJpZM4KF_0G .

[pbruenn]

1. Yeah, that's kind of what I mean. As the port number depends on "where" you have to get the variable from. I would suggest to add a new AdsRoute for each of this places. To me the task/symbolPort solution looks like the attempt to capture two targets with one AdsRoute. What if you have three? That's, why I think the one location (target port) per AdsRoute approach is the most natural one.

[pbruenn]

Today I started some cleanups and refactoring. I hit a point where I think it would be good to get rid of having two ports (task/symbol) in AdsRoute, to do further simplifications. But I want to give you time to convince me from keeping it as is. So I will grab this opportunity and start working on another project for the next week, giving you some time to grab arguments. After that my next steps would be to move more code into AdsRoute, to reduce the redundant AmsAddr members all over in AdsDevice, AdsHandle, etc. My idea is to have something like this:

void AdsDevice::SetState(const ADSSTATE AdsState, const ADSSTATE DeviceState) const
{
    m_Route.WriteControlReqEx(AdsState, DeviceState);
}
...
void AdsRoute::WriteControlReqEx(const ADSSTATE AdsState, const ADSSTATE DeviceState) const
{
    auto error = AdsSyncWriteControlReqEx(m_LocalPort,
                                          &m_SymbolPort,
                                          AdsState,
                                          DeviceState,
                                          0, nullptr); // No additional data

    if (error) {
        throw AdsException(error);
    }
}

On the first look this seems all like bloat, but I have a feeling that we can reduce the overall complexity even more with that approach and might end up with a very handy OOI. So that's something I want to try out (and throw away if it doesn't work out). Do you have any additional use case which is not yet covered by AdsLibOOITest? Regards, Patrick

[mwiarda]

I guess you are right. You just have to know where you want to get the variable from...

On Thu, Oct 27, 2016 at 9:15 AM, Patrick Brünn notifications@github.com wrote:

1. Yeah, that's kind of what I mean. As the port number depends on "where" you have to get the variable from. I would suggest to add a new AdsRoute for each of this places. To me hte task/symbolPort solution looks like the attempt to capture two targets with one AdsRoute. What if you have three? That's, why I think the one location (target port) per AdsRoute approach is the most natural one.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Beckhoff/ADS/issues/40#issuecomment-256565100, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFL188hwgttFEWdxfKVGeIo8sIySK_ks5q4E-TgaJpZM4KF_0G .

[mwiarda]

I know what you mean. Putting the low-level requests into the route might work. Can't decide if I like it though. Lets just see how it turns out.

I would really like to get rid of handling with AdsNotificationAttrib structures (at least as the default way). That's why I added the derived notification classes. What do you think?

At first glance I don't see anything missing from the tests. I like how we got rid of 300 lines of code and a lot of tests by simply changing the interface.

[pbruenn]

1. Yep, I try that next week
2. I don't like that AdsNotificationAttrib, too. If we find an elegant way to get rid of it, we should do. And I reconsider adding your classes back again (I just didn't like the GetNotificationAttribs() on the first look).
3. Me, too. Within the next weeks I will collect some more input from other application experts. I guess they have additional input of what is needed, and how it could be further improved. But I think with your help we build a good basis for further development.

[pbruenn]

Happy New Year ;-)

It took a while, but finally I got some time for further refactoring:

• symbol/taskPort converted into a single port
• AdsDevice is merged into AdsRoute
• refactored automanaged resources such as symbol handles, notification handles, ads port handles, etc as AdsResource

A new feature is "hostname support", which is already available on master, too.

From the discussions with application engineers I learned that our AdsRoute is better described as AdsDevice. I didn't do the rename just now, to avoid confusion. But it is planned for the future. A must-have feature of such an AdsDevice(AdsRoute) Object is to automatically manage device state changes. So you connect to some remote AdsDevice, get your handles, symbols, etc. and then the remote does a mode change from Run->Config->Run. Now, all your handles would be invalid, and you have to get them back again. AdsDevice should do that job for you.

It's on my list for the next time block for dev-ooi.

Regads, Patrick

[mwiarda]

Hey, I'm back :-)

Had a lot of personal stuff going on. I'll look into the renaming of AdsRoute to AdsDevice next. Ok?

Regards, Michael

[mwiarda]

Added pull request #45 . Renamed AdsRoute to AdsDevice. I don't see much confusion there. Next step would be the management of device state changes. I might look into that after the weekend.

Regards, Michael

[pbruenn]

Hi Michael, welcome back ;-). Today, I did some rebasing, to make it easier to track the difference between dev-ooi and master. I did cherry-pick some commits in the past, but today showed there are still some changes I want to merge back to master. Please take a look at ooi-rebase-v5? My plan would be to merge non-ooi patches from ooi-rebase-v5 to master. And then create a new branch 'dev-ooi-r1' from dev-ooi and rebased it on master. OOI development should then continue on dev-ooi-r1, until we rebase again with dev-ooi-r2.... What do you think of that idea?

Regards, Patrick

[mwiarda]

Hi Patrick,

if you want to keep those two branches separated that is the way to go. However, another option would be to merge the changes into master and continue development there. The two parts are relatively independent of each other. At least the core functionality of the main branch is untouched. On first glance it seems that the example code is the only thing that would have to be merged manually. On second thought, you are probably right to put the OOI stuff in another branch just to mark it as experimental.

I'm fine with both options.

Hope you had a great weekend in the sun! Michael

[pbruenn]

Hi Michael,

yes, I want to keep OOI on a separate branch as long as there is a good chance, that we introduce breaking chance to the API. I believe the current version on dev-ooi-v2 is a good base for future development. But as we still have basic features (auto-reconnect, maybe routing, ...) to implement, the risk is to high, we need to change the existing API of OOI. I will lock down dev-ooi, now. Use dev-ooi-v2 for future development. I will close this issue, too. And open a new one #46 for our missing features.

Hope you had a nice weekend like me, Patrick p.s.: do you need Win64 and Win32 builds? Do you need Release and Debug configuration? Testing all VS configurations is a real pain for me. So if we could reduce the possible build combinations I would be very happy.

[mwiarda]

Sounds good to me. X86 builds should be enough I guess. For now Debug should also be sufficient. We can add the Release configuration back later.

Michael