OOI Refactoring: Removed dead code and split up into several files

[mwiarda]

I was just looking at the problem of destroying the variables of a client when the client object is destroyed. I actually don't like to have the variables coupled to a client and then still floating around in space after destroying the client. This could lead to mistakes when the client goes out of scope but the variables are still in use (Port already closed, no route to host, ...). I would prefer some kind of interface that allows me to create variables that manage a client object in the background. The client should be completely invisible to the average user. I just get my variables from somewhere and when all my variables go out of scope my client dies. Looking at the port problem I found last night I would prefer something like this: AdsVariable::AdsVariable(netAddr, symbolName) { static AdsClient client; ... } This leaves us with a client that only dies when the process is terminated. But the average user would not have to think of the lifetime of the client and variables. Only question is... Where would we place something like ReadDeviceInfo? Namespace function? And there is still the repeating of the netAddr parameter...

What do you think?

[pbruenn]

I would prefer a OOI without AdsClient, too. All the AdsClient does is managing the AmsPort(OpenPortEx) for us, so we could easily move that handling inside of AdsVariable. Having all AdsVariables share the same local port shouldn't be a problem. We can think of something smart, when we create the AdsNotification object. But I guess a simple implementation would just use a new port when the 500 notifications limit is reached. Or pass an optional parameter.

I would simply create another specialization for AdsVariable<DeviceInfo>

I would deal with the repeating netAddr parameter similar to how the linux kernel deals with such repeating parameters:

#define PCI_DEVICE(vend,dev) \
         .vendor = (vend), .device = (dev), \
         .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID

http://lxr.free-electrons.com/source/include/linux/pci.h#L697

static const struct pci_device_id i801_ids[] = {
         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_3) },
         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_3) },
...
         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS) },
         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) },
         { 0, }
};

http://lxr.free-electrons.com/source/drivers/i2c/busses/i2c-i801.c#L858

Personally I would have made it even simpler by adding:

#define PCI_DEVICE_INTEL(dev) \
         PCI_DEVICE(PCI_VENDOR_ID_INTEL, dev) 

[mwiarda]

For the notification limit we'll sure find some solution.

I'd rather use something like Ads::GetDeviceInfo(...) than a template specification. It is read only right? How about How about an AdsDevice class instead? That class could incorporate GetDeviceInfo, SetState(AdssState), GetState().

I don't really know where you want to go with the preprocessor macros. It makes sense in the intel case. But that would have to be implemented by the library user. Like #define GET_LOCAL_ADS_VARIABLE(PATH) GetAdsVariable("127.0.0.1.1.1", 851, PATH) auto x = GET_LOCAL_ADS_VARIABLE("MAIN.Variable1")

Did you have something else in mind?

[pbruenn]

AdsDevice is a good idea, I would go with that.

Yes, a macro like that. Everyone would be free to make his own. We could just show one in the examples. The version I had in mind would look like this:

#define PLC_TASK_X "127.0.0.1.1.1", 851
auto x = GetAdsVariable(PLC_TASK_X, "MAIN.Variable1");

[mwiarda]

Ok that makes sense. I just didn't see any way to force users to use it.