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

Issue #25 fix #33

Merged
merged 7 commits into from
Aug 23, 2013
Merged

Issue #25 fix #33

merged 7 commits into from
Aug 23, 2013

Conversation

shaun-edwards
Copy link
Member

These changes address split packet issues associated with the tcp socket read command (issue #25).

The read command was changed to utilize the socket select function to query the socket buffer state. This allows the read to poll the socket for new data in a "interrupt-able" way. This means that "Control-C" will break out of the program as expected. The previous approach of simply reading or performing a blocking read did not seem to work this way.

Unit tests were performed that demonstrated the problem and verified the fix.

Minor changes were made to allow parallel test execution (default under catkin).

@ghost ghost assigned JeremyZoss Aug 13, 2013
@JeremyZoss
Copy link
Member

I may need you to help talk me through this. A few questions:

  1. There does not seem to be a way for the user to specify an overall timeout when calling receiveBytes(). I'm not clear on what benefit the existing timeout provides. Why not just block on select() forever? Is this timeout what allows you to handle Ctrl-C signals? If so, it should be documented in comments, since it's not clear.
  2. When receiveBytes() called and data not ready (e.g. while waiting for a command), code will log a WARN every time the socket times out. This could potentially generate continuous WARN msgs at 1Hz or so. Should downgrade this to a DEBUG msg?
  3. Why are there three separate "error" returns from poll() that seem to be handled independently? Wouldn't the old isReadyReceive() function be adequate?
  4. You don't seem to have a test case for: sendBytes(ONE_INT) receive bytes(TWO_INTS). This will probably block forever in the current design.

It seems like you've addressed split-packet concerns (repeated READ calls until expected # of bytes received), but there is still no way for high-level users to specify an overall timeout on a read. To higher-level code, it will still appear as if the code is blocking "forever" until the requested data has been read. This is okay, but we should log a new enhancement issue to track this need.

Ideally, I would like to see this fix include the API changes required to implement a true read-timeout, specified by the user during a call to receiveBytes(). This could be propagated through the existing code base using parameters that default to wait-forever, so as to not break existing code. There may be some higher-level concerns, though, as I don't think the simple_message protocol will gracefully handle the case where only part of a message is read. It will probably require the user to drop/re-establish connection to reset the simple_message data stream.

@shaun-edwards
Copy link
Member Author

@JeremyZoss, I had a long response typed out, but then github lost it. So in short, I made some modifications to address your concerns. As far as a read timeout, I think we should submit a new issue to address that.

JeremyZoss added a commit that referenced this pull request Aug 23, 2013
@JeremyZoss JeremyZoss merged commit dd0490e into groovy-devel Aug 23, 2013
@JeremyZoss JeremyZoss deleted the issue25_recv_bytes branch August 23, 2013 20:11
Jmeyer1292 pushed a commit to Jmeyer1292/industrial_core that referenced this pull request Feb 14, 2017
…ipulation_crashes_in_eigen_calls

- Issue ros-industrial#31 demo manipulation crashes in eigen calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants