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

Add more precise interface handleTimeoutMicroseconds #546

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
9 changes: 9 additions & 0 deletions lcm/lcm-cpp-impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,15 @@ inline int LCM::handleTimeout(int timeout_millis)
return lcm_handle_timeout(this->lcm, timeout_millis);
}

inline int LCM::handleTimeoutMicroseconds(int timeout_micros)
KanonWY marked this conversation as resolved.
Show resolved Hide resolved
{
if (!this->lcm) {
fprintf(stderr, "LCM instance not initialized. Ignoring call to handle()\n");
return -1;
}
return lcm_handle_timeout_microseconds(this->lcm, timeout_micros);
}

template <class MessageType, class MessageHandlerClass>
Subscription *LCM::subscribe(const std::string &channel,
void (MessageHandlerClass::*handlerMethod)(const ReceiveBuffer *rbuf,
Expand Down
11 changes: 11 additions & 0 deletions lcm/lcm-cpp.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ class LCM {
*/
inline int handleTimeout(int timeout_millis);

/**
* @brief Waits for and dispatches messages, with a timeout.
*
* New in LCM 1.1.0.
*
Comment on lines +148 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

This method does not exist in LCM 1.1.0

Suggested change
* New in LCM 1.1.0.
*

* @return >0 if a message was handled, 0 if the function timed out,
* and <0 if an error occured.
* @sa lcm_handle_timeout_microseconds()
*/
inline int handleTimeoutMicroseconds(int timeout_micros);

/**
* @brief Subscribes a callback method of an object to a channel, with
* automatic message decoding.
Expand Down
26 changes: 26 additions & 0 deletions lcm/lcm.c
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,32 @@ int lcm_handle_timeout(lcm_t *lcm, int timeout_milis)
}
}

int lcm_handle_timeout_microseconds(lcm_t *lcm, int timeout_micros)
KanonWY marked this conversation as resolved.
Show resolved Hide resolved
{
fd_set fds;
FD_ZERO(&fds);
SOCKET lcm_fd = lcm_get_fileno(lcm);
FD_SET(lcm_fd, &fds);

struct timeval timeout;
timeout.tv_sec = timeout_micros / 1000000;
timeout.tv_usec = timeout_micros % 1000000;

if (timeout_micros < 0) {
return -1;
}

int select_result = select(lcm_fd + 1, &fds, NULL, NULL, &timeout);
if (select_result > 0) {
int lcm_handle_result = lcm_handle(lcm);
return lcm_handle_result == 0 ? 1 : lcm_handle_result;
} else if (select_result == 0) {
return 0;
} else {
return select_result;
}
KanonWY marked this conversation as resolved.
Show resolved Hide resolved
}

int lcm_get_fileno(lcm_t *lcm)
{
if (lcm->provider && lcm->vtable->get_fileno)
Expand Down
24 changes: 24 additions & 0 deletions lcm/lcm.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,30 @@ int lcm_handle(lcm_t *lcm);
LCM_EXPORT
int lcm_handle_timeout(lcm_t *lcm, int timeout_millis);

/**
* @brief Wait for and dispatch the next incoming message, up to a time limit.
*
* This function is equivalent to lcm_handle(), but if no messages are received
* and handled by the time @p timeout_micros microseconds elapses, then the
* function returns.
*
* This function largely exists for convenience, and its behavior can be
* replicated by using lcm_fileno() and lcm_handle() in conjunction with
* select() or poll().
*
* New in LCM 1.1.0.
*
Comment on lines +336 to +337
Copy link
Contributor

Choose a reason for hiding this comment

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

This method does not exist in LCM 1.1.0

Suggested change
* New in LCM 1.1.0.
*

* @param lcm the %LCM object
* @param timeout_micros the maximum amount of time to wait for a message, in
* microseconds. If 0, then dispatches any available messages and then
* returns immediately. Values less than 0 are not allowed.
*
* @return >0 if a message was handled, 0 if the function timed out, and <0 if
* an error occured.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Suggested change
* an error occured.
* an error occurred.

It also occurs in the lcm_handle_timeout docstring, in case you feel like fixing it there too.

*/
LCM_EXPORT
int lcm_handle_timeout_microseconds(lcm_t *lcm, int timeout_micros);

/**
* @brief Adjusts the maximum number of received messages that can be queued up
* for a subscription.
Expand Down
9 changes: 9 additions & 0 deletions test/cpp/memq_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ TEST(LCM_CPP, MemqTimeout)
// Invalid timeout specification should result in an error.
EXPECT_GT(0, lcm.handleTimeout(-1));

// No messages available. Call should timeout immediately.
EXPECT_EQ(0, lcm.handleTimeoutMicroseconds(0));

// No messages available. Call should timeout in a few ms.
EXPECT_EQ(0, lcm.handleTimeoutMicroseconds(10));

// Invalid timeout specification should result in an error.
EXPECT_GT(0, lcm.handleTimeoutMicroseconds(-1));

Copy link
Contributor

Choose a reason for hiding this comment

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

Underneath this line, there is the following test for handleTimeout:

    // Subscribe to and publish on a channel.  Expect that the message gets
    // handled with an ample timeout.
    bool msg_handled = false;
    lcm.subscribeFunction("channel", MemqTimeoutHandler, &msg_handled);
    lcm.publish("channel", "", 0);
    EXPECT_LT(0, lcm.handleTimeout(10000));
    EXPECT_TRUE(msg_handled);

Can you duplicate these assertions for handleTimeoutUs as well? It would be good to have as many tests for the new method as for the old.

Copy link
Author

Choose a reason for hiding this comment

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

I will do it, maybe late!

// Subscribe to and publish on a channel. Expect that the message gets
// handled with an ample timeout.
bool msg_handled = false;
Expand Down
Loading