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

Conversation

KanonWY
Copy link

@KanonWY KanonWY commented Nov 29, 2024

Just add a new interface handleTimeoutMicroseconds

lcm/lcm.c Outdated Show resolved Hide resolved
lcm/lcm.c Show resolved Hide resolved
lcm/lcm-cpp-impl.hpp Outdated Show resolved Hide resolved
KanonWY and others added 3 commits November 29, 2024 21:15
horten the name

Co-authored-by: Tim Perkins <[email protected]>
shorten  function name

Co-authored-by: Tim Perkins <[email protected]>
Copy link
Contributor

@nosracd nosracd left a comment

Choose a reason for hiding this comment

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

Are you interested in adding lcm_handle_timeout_us to the Python module? If so, you can reference the existing bindings for lcm_handle_timeout via git grep -n handle_timeout lcm-python/. There's also some tests for those that you can reference with git grep -n handle_timeout test/python/.

If not, that's fine too; I'll just pop an issue to track that as future work.


// Invalid timeout specification should result in an error.
EXPECT_GT(0, lcm.handleTimeoutUs(-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!

Comment on lines +336 to +337
* New in LCM 1.1.0.
*
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.
*

* 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.

Comment on lines +148 to +149
* New in LCM 1.1.0.
*
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.
*

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.

3 participants