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

WIP testing-mapping #25

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jhenson29
Copy link
Collaborator

@jhenson29 jhenson29 commented Jul 11, 2018

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This is a work in progress, and I want some feedback (If yes, please mark it in the title -> e.g. [WIP] Some awesome PR title)

Related Issue

Suggested changes to move data serialization and deserialization into Templates.
updates for defining, mapping, serializing, and deserializing arrays w/ examples
finished template class
updates for controller, tag, and tag group class to support templates
fixed other misc bug found
added additional tests
@coveralls
Copy link

coveralls commented Jul 26, 2018

Pull Request Test Coverage Report for Build 101

  • 180 of 183 (98.36%) changed or added relevant lines in 6 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+19.06%) to 70.079%

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

💛 - Coveralls

update to run string through mapping generator
@pcnate
Copy link
Contributor

pcnate commented Mar 14, 2019

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

@jhenson29
Copy link
Collaborator Author

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.

pcnate and others added 2 commits March 18, 2019 18:21
changed semi-colons to commas in examples
Fix error in UDT examples in README
@Penlane
Copy link

Penlane commented Apr 16, 2019

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
Copy link
Collaborator Author

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
Copy link

Penlane commented Apr 26, 2019

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
Copy link
Collaborator Author

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.

@jhenson29 jhenson29 mentioned this pull request Feb 8, 2020
@jschenkDD
Copy link

Any work on this?

@jhenson29
Copy link
Collaborator Author

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.

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

Successfully merging this pull request may close these issues.

5 participants