Skip to content

Implementing TCP timeout #3

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

Merged
merged 6 commits into from
Mar 1, 2018

Conversation

KyleARectorAStuff
Copy link
Contributor

@KyleARectorAStuff KyleARectorAStuff commented Feb 28, 2018

This PR contains the implementation of a timeout for the TCP interface read functions. The default timeout value is set to 5 milliseconds, but may be passed in as an optional argument to the TCPInterface::read and TCPInterface::read_exactly methods.

This PR is to resolve #2

…execution

Before this commit, the tcp_interface read method would constantly return a timeout error, even
if data had been read properly. After this commit, the read method returns an OK status if the read
was successful, or TIMEOUT or READ_FAILED depending on the failure type. In the Boost asio library,
the io_service can be run continuously, or run once until an event hander has been dispatched. The
return value of the run_one method was previously used as a while loop exit condition, but this
resulted in the initial behavior describe above, as if the run_one method actually returned after
several even handlers were dispatched, instead of just one. After removing the while loop and using
the method alone, the desired behavior was achieved.
Before this commit, the timeout for the TCPInterface::read() method had a hard-coded timeout
value of 5 ms. After this commit, the TCPInterface::read() function takes an optional
parameter for the timeout, in milliseconds. This parameter defaults to 5 ms.
Before this commit, the read_exactly method used a blocking read call. After this commit, the
read_exactly function has a configurable timeout in milliseconds, with a default of 5 ms.
Copy link
Contributor

@joekale joekale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything looks good to me

Copy link
Contributor

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a fairly integral part of several of our drivers and this is a fairly significant change, has it been tested on more than one driver?

…dler

Before this commit, the result of a read or timeout was stored in a private variable,
populated by the respective callback. After this commit, the conditional statements that
proviously relied on the flags instead rely on the gloabl error message's value.
Additionally, the timeout handler has added error checking to prevent it from executing fully
when the timer.cancel() method is called after a successful read.
Before this commit, a timeout was used in every call TCPInterface::read or
TCPInterface::read_exactly, with a default of 0 ms. After this commit,
the default is set to 0 ms, and if the read or read_exactly methods receive
a 0 timeout request, it will not set a deadline for the timeout, resulting
in a blocking read. This allows for the TCPInterface to behave with a
timeout, or else be used as it was previously.
@joekale
Copy link
Contributor

joekale commented Mar 1, 2018

Adding the default timeout to be 0 and a 0 timeout meaning no timeout being used provides support for older drivers not equipped to deal with timeouts. With that addition the ESR Eth driver (on branch with parse corrections) functions as expected with no changes required.

Copy link
Contributor

@samrustan samrustan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you do have to use run_once() in a while. --inside joke with Kyle, but you see what I did there...right?

Error handling is good.

Timeout handling is good.

@samrustan samrustan merged commit b334a4e into astuff:master Mar 1, 2018
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.

Need To Implement Timeouts
5 participants