cmseaton42 / node-ethernet-ip

A Lightweight Ethernet/IP API written to interface with Rockwell ControlLogix/CompactLogix Controllers.
MIT License
265 stars 106 forks source link

WIP testing-mapping #25

Open jhenson29 opened 6 years ago

jhenson29 commented 6 years ago

Template class update.

Addition of template class for udts. Moved serialization and deserialization for all types into templates.

Description, Motivation, and Context

How Has This Been Tested?

Tested with atomic types, udts, nested udts, strings, bit indexes, bool arrays. All tests done with tag read/write, tag group read/write and subscribe.

Screenshots (if appropriate):

Types of changes

Checklist:

Related Issue

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 101


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/template/index.js 66 67 98.51%
src/controller/index.js 6 8 75.0%
<!-- Total: 180 183 98.36% -->
Files with Coverage Reduction New Missed Lines %
src/controller/index.js 4 12.16%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 100: 19.06%
Covered Lines: 681
Relevant Lines: 970

💛 - Coveralls
pcnate commented 5 years ago

I can confirm this code base works for my application. You cannot use the Types.STRING as you get a type mismatch.

jhenson29 commented 5 years ago

I can confirm this code base works for my application. You cannot use the Types.STRING as you get a type mismatch.

Yes, I believe the built-in string type as well as user-defined string types are treated as UDTs.

Penlane commented 5 years ago

I ran into a bug ( I think ... ). It's kinda difficult to reproduce/explain, but I will try to do my best. When aligning a BOOL after an INT-Array, in a UDT, the *generator function does not correctly set the alignment-offsets. First, the INT-Array is aligned in 16-bit values, but there's a hardcoded evaluation if (length > 0 && alignment < 32) alignment = 32;

that sets the alignment to 32 regardless of the datatype. Is this because of the padding bytes?

After that line offset = Math.ceil(offset / alignment) * alignment; calculates the offset for the next member, but it's 2 bytes short. I'll try to explain what should be happening with a wireshark cap: pic

The red parts are the "padding" / "alignment" bytes between the members. The green parts are the 16-bit aligned Values of the INT[3]-Array.

The offset before the INT[3]-Array is 2848Bit = 356Byte, which is fine. In order to read the Bool correctly, the generator should now increase the offset 3 * 16-Bit (for 3 * 16-Bit Array values) and then another 16-Bit for the padding byte. However, it only increases the 48-Bit by the Array. In the next loop (now for the BOOL-Tag), the offset Math.ceil function divides by 8, which gives a "round" value so there's no increase in offset. Which means the whole read UDT is misaligned by 2 Bytes now.

I tried to explain as much as I can, but I realize it's a very niche case. Let me know if I can provide more information.

jhenson29 commented 5 years ago

AFIAK all non-atomic types (including arrays of atomic types) resolve to 32-bit alignment (except when including LINT in certain versions, in which case it's 64 bit alignment...not currently accounted for...). Can you send me the full udt l5x and the logix designer version to review?

Penlane commented 5 years ago

Alright, I made a stripped-down version which reproduces the error for me (String, Int, IntArr read fine, but the bools are misaligned as far as I can see) jhensonUDT.zip

This is how I filled the testdata (String all 'A's) jUDT_filled

This is what I see in the debugger (notice the BOOL values) jUDT_debug

jhenson29 commented 5 years ago

Perfect. Thanks. I'll dig into it this weekend. Just looking at how the UDT builds in logix, it looks like it offsets the way I expect it to it should be pretty quick to debug the source of the offset error in the parser.

jschenkDD commented 4 years ago

Any work on this?

jhenson29 commented 4 years ago

I haven’t really been keeping up with this. I have a local copy with these updates pulled in that I use, but I’m moving away from EtherNet/IP and towards EtherCAT and ADS, so I haven’t spent any time on development for this.