Open timyhac opened 4 months ago
Thanks for the great write up!
I think that I made a mistake when I supported more than the atomic types in the core DLL.
The core should support the base protocol. This includes connection handling, packing requests, handling fragmented requests and responses, and protocol-level error handling. And it should have provided a set of hooks to call for encode/decode. At this level requests and responses are just bags of bytes.
The only data convenience functions should have been the atomic types. And those should have been in a separate layer.
Unfortunately I started to support strings and that opened up a big can of worms. Any kind of structured data should not be the core's concern. Strings raise the difficulty even higher because they are inconsistent even across the same hardware.
Strings should not be supported in the core or the core wrapper. They make more sense to support as a library of example code. Even then you need to know the context. E.g. I've found that the same string type can be returned in at least three different formats with Omron PLCs.
And do not forget about BOOL arrays! Those are really complicated because even the meaning of the tag path changes depending on the manufacturer! 'MyBOOLArray[3]" means very different things on a Rockwell PLC vs an Omron PLC.
I wish I could do it over without breaking everything! Best, Kyle
— Reply to this email directly, view it on GitHub https://github.com/libplctag/libplctag.NET/issues/406, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4LC7PTWQPQ2O23ATNITDZMZCCNAVCNFSM6AAAAABLAK5SCSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQYTGMRUHEZDSOA . You are receiving this because you are subscribed to this thread. Message ID: @.***>
I have some concerns with this. (Note: Please correct me if I misunderstood anything in your explanation.)
We're currently using the custom mappers to handle UDTs quite extensively.
How do you imagine this would impact how UDTs are handled?
Would something in this new version of libplctag.NET handle UDTs itself in a way that wouldn't necessitate the mappers? If not, we would need to use this potential timyhac.libplctag.Mappers
package or maintain these classes ourselves? If i understand correctly Tag<M,T>
and IPlcMapper
, would move to be examples?
I'm concerned with needing more dependencies if the latter were the case. I would prefer that it remains part of the main library to needing another library to maintain.
Just to give a little more information regarding your mention of wrappers, we do have our own wrapper around libplctag.NET, but for the purpose of being able to support other PLC manufacturers and protocols like Siemens and OPC UA.
We have an IPlcComm
interface and then an implementation for each PLC type, currently LibPlcTagComm
and SiemensOpcUaComm
.
We define our tags in an XML file with a generic name that we use throughout our business logic. This name is defined with associated in the XML file, for each PLC driver that we support, with information used to generate the tag with (address, data type, etc.). Then I do some reflection to generate a dynamic typed Tag<M,T>
instance using the appropriate native data type and its corresponding PlcMapper. It may be a bit messy but it's working quite well for us. (Unfortunately I can't directly provide the code itself...)
Here again, we'd either need to rewrite all of this logic to use just what libplctag.NET provides or use this additional Mapper library and again, I'd prefer if this all just remained part of the library.
Hi @trinnan42 - thanks for the feedback!
You are correct in saying you would need to find an alternate implementation for the mapper system - either one you maintain yourself or a separate package. The proposal can be seen in this branch.
As I see it, the value of Tag<M,T>
and the Mapper system is:
For point 1, it sounds like you're already doing some logic to convert from a typed value back into dynamic
- so I don't think you're receiving much of that benefit.
For point 2, frankly I'm doing a terrible job at supporting scenarios other than the fairly specific Rockwell/Atomic tag scenario - and I don't really have a way to improve this other than wait for complaints to roll in.
Do you mind if I ask what value you get out of the Tag<M,T>
class?
Thanks so much for the detailed response to the post!
Any kind of structured data should not be the core's concern. Strings raise the difficulty even higher because they are inconsistent even across the same hardware. Strings should not be supported in the core or the core wrapper.
@kyle-github - yep 100%, and I think that the libplctag.NET wrapper using the libplctag name makes some implicit promise about what it does, which can be tricky to get to the bottom of when things go wrong.
@timyhac
Well, to try to explain with more detail what I'm doing without being able to share actual code:
Dictionary
instances which map these data types to C# native data types and the Mappers.Tag<,>
with the types from these dictionaries based on the Data Type Enum's value.Read<T>
and Write<T>
methods exposed to my code defined with my IPlcComm interface.Tag<M,T>
and then I just directly return Tag<M,T>.Value
for Read and set Tag<M,T>.Value
for Write.I guess I'm not exactly sure myself what value I'm getting out of Tag<M,T>
. When I began using this a couple years ago. I based what I'm doing off of the examples provided which all seemed to use Tag<M,T>
. It was my assumption that this was how the library was meant to be used. I never used or looked at the base C library, so I didn't know what it offered.
Since I'm not familiar with libplctag itself, I guess what I'm not sure about is how I would do things without Tag<M,T>
. Would we use just the Tag class and then just use its Set[Type]
and Get[Type]
methods like in this example?
It seems I'd have to replace this code with something like a switch statement that just uses the base Tag.Set[Type]
method based on the type I'm trying to read?
We're also doing a lot with arrays of tags and UDTs. That support is provided by the PlcMapper system, right? Otherwise we'd be looping through and reading the tag with these base Set and Get methods? I think the PlcMapper system is indeed preventing us from writing some of that code ourselves.
We're also using the code from the NotifyChanged
example to keep things current. Is that something specific to Tag<M,T>
?
I was somewhat watching the recent problems someone was having with Omron, is that part of what's prompting this? The differences between how the different PLC manufacturers handle the specifics and the mappers not handling things properly between this differences?
We do intend to support Omron as some of our customers do require their PLCs, though we haven't made any attempt to implement this so far. I was planning on attempting to use OPC UA (and that might be our path forward for all PLCs given the cybersecurity pushes for secure protocols that we're getting from some customers). It seems Rockwell is slow to adopt OPC UA and sounds like they want to charge extra for it, unlike Siemens who just include it in their CPUs.
Thanks @trinnan42 - this is great info!
When I began using this a couple years ago. I based what I'm doing off of the examples provided which all seemed to use Tag<M,T>. It was my assumption that this was how the library was meant to be used.
100% correct.
Although Tag
has always been a type available to you, Tag<M,T>
was the one we used in all of the examples and I promoted.
We're also doing a lot with arrays of tags and UDTs.
100% correct and this is where the Mapper system shines. Yes you would need to find some other way to do this.
the PlcMapper system is indeed preventing us from writing some of that code ourselves.
100% correct. There are some major limitations with the Mapper system:
Tag
instead. Many issues have been raised against this.AutoSyncWriteInterval
doesn't work.Solving most of these issues would have been simpler if we instead used inheritance. In retrospect this would have been a better choice. Having said that, my opinion is that libplctag.NET shouldn't ship that alternative, because it is not obvious what methods/properties the base class should expose, and it is trivial to write your own (particularly if there is an example to copy).
I was somewhat watching the recent problems someone was having with Omron, is that part of what's prompting this?
It certainly is another example of the issue that library consumers face but we have had these problems for years. For example, #212 has been open for almost 3 years.
I'm concerned with needing more dependencies if the latter were the case. I would prefer that it remains part of the main library to needing another library to maintain.
This might not be the point you're making but potentially there is a case for remaining; in that you could argue being "official" it is more likely to get exposure, trust and momentum around it, and therefore would be maintained at a higher level.
I create a dynamic tag variable by creating an instance of Tag<,> with the types from these dictionaries based on the Data Type Enum's value. Would we use just the Tag class and then just use its Set[Type] and Get[Type] methods ...
You can absolutely sidestep the Mapper system (even as libplctag.NET currently stands today) and do something like this:
enum MyTagType { SINT, INT, DINT /* etc */ }
class MyTag
{
private readonly Tag _tag;
private readonly Func<dynamic> decoder;
private readonly Action<dynamic> encoder;
public MyTag(MyTagType tagType)
{
_tag = new Tag()
{
PlcType = PlcType.ControlLogix,
Protocol = Protocol.ab_eip,
};
switch (tagType)
{
case MyTagType.SINT:
_tag.ElementSize = 1;
decoder = () => _tag.GetInt8(0);
encoder = (value) => _tag.SetInt8(0, value);
break;
case MyTagType.INT:
_tag.ElementSize = 2;
decoder = () => _tag.GetInt16(0);
encoder = (value) => _tag.SetInt16(0, value);
break;
case MyTagType.DINT:
_tag.ElementSize = 4;
decoder = () => _tag.GetInt32(0);
encoder = (value) => _tag.SetInt32(0, value);
break;
}
}
public Task InitializeAsync() => _tag.InitializeAsync();
public Task WriteAsync() => _tag.WriteAsync();
public Task ReadAsync() => _tag.ReadAsync();
public object Value { get => decoder(); set => encoder(value); }
public string Gateway { get => _tag.Gateway; set => _tag.Gateway = value; }
public string Path { get => _tag.Path; set => _tag.Path = value; }
public string Name { get => _tag.Name; set => _tag.Name = value; }
}
Which would be used like this:
var myTag = new MyTag(MyTagType.DINT)
{
Name = "PROGRAM:SomeProgram.SomeDINT",
Gateway = "10.10.10.10",
Path = "1,0",
};
await myTag.ReadAsync();
var originalValue = myTag.Value;
Console.WriteLine($"Original value: {originalValue}");
int newValue = 3737;
myTag.Value = newValue;
await myTag.WriteAsync();
Console.WriteLine($"New value: {newValue}");
@timyhac Your explanations are much appreciated! I've got a better understanding now. This certainly alleviates my concerns.
As a recent user of libplctag.NET, I too have to quote this:
When I began using this a couple years ago. I based what I'm doing off of the examples provided which all seemed to use Tag<M,T>. It was my assumption that this was how the library was meant to be used.
Accordingly, I developed many custom mappers in order to work with all my custom datatypes and even for Omron system structure (_sAXIS_REF, etc.), Omron being the brand I use.
How I "share" tag values in my application is actually a mess, basically I register them in a list and access their values with a LINQ method finding them by name. I couldn't come up with a better idea at the time, and the I stayed with that.
Following this with interest.
Thanks for your input @MountainKing91!
A study of applications or libraries that are consuming libplctag.NET shows that most will create yet another wrapper around Tag<M,T>.
@timyhac do you do this only by github search or is there anything else? I.e. user sharing their application or something?
@MountainKing91 - just github/google.
@trinnan42, @MountainKing91 and @mitchrazga thankyou so much for your engagement and input!
I believe that removing the Mapper system and associated classes is still the right call to benefit the libplctag community long term, but have decided to phase out the mapper system in two stages to ease the transition:
Tag
instead of Tag<M,T>
.As you suggested, much of the issue is that the examples and documentation promote the use of the mapper system - so stage 1 will mitigate that part of the problem - while allowing current users to gradually transition.
For stage 2, I have not yet put in place an alternative for current users to migrate to, but I am thinking it will be a separate package but a drop-in replacement - it would be a separate package, and would not be an "official" .NET wrapper.
Let me know your thoughts on how to set up a separate "extras" package. I am really interested in doing that for the core DLL. I want to pull out the examples, tools and tests (which are horribly mixed up now) from the main library.
Heya @kyle-github
The examples and tests would stay in the libplctag.NET github repository, but I was thinking that the classes shipped with the nuget package that I just deprecated would be moved to a separate repository and released as a separate nuget package from there. The purpose being to make it clear that it is not a "libplctag" initiative.
A separate but related thought was that there could be a repository which does attempt to document the behaviour of the various devices the libplctag community uses. Its purpose would be to be a store of knowledge for the Modbus and Ethernet/IP services that those devices offer. A non-goal would be to be comprehensive and to work out-of-the-box, but instead to document what the community has learnt so far, and to explicitly document where the knowledge gaps are. This knowledge could be documented as executable programs - so that others could easily run the tests and confirm whether it is true on their device, or whether there is a gap in this knowledge that needs to be clarified.
Hi guys, I'm looking at the examples, trying to move past using the mappers.
I see in the examples folder that there are now two classes BaseTag.cs and Definitions.cs. Is this intended to be an alternative to mappers or am I missing something? I will probably have the chance to start a new project with Omron + Libplctag, and I would like to start "fresh" and not use mappers if possible, even if until now I heavily relied on them.
Ty!
Yes exactly - in the top post I said a little bit about it under the "What's the alternative?" heading in the top post on this issue.
A few years after the release of the official libplctag.NET wrapper, a pattern has emerged that all library consumers face a key problem;
In this ticket I will be making the argument that the Mapper system (
Tag<M,T>
,IPlcMapper
, etc) provided by libplctag.NET has magnified this problem by obscuring/hiding it from library users. This is to the detriment of those users, reduces the ability of the libplctag community to assist those users, and increases the burden on library authors - and therefore should be removed.Introduction
libplctag handles the details of communicating with PLCs over Modbus or Ethernet/IP Explicit Messaging, so you can easily access the value of PLC tags.
This statement is mostly true; while it is correct that you (most of the time) do not need to understand the protocol itself, you do need to understand the device's Modbus or Ethernet/IP API, as not all devices are the same.
This is similar to using a HTTP REST API - while you do not need to understand the details of TCP/IP or even HTTP, you definitely need to know what data to send and how to interpret the response.
For example; you need to know how to configure the EthernetIP or Modbus requests by using an appropriate tag "name", path/gateway, and other attributes, and how the device will structure its response (a byte array) to encodes tag values. For basic cases such as Atomic tag types (e.g. DINT, REAL) - this is usually straightforward and you only need to use a single Data Accessor method to extract/write the value, furthermore these are usually common across devices and for tag names. UDTs, STRINGs, BOOLs and arrays however, are non-trivial.
In general, you will need prior knowledge of how these tags are structured. This knowledge needs to come from device manuals or from reverse engineering efforts.
The good: it handles a common case well!
In
Tag<M,T>
, theM
is aPlcMapper
which converts between a byte array (the tag buffer) and theT
.NET type and makes it possible to implement the Decode/Encode logic for a data type once, and then re-use it for 1D, 2D and 3D arrays as well as the atomic type. For this use-case it excels. This system can even be used for UDTs with static sizes. The value is strongly-typed, which holds significant value in the .NET ecosystem and particularly C#.It is entirely possible that this serves the silent majority of users perfectly - they have no complaints, and we do not hear from them. They have used the system as it was intended and moved on with their life.
It's utility stems from being able to instantiate a strongly-typed Tag, simply by providing a Mapper class type (and most of the time this is built-in). And even this can be hidden because we also ship "simple" classes.
The complexity cliff
Tag<M,T>
makes a (somewhat implicit) promise that you as a consumer do not need to understand how PLC data is encoded in the buffer, as thePlcMapper
handles that for you. However, over time it has become clear that there are additional capabilities that PLCs offer, and that library consumers need, that the Mapper system doesn't fulfil.At the time of development, the Mapper system was consistent with the core libplctag API, and was universally applicable (at least within the confines of libplctag.NET developers' limited exposure to other PLC vendors). The key insight was that single values (whether they be atomic or UDTs) and arrays of values can use the same decoding logic. The logical conclusion is that much of the setup of tags can be contained within
PlcMapper
classes, andTag<M,T>
merely needs to expose the Read/Write commands and events. This abstraction falls off a "cliff" as soon as you go outside the simplest of cases. But the promise remains - the PlcMapper is meant to do it, and it does not - therefore it is a bug in thePlcMapper
orTag<M,T>
.Ultimately, using libplctag does not preclude you (the library consumer) from understanding the device's Ethernet/IP API.
Consumers will encounter this working
MyBoolArray
andMyBoolArray[0]
return different data on Omron).If we want to continue to make the promise that consumers do not need to understand their device's Ethernet/IP API, the only path forward for this project is to wait for complaints to arrive, and incrementally add handlers for specific cases into the mappers. The mappers would effectively become a documentation store for this knowledge.
Its another API to learn, support, test and maintain
The good news is that consumers are not locked into using the built-in mappers. If they are able to understand how their devices behave, they can add custom mappers.
However, this is another API that consumers must learn. The Mapper system uses some somewhat advanced C# language features which can be tricky for early-career developers to understand and implement.
If there are features that consumers want (such as value change detection), consumers could spend a considerable amount of time trying to determine how libplctag.NET supports it (which it may not), rather than just implementing it themself.
Furthermore:
It would be simpler if that layer was removed, and consumers directly interfaced with
Tag
- it confronts them with the complexity of the underlying API and does not hide any of the "foot-guns" (which exist whether they can see them or not).Many consumers still add their own wrappers
A study of applications or libraries that are consuming libplctag.NET shows that most will create yet another wrapper around
Tag<M,T>
. This implies that it is not the optimal level of abstraction and doesn't add much value.Some examples:
What's the alternative?
Use an abstract base class and handle the specific data types with derived classes.
This has several advantages over the
Tag<M,T>
+PlcMapper
system:The issue with this is that there is not an obvious universal interface that would serve as the base class. As an example, should the Tag value be exposed as a .NET type such as
int
or should it beobject
- or should it be exposed at all? What if the type was an array, and instead of exposing the entire array, the library consumer would prefer to force their application code to go through a method with an indexer (so that it can raise events or run other logic).Summary
While
Tag<M,T>
has utility for a common use-case, the API it exposes is not the lowest-common denominator. Having it is a burden on library consumers and maintainers alike.I recommend:
ITag
,Tag<M,T>
, all Mappers and Simple Tag classes, and also not supplying an alternative like an abstract base type.Tag<M,T>
and a demonstration of an alternative like an abstract base mapper type to the Examples folder.If this happens, the libplctag.NET would be closer to a "wrapper" in spirit, rather than what could be argued to be an opinionated framework for working with libplctag.
I would be happy to offer a separate Nuget package for the Mapper system, but not as an "official" package by the libplctag organization, instead released under a "timyhac" brand such as
timyhac.libplctag.Mappers
I've created this as a Github issue to open it up for community feedback. I realize that there are many users of the Mapper system at this point and I want to take into account the impact on them.