Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Mapper system (Tag<M,T>, IPlcMapper, DintPlcMapper, TagReal etc). #406

Open
timyhac opened this issue Jul 17, 2024 · 14 comments
Open

Comments

@timyhac
Copy link
Collaborator

timyhac commented Jul 17, 2024

A few years after the release of the official libplctag.NET wrapper, a pattern has emerged that all library consumers face a key problem;

What is the binary format of a tag buffer (a byte array)? How does it encode the tag's value?

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>, the M is a PlcMapper which converts between a byte array (the tag buffer) and the T .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 the PlcMapper 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, and Tag<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 the PlcMapper or Tag<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

  • When working with some of the more complicated types (BOOL Arrays, Strings, UDTs)
  • Using some advanced CIP services such as Tag Listing.
  • Arrays of arrays (a.k.a. Jagged arrays)
  • Dynamic Tags
  • When using multiple make/models of device (Allen-Bradley encode some tags different to Omron)
  • When using different tag names (e.g. MyBoolArray and MyBoolArray[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:

  • libplctag wiki does not apply to most of it.
  • There are three wrapper classes plus the mapper - objects which must be garbage collected.
  • There are many cases where the returned PLC memory does not have a static size.
  • It is code that the libplctag community must document, support and maintain.

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:

  • Several use-cases would have been immediately enabled (e.g. hooks to add before ad after Initialize is called).
  • Consumers can add their own methods such as custom setters/getters that trigger events.

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 be object - 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:

  • Removing ITag, Tag<M,T>, all Mappers and Simple Tag classes, and also not supplying an alternative like an abstract base type.
  • Instead, moving Tag<M,T> and a demonstration of an alternative like an abstract base mapper type to the Examples folder.
  • Move Tag Listing for Rockwell PLC to the Examples folder.
  • Creating a separate repository within the libplctag organization for known tag buffer encodings for combinations of PlcType, Tag Name, Protocol, etc
  • Clarify the promise that libplctag makes to customers - what should libplctag be capable of, and what is left up to the consumer to discover?

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.

@kyle-github
Copy link
Member

kyle-github commented Jul 17, 2024 via email

@trinnan42
Copy link

trinnan42 commented Jul 17, 2024

I have some concerns with this. (Note: Please correct me if I misunderstood anything in your explanation.)

UDTs

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.

Wrapper

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.

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 18, 2024

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:

  1. The tag value is exposed as a C# type rather than a collection of Data Accessors.
  2. The mapper logic is a store of knowledge for how the tag bytes are structured, and which Data Accessors to use, for various scenarios.

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!

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 18, 2024

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.

@trinnan42
Copy link

@timyhac

Well, to try to explain with more detail what I'm doing without being able to share actual code:

  1. I have an Enum which defines the available Data Types.
  2. I have some Dictionary instances which map these data types to C# native data types and the Mappers.
  3. 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.
  4. I have Read<T> and Write<T> methods exposed to my code defined with my IPlcComm interface.
  5. The Read method instantiates this dynamic instance of 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.

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 19, 2024

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:

  • It does not allow you configure string attributes. Makes working with STRING types that don't have correct defaults impossible. You must use Tag instead. Many issues have been raised against this.
  • It does not allow you to set up raw tags due to the need to Initialize the tag, then write to the buffer.
  • Things like NotifyChanged are not supported directly (and can't be, due to the need to choose an appropriate hashing algorithm which the library can't do).
  • 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}");

@trinnan42
Copy link

@timyhac Your explanations are much appreciated! I've got a better understanding now. This certainly alleviates my concerns.

@MountainKing91
Copy link

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.

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 21, 2024

Thanks for your input @MountainKing91!

@MountainKing91
Copy link

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?

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 23, 2024

@MountainKing91 - just github/google.

timyhac added a commit that referenced this issue Jul 26, 2024
Although issue #406 says to **remove** these classes, start the process by deprecating them instead.
@timyhac
Copy link
Collaborator Author

timyhac commented Jul 26, 2024

@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:

  1. Deprecate the mapper system, and rewrite examples and docs to use Tag instead of Tag<M,T>.
  2. Remove the mapper system from the nuget library.

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.

@kyle-github
Copy link
Member

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.

@timyhac
Copy link
Collaborator Author

timyhac commented Jul 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants