Tag Datatype Validation about libplctag.net HOT 12 CLOSED

I agree with this, it would be good if @kyle-github could provide some guidance on how we should implement this - should it fit into the existing error code system?

from libplctag.net.

PLCTAG_ERR_BAD_PARAM makes some sense to use here.

The getter functions are a compromise. I did not want to force wrappers to deal with pointers to integers and wanted to keep code fairly simple. The only implementation I could think of that would allow that was to return a single value. But that meant that errors would difficult to report.

What would you do if you were reading a UDT? What if the data type was right, but the offset was incorrect? At least for Allen-Bradley fields are somewhat aligned though I think 64-bit fields may only be 32-bit aligned in some situations.

This gets worse when you think of Modbus. All values are either bits or 16-bit words. On top of that there are larger values mapped, but not by the protocol. There would be no way for the library to determine that the type was wrong.

I think this is a really hard problem as you have no type information from the PLC for Modbus at all.

If I finish up my ideas on getting full tag/UDT definitions then the wrapper could definitely figure out the allowed offsets and types at all locations, but that is a lot of code. And that still does not work for Modbus.

from libplctag.net.

Even though we don't know whether something is correct, we do know when its wrong - so perhaps all we could do here is add some extra information to the (currently blank) exception, saying that we have a BadParam.

https://github.com/libplctag/libplctag.NET/blob/master/src/libplctag/Tag.cs#L406

This for example could go from this

public ulong GetUInt64(int offset)
{
    var result = plctag.plc_tag_get_uint64(tagHandle, offset);
    if (result == ulong.MaxValue)
        throw new LibPlcTagException();
    return result;
}

To this:

public ulong GetUInt64(int offset)
{
    var result = plctag.plc_tag_get_uint64(tagHandle, offset);
    if (result == ulong.MaxValue)
        throw new LibPlcTagException(Status.ErrorBadParam);
    return result;
}

The setters already return an appropriate status code so we'd just need to modify the getters.

from libplctag.net.

I am not sure if you are trying to just show how the throw would work or not, but the logic in those examples is not correct. It is valid to get the INT_MIN/MAX, UINT_MIN/MAX values. That is one of the problems of using them. They are valid values as well as used to note that there was some sort of error. So about all you can do is check to make sure that you are not also incorrectly handling the offset (i.e. reading past the end of the tag buffer).

This surfaces the issue that this is a terrible way to return status :-( I could also set the tag status so that you could call plc_tag_status() if you get the appropriate MIN/MAX value. I worry that will break code that expects the tag status to only change on read/write.

I really need to think about this.

BTW, at least on AB, you can look at the tag size in bytes and the element count (using plc_tag_get_int_attribute() and thus determine the element size. While that is not a type, you can at least figure out if the size of the get/set differs from the size of each element. Again, UDTs really mess this up. I need to double check whether I allow reading of element size directly.

from libplctag.net.

See core library issue #205. The more I think about this, the more I think that I screwed this up initially. There should have been a default value to return on error, not a hard coded one.

from libplctag.net.

Just reading the documentation on this, it looks like we already could be returning the status from the get_status() rather than assuming one.
İ must have missed this when coding this part.

Unsigned getters return the appropriate UINT_MAX value for the type size on error. I.e. plc_tag_get_uint16() returns 65535 (UINT16_MAX) if there is an error. You can check for this value and then call plc_tag_status to determine what went wrong. Signed getters return the appropriate INT_MIN value on error.

İn terms of that code being correct or not, i think it reflects how to check for error - at least how the documentation days you should. İf the value in the tag happens to be the same as UİNT_MAX, then that's too bad - that's just a limitation of this library - right?

from libplctag.net.

Oh! So if get_status() returns Plc_Ok then we can still return the value and not throw an exception. So it should be this:

public ulong GetUInt64(int offset)
{
    var result = plctag.plc_tag_get_uint64(tagHandle, offset);
    if (result == ulong.MaxValue)
    {
        var status = GetStatus();
        if (status != Status.Ok)
            throw new LibPlcTagException(status);
    }
    return result;
}

from libplctag.net.

Except that isn't what the code in the core library does. I have refactored it a bit in the past year or so to deal with different endian configurations to support Modbus. I wonder if I managed to edit that out. I will need to check!

from libplctag.net.

G'day @kyle-github - I think the latest prerelease would allow us to implement the above code right?

from libplctag.net.

yes. I intend to release 2.1.20 this weekend.

from libplctag.net.

Okidoke - I've prepared a draft PR with the required changes here: #100

Once the NativeImport package has the new binaries, we could merge this PR and we'd get the right behaviour.

from libplctag.net.

Hmm, this will be a non backwards compatible change. When we roll this in, it should be a minor version rev. Looks like we're prepping for v1.1.0...

from libplctag.net.