Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions include/ur_client_library/ur/ur_driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,14 @@ class UrDriver

private:
static std::string readScriptFile(const std::string& filename);
/*!
* \brief Reconnects the secondary stream used to send program to the robot.
*
* Only for use in headless mode, as it replaces the use of the URCaps program.
*
* \returns true of on successful reconnection, false otherwise
*/
bool reconnectSecondaryStream();

int rtde_frequency_;
comm::INotifier notifier_;
Expand Down
5 changes: 3 additions & 2 deletions src/comm/tcp_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -296,12 +296,13 @@ void TCPServer::readData(const int fd)
}
else
{
if (0 < nbytesrecv)
if (nbytesrecv < 0)
{
if (errno == ECONNRESET) // if connection gets reset by client, we want to suppress this output
{
URCL_LOG_DEBUG("client from FD %s sent a connection reset package.", fd);
URCL_LOG_DEBUG("client from FD %d sent a connection reset package.", fd);
}
else
{
URCL_LOG_ERROR("recv() on FD %d failed.", fd);
}
Expand Down
7 changes: 7 additions & 0 deletions src/comm/tcp_socket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,14 @@ bool TCPSocket::read(uint8_t* buf, const size_t buf_len, size_t& read)
return false;
}
else if (res < 0)
{
if (errno != EAGAIN && errno != EWOULDBLOCK)
{
// any permanent error should be detected early
state_ = SocketState::Disconnected;
}
return false;
}

read = static_cast<size_t>(res);
return true;
Expand Down
28 changes: 25 additions & 3 deletions src/ur/ur_driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,8 @@ bool UrDriver::sendScript(const std::string& program)
{
if (secondary_stream_ == nullptr)
{
throw std::runtime_error("Sending script to robot requested while there is no primary interface established. "
"This "
"should not happen.");
throw std::runtime_error("Sending script to robot requested while there is no secondary interface established. "
"This should not happen.");
}

// urscripts (snippets) must end with a newline, or otherwise the controller's runtime will
Expand All @@ -562,6 +561,16 @@ bool UrDriver::sendScript(const std::string& program)
return true;
}
URCL_LOG_ERROR("Could not send program to robot");

URCL_LOG_INFO("Reconnecting secondary stream to retry sending program...");
secondary_stream_->close();
if (secondary_stream_->connect() && secondary_stream_->write(data, len, written))
{
URCL_LOG_DEBUG("Sent program to robot:\n%s", program_with_newline.c_str());
return true;
}
URCL_LOG_ERROR("Retry sending program failed!");

Copy link
Member

Choose a reason for hiding this comment

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

We could also use the reconnectSecondaryStream method you created. Also, the code duplication could be reduced (Though I am not sure whether this would be a better approach):

size_t num_tries = 0;
while( num_tries < 2)
{
  if (secondary_stream_->write(data, len, written))
  {
    URCL_LOG_DEBUG("Sent program to robot:\n%s", program_with_newline.c_str());
    return true;
  }
  URCL_LOG_ERROR("Could not send program to robot");
  if (num_tries == 0)
  {
    URCL_LOG_INFO("Reconnecting secondary stream to retry sending program...");
    reconnectSecondaryStream();
  }
}
URCL_LOG_ERROR("Retry sending program failed!");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points about code duplication and reusing reconnectSecondaryStream. Probably subjective but I find that having a loop with 2 iterations actually doing different things is not the best when it comes to clarity. I tried an alternate solution in 7639529. How does that look?

Copy link
Member

Choose a reason for hiding this comment

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

I had the same feeling and also had the thought that using a lambda function would make things prettier. Thank you for your solution, that looks nice!

return false;
}

Expand All @@ -578,6 +587,19 @@ bool UrDriver::sendRobotProgram()
}
}

bool UrDriver::reconnectSecondaryStream()
{
URCL_LOG_DEBUG("Closing secondary stream...");
secondary_stream_->close();
if (secondary_stream_->connect())
{
URCL_LOG_DEBUG("Secondary stream connected");
return true;
}
URCL_LOG_ERROR("Failed to reconnect secodary stream!");
return false;
}

std::vector<std::string> UrDriver::getRTDEOutputRecipe()
{
return rtde_client_->getOutputRecipe();
Expand Down
22 changes: 22 additions & 0 deletions tests/test_tcp_socket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,28 @@ TEST_F(TCPSocketTest, test_deprecated_reconnection_time_interface)
EXPECT_TRUE(client_->setup(2));
}

TEST_F(TCPSocketTest, test_read_on_socket_abruptly_closed)
{
client_->setup();

// Make sure the client has connected to the server, before writing to the client
EXPECT_TRUE(waitForConnectionCallback());

std::string send_message = "test message";
size_t len = send_message.size();
const uint8_t* data = reinterpret_cast<const uint8_t*>(send_message.c_str());
size_t written;
server_->write(client_fd_, data, len, written);

// Simulate socket failure
close(client_->getSocketFD());

char characters;
size_t read_chars = 0;
EXPECT_FALSE(client_->read((uint8_t*)&characters, 1, read_chars));
EXPECT_EQ(client_->getState(), comm::SocketState::Disconnected);
}

int main(int argc, char* argv[])
{
::testing::InitGoogleTest(&argc, argv);
Expand Down
13 changes: 13 additions & 0 deletions tests/test_ur_driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,19 @@ TEST_F(UrDriverTest, target_outside_limits_pose)
waitForProgramNotRunning(1000);
}

TEST_F(UrDriverTest, send_robot_program_retry_on_failure)
{
// Start robot program
g_ur_driver_->sendRobotProgram();
EXPECT_TRUE(waitForProgramRunning(1000));

// Check that sendRobotProgram is robust to the secondary stream being disconnected. This is what happens when
// switching from Remote to Local and back to Remote mode for example.
g_ur_driver_->secondary_stream_->close();

EXPECT_TRUE(g_ur_driver_->sendRobotProgram());
}

// TODO we should add more tests for the UrDriver class.

int main(int argc, char* argv[])
Expand Down
Loading