Closed mwiarda closed 7 years ago
Hi Michael, I am just back from vacation and had a short look at it, but my first impression is very good. I will pull it to a new branch and test a little with different platforms. Regards, Patrick
I had a closer look and got some questions:
struct Child : Parent {
over:
class Child : public Parent {
public:
Hi Patrick,
hope you had a nice, relaxing vacation. I can't experiment much at the moment since I'm on vacation this week. However, I played around with a few possible solutions before submitting.
Thanks for your feedback! Regards, Michael
On 22 Aug 2016 4:11 p.m., "Patrick Brünn" notifications@github.com wrote:
I had a closer look and got some questions:
struct Child : Parent {
over:
class Child : public Parent { public:
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Beckhoff/ADS/pull/35#issuecomment-241425093, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFL2mwSFsTGVcVqn4zzLgwxYbTzeKwks5qia4tgaJpZM4JbGR1 .
Just had another thought. Maybe it is a bad idea to implement count as a template parameter. This limits the use of the function to cases where the size is known at compile time. There might be cases in which you want to read the size at runtime for example to limit network usage by only reading parts of an array.
What do you think?
Thanks, my vacation was fine and relaxing, I hope you take your time, too. I am fine with further discussion next week. I commited ff0047ac2437971a0fa1eef789b0c2c67dd179a5 to show what I mean by 1. For me it feels more clean to have a dedicated AdsClient (maybe we need another name) object for each pair of remote AmsAddr + AmsPort. When we follow that direction, I think we always hit some ownership, resource leakage problems, if we try to implement the OOI on top of AdsLib.h. In my opinion we should access AdsRouter.h directly. And maybe advance it to allow allocation of AdsClient objects from it such like: AdsClient& AmsRouter::AdsPortOpen(AmsAddr, IP). That was the idea of AmsPort, but then I realized TcAdsDll really allows to send to different AmsPorts/NetIds by the same "AmsPort", I had to implement this flexibility, too. Maybe now is the time to remove that, at least for the new object oriented interface?
My opinion about the array functions is: I don't like them. The code looks to redundant. And as you stated we are passing them by value. Today I tried different approaches to combine the arrays and the normal functions but in the end an interface with a signature like this: (void *buffer, size_t buffer_size) seems the most appropriate solution to me.
Another open point is potential resource leakage and wasting with GetHandle()/ReleaseHandle(). I would like to encapsulate that inside of a handle_guard (something like std::lock_guard).
And when we are about handles. Is it really a good idea to get a new one for each read/write? Or should AdsClient keep track of them? What about an even more crazy interface like this:
Handle<std::array<uint8_t, 4>> handle{remoteNetId, remoteIpV4, "MAIN.byByte"};
//or
Handle<std::array<uint8_t, 4>> handle{adsClient, "MAIN.byByte"};
handle.write({1, 2, 3, 4});
bool success = (handle.read().value[0] == 1);
In case this is really useful, it could be easily advanced for access by indexgroup/offset. But to be honest I have no idea how your real world applications look like and if they would benefit from such an interface (Thats what I meant here #26).
commit 39d2ea2acd11f76632f5c00f30f083e49a9e86a1 should show what I meant with "handle_guard". The diff is a little unlucky, as I had to reorder GetHandle()/ReleaseHandle(), but on a closer look it should show, there are just little changes in these functions.
I finally found the time to have a look at your ideas.
1) The handle_guard is a good idea. I really didn't think about leaking handles.
2) Binding a Client to an AmsAddr and port is fine for me. I hate passing redundant information to the functions.
3) Where exactly do you see the resource leakage problems that arise from using AdsLib.h?
4) I agree that the array functions are a violation of the DRY principle. BUT: I really don't like the (void* buffer, size_t buffer_size)
interface type. The C++ Core Guidelines propose using something like span
Imagine:
auto intVar = AdsVariable<uint32_t>(netId, port, symbolName); // Connects to ads variable (gets handle?)
intVar = 1; // Writes 1 to remote variable
uint32_t x = intVar; // Reads value of variable and saves it to x
auto intArr = AdsVariable<uint32_t>(netId, port, symbolName, count);
intArr[0] = 1; // Writes to index 0
uint32_t x = intArr[0]; // Reads only value at index 0
std::vector<uint32_t> x = intArr; // Reads array to vector
What do you think?
Regards Michael
Forget 3. I played around a bit and don't expect a problem anymore.
(void* buffer, size_t bytes_in_buffer)
would make clear we are pointing to some raw continues memory with at least bytes_in_buffer. That interface could be used by f.e. AdsVariableI didn't have too much time but the interface idea intrigued me. So I played around a little without any testing.
Here is my first result: https://gist.github.com/xift/7bf356b44556cfba07ee1b639f5c0f04
There are some shortcomings but I really like that the following code at least compiles:
void test()
{
const AmsAddr adsAddr;
AdsVariable<int> var(adsAddr, "test", 10); // int array
AdsVariable<char> var2(adsAddr, "test"); // char variable
int readValueFromArray = var[1];
std::vector<int> readCompleteArray = var;
var[2] = 23; // Write to array
var2 = 'A'; // Write to value
char readValue = var2;
}
What do you think?
I tried to integrate that into dev-ooi shortlog:
still missing:
preview:
AdsClient adsClient {remoteNetAddress, remoteIpV4};
auto simpleVar = adsClient.GetAdsVariable<uint8_t>("MAIN.byByte[0]");
simpleVar = 0x99;
std::cout << "Read " << (uint32_t)simpleVar << '\n;
auto simpleVarIndex = adsClient.GetAdsVariable<uint8_t>(0x4020, 0);
simpleVarIndex = 0x88;
std::cout << "Read " << (uint32_t)simpleVarIndex << '\n;
auto arrayVar = adsClient.GetAdsVariable<std::array<uint8_t, 4> >("MAIN.byByte");
arrayVar = std::array<uint8_t, 4>{ 1, 2, 3, 4 };
std::array<uint8_t, 4> readArray = arrayVar;
auto arrayVarIndex = adsClient.GetAdsVariable<std::array<uint8_t, 4> >(0x4020, 0);
arrayVarIndex = std::array<uint8_t, 4>{ 1, 2, 3, 4 };
std::array<uint8_t, 4> readArrayIndex = arrayVarIndex;
Looks nice!
On the missing parts: 1) Definitely 2) I started with that while playing around. 3) Definitely 4) Remembering the variables and cleaning up later should not be a problem.
And now a problem I ran into when playing around a little: My test PLC project has several inputs and outputs. Application port is the usual 851 and the tasks symbol port is 350. Here is the problem. When I read or write arrays by name I have to use the application port and when I use index group and index offset I need to use the symbol port. I basically have to set up two separate AdsClient instances for that. It is highly inconvenient and the error message when using the wrong port (1793) is too cryptic to easily spot the mistake.
To visualize the problem:
How can we solve that? 1) Add symbolPort to the AdsClient constructor 2) New GetAdsVariable parameter: GetAdsVariable(symbolPort, indexGroup, indexOffset)
Regards Michael
Sidenote: AdsClient.h does not compile with Visual Studio 2015. It doesn't like the HandleGuard.
return AdsHandleGuard {new uint32_t {handle}, std::bind(&ReleaseHandle, address, port, std::placeholders::_1)};
No instance of constructor "std::unique_ptr<_Ty, _Dx>::unique_ptr [with _Ty=uint32_t, _Dx=std::_Binder<std::_Unforced, void ()(AmsAddr address, long port, uint32_t handle), const AmsAddr, long, const std::_Ph<1> &>]" matches the argument list. Argument types are: (uint32t , std::_Binder<std::Unforced, void ()(AmsAddr address, long port, uint32_t *handle), const AmsAddr &, long &, const std::_Ph<1> &>)
You seem to be quite familiar with modern C++. Do you often use
SInce you opened the OOI branch I guess we can close this pull request and start a new one for the OOI branch.
I split your last comments into two new issues to track them further:
Hi,
I proposed an object-oriented interface for the library earlier. This is my first shot.
My attention was to reduce the hassle of using the current interface. For me, the major drawbacks of the original library are:
1) Handling of memory buffers 2) Function call order (OpenPort, Do something, close port) 3) Too many parameters for function calls 4) Error codes
I tried to solve them like this: 1) Using shared_ptr to allocate memory internally 2) Using RAII 3) Use the type system to deduce type sizes and return values instead of output parameters 4) Generate exceptions instead of returning error codes
The whole thing is a header-only addition to make it easy to include. Every feedback is welcome.
Regards Michael