Added msp response size check before adding new requests to msp with … #489
Conversation
…allow_packing=1 and fragmented reads disabled
|
I wasn't sure whether to add the supports_fragmented_read check, as this is currently always false, but it will make it easier for you when you do add fragmented read support. Also not sure why the CI tests are failing. Let me know if you want any changes. |
|
The failure of the CI almost looks like a GitHub problem. |
|
I'm going to merge and see if that helps... |
|
Looks like all the tests passed. Please test away on your Omron hardware! |
| * we later subtract 2 bytes for each packet, this is to allow for the INT offset to the packet within the msp | ||
| * finally we subtract 10 bytes just to give a comfort blanket as I had seen PLC_TAG_TOO_LARGE issue without it due to too much | ||
| * data being packed into a single packet */ | ||
| remaining_response_space = conn->max_payload_size - 2 - 4 -10; |
There was a problem hiding this comment.
We could just set up a defined constant called "OMRON_OVERHEAD_FUDGE" or something. It would at least let us know what the numbers were for.
| @@ -142,6 +142,10 @@ struct omron_request_t { | |||
| /* used by the background thread for incrementally getting data */ | |||
| int request_size; /* total bytes, not just data */ | |||
| int request_capacity; | |||
| int response_size; /* size of data we expect to be returned by this request */ | |||
There was a problem hiding this comment.
IIRC, the request_size includes the EIP and CPF header in the overhead. This should probably be changed so that these requests do not include that data. It is extremely redundant.
|
This all looks pretty sane. Let's get this release soon into the release branch. Thanks for being patient on this and thanks again for the PR! |
|
Thanks Kyle, I've done a few more edge case tests, with packing the exact maximum amount of data based on the response packet limiter and trying to pack 1 byte over the limit and both cases work properly. Ill let you know if I encounter any more bugs, the code I've been working on will be going into production soon, so I'm sure they will be spotted if they exist. |
* Make file number optional if input or output file. Fix #481. * Added fix for omitted data file number on I or O files. Added support for a numeric subelement. Fix #479. (#483) * Fix #486. (#487). String size change problem. Fixed with #488. * Added atomic flag and checks to aid in making all sessions terminate when the library is shutting down. Fix #450. * plc_tag_set_string() feature additions (#488). Setting string can change tag data buffer size. And added flag, allow_field_resize, to suppress change. * Added msp response size check before adding new requests to msp with allow_packing=1 and fragmented reads disabled (#489) * Support for Omron to use multi-request packets. --------- Co-authored-by: Phil Smith <105802449+ptsOSL@users.noreply.github.com>
…allow_packing=1 and fragmented reads disabled
This is a fix for #454. This checks for conditions where adding additional packets to a multi-service-packet (msp) message would cause the response packet to exceed the maximum size of an ethernet/IP message.
If a tag is being read for the first time, we do not add it to a msp as we dont know its size and cant guarantee that it will fit into the response packet. So we just send it as a normal, single request.
If a tag is not being read for the first time, we know the size of its response data. We use this size to check if there is room in the response before adding the request to the msp. If there is not room then we send the previous msp and start a new one.
There is an exception to the above, if fragmented reads are supported, it does not matter if there is not room in a single response as they will be packed into multiple requests. This is not yet supported for Omron, but will be in the future.
I have tested this and it stops the PLC_TAG_TOO_LARGE issue. The largest amount of data which can now theoretically be read in an msp, is two variables which are 978 bytes large giving a total of 1956 bytes of useful data. The expected extra bytes are (2 bytes for the msp count of packets, 4 bytes for the cip response header, 2 bytes per packet for the offset to the data within the msp and a variable number of bytes for the padding between the datatypes. We assume this padding is always 8 bytes just to be safe. We also add a 10 byte comfort blanket. Giving the total header/padding/extra data = 2+4+10+n*(8+2+dataSize). Where n is the number of requests in the msp. The optimal situation is n=2 and dataSize=978 which gives 2 bytes of spare room and 1956 bytes of useful data in the msp, which is 98% efficient. If dataSize=980, then we cannot fit two requests into the msp and I have tested this.