search

danomagnum / gologix

Ethernet/IP client library for Go inspired by pylogix that aims to be easy to use. Supports being a client and a class 3 / class 1 server.
MIT License
32 stars 8 forks source link

PathLength should be recalculated when serializing the class attribute. #16

Closed ConstRico closed 5 months ago

ConstRico commented 5 months ago

Neither GetAttrSingle nor GenericCIPMessage takes into account the length of the class. If CIPService=0x14, CIPClass=0x04, CIPInstance=0x303 and CIPAttribute = 0x03, the PathLength is always 3, but it should be 4 actually.

ConstRico commented 5 months ago

p, err := Serialize( class, instance, msg_data, ) in GenericCIPMessage method is right.

danomagnum commented 5 months ago

GetAttrSingle was a straightforward fix.

I had to rework the GenericCIPMessage function signature. It is now

func (client *Client) GenericCIPMessage(service CIPService, path, msg_data []byte) (*CIPItem, error)

Which will allow messages with and without an attribute on the path. This means you have to do one more function call though (or hard code the bytes of the path).

path, err := gologix.Serialize(gologix.CIPClass(0x04), gologix.CIPInstance(0x303), gologix.CIPAttribute(0x03))
...
r, err := client.GenericCIPMessage(0x14, path.Bytes(), []byte{ service data goes here})

What is service 0x14? I don't have a definition for it.

ConstRico commented 5 months ago

I agree with your perspective. However, the fundamental issue lies in the fact that PathLength does not consider the length of CIPAttribute. This problem is present in all methods.

Hah, 0x14 is actually GetAttrSingle. But I want to PathLength is dynamic, so I decide to use GenericCIPMessage.

Perhaps I can create a new branch to fix this issue and then submit a Merge Request to you. What do you think?

danomagnum commented 5 months ago

Oh ok. GetAttributeSingle is 14 in decimal (0x0E) (unless there are two getattributesindle services - which wouldn't surprise me).

Take a look at the latest commit (fdf9aca). I think it addresses the issues - I got rid of the hard-coded path length and calculate them appropriately (hopefully). I also fund a bug in the length calculation for the CIPAttribute type also so that was a good fix.

ConstRico commented 5 months ago

Looks good! Will you be releasing a new version soon? I referenced your code as go library, I cannot verify it unless you release a new version now.

danomagnum commented 5 months ago

I will do the new release now. Usually takes a little bit for the google module cache to update but then you'll be able to get the new version.

danomagnum commented 5 months ago

https://github.com/danomagnum/gologix/releases/tag/v0.21.0-beta

ConstRico commented 5 months ago

Thank you for your prompt response! All features are now in effect, if there are other needs, I believe we can address them in our local code.

BTW, the current methods are not as user-friendly as they were before, especially for someone new to CIP. However, this is not a significant issue.