libplctag / libplctag

This C library provides a portable and simple API for accessing Allen-Bradley and Modbus PLC data over Ethernet.
Mozilla Public License 2.0
700 stars 230 forks source link

Message packing on OmronNJ/NX beyond max response size #454

Open ptsOSL opened 5 months ago

ptsOSL commented 5 months ago

I have been encountering a problem with packing of CIP requests when reading data from OmronNJ. libplctag is packing many read requests into a single request message, as I have requested. However, if the size of the response packet (the size of all of the data which is being read) is beyond the max packet size of 1996 bytes, then I receive error 0x1B, routing failure (response packet too large). This is mostly a problem when packing multiple large structures together. I am guessing this is only a problem on Omron as it does not support fragmented reads. Looking through the source code, I didn't see anywhere that libplctag checks to see if the combined size of the response message would fit in a single response packet, but i might have missed it? Would it make sense to individually read each item that is to be packed when the tags are initialized to see if they can be packed together? This is something the user can do, however it seems more like something libplctag should do, but I might be misunderstanding something. Cheers

kyle-github commented 5 months ago

I don't have an Omron, so take what I say with a grain of salt. From what I understand, Omron decided to behave differently than Rockwell in some key areas:

One of the results of all this is that you can request several tags at once, but if the results do not fit into the 1900 byte result packet, the whole bundled request seems to fail. I think this is what you are hitting.

So while Omron is using CIP it is not using the Rockwell commands. Unfortunately, I have not found a good source of documentation for Omron on this. There is a Python library that supports Omron with CIP that could be a source of ideas.

ptsOSL commented 5 months ago

So I have been developing with libplctag for OmronNJ for a few months now and some of the behaviour you describe differs to what I have seen on my Omron NJ:

Yes this is my problem, I suppose this issue is about whether it would be a good idea to add checks to libplctag to stop users breaching this limit? Before packing requests together, the library could add up the size of each reads response data and only pack requests if their response data will fit in a single packet. Of course the library would have to do an initial read for each request to find out the sizes, but in the long run it would improve compatibility with PLCs which dont support fragmented reads and give better performance.

This is the manual I have been using

ptsOSL commented 1 month ago

Hi Kyle, is there anything I can do to help you get this issue fixed? I currently have a forked branch which resolves this issue. It calculates how big a response is going to be before packing each new request into a multiple service packet. Its not a perfect fix, I thought I had worked out a way to figure out precisely how big the response will be, but something is a bit off. I added a 10 byte "comfort blanket" when calculating the response size and have stopped seeing the PLC_TAG_TOO_LARGE issue. I am now seeing up to 1980 bytes of packed data, so its still pretty efficient, far more than having no packing anyway.

This fork is prior to you refactoring the omron stuff out, I can reapply the changes in my fork to this now refactored code? Or you did mention in the previously closed pull request #461 that you wanted to add "Omron-style fragmented requests", in which case it is probably not worth implementing my changes.

In the meantime, I am happy to continue using my fork, I was just interested to know if you still want to implement this Omron-style fragmented requests? Cheers

kyle-github commented 1 month ago

Hi Phil,

Thanks for bringing this back up. Go ahead and apply the PR to the prerelease branch. That has the Omron split (as does release, but prerelease has some other fixes). I was hoping to get to the refactoring of the Omron branch so that adding the Omron fragmented requests would be easier, but I have not finished that up. I would prefer to unblock Omron users as much as possible as soon as possible and your patches will go a long, long way toward that!

Best, Kyle

On Wed, Aug 28, 2024 at 2:52 AM Phil Smith @.***> wrote:

Hi Kyle, is there anything I can do to help you get this issue fixed? I currently have a forked branch https://github.com/ptsOSL/libplctag/tree/packing_fix which resolves this issue. It calculates how big a response is going to be before packing each new request into a multiple service packet. Its not a perfect fix, I thought I had worked out a way to figure out precisely how big the response will be, but something is a bit off. I added a 10 byte "comfort blanket" when calculating the response size and have stopped seeing the PLC_TAG_TOO_LARGE issue. I am now seeing up to 1980 bytes of packed data, so its still pretty efficient, far more than having no packing anyway.

This fork is prior to you refactoring the omron stuff out, I can reapply the changes in my fork to this now refactored code? Or you did mention in the previously closed pull request #461 https://github.com/libplctag/libplctag/pull/461 that you wanted to add "Omron-style fragmented requests", in which case it is probably not worth implementing my changes.

In the meantime, I am happy to continue using my fork, I was just interested to know if you still want to implement this Omron-style fragmented requests? Cheers

— Reply to this email directly, view it on GitHub https://github.com/libplctag/libplctag/issues/454#issuecomment-2314859163, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4LC5WLEMOLYW75SH6DSLZTWMV3AVCNFSM6AAAAABGD3LTEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJUHA2TSMJWGM . You are receiving this because you commented.Message ID: @.***>

ptsOSL commented 1 month ago

This pull request #489 should fix this issue, the tests failed for some reason, would be nice to see them work as this touches a very important piece of code for Omron users.