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 overall spin timeout to rosserial read. #334

Merged
merged 1 commit into from Nov 13, 2017

Conversation

@mikeodr
Copy link
Contributor

mikeodr commented Nov 9, 2017

Reading may block for to long, while there is a check for a timeout, it is not at the global scope of the while(true) statement.

{
// Reset back to first mode.
mode_ = MODE_FIRST_FF;
return -2;

This comment has been minimized.

Copy link
@mikepurvis

mikepurvis Nov 9, 2017

Member

I supposed with a timeout of 0 (the default), the behaviour is unchanged, but it would be good to use a constant here instead, and also clarify that the expectation for users of setSpinTimeout is that they check for this return code on spinOnce, and if they see it, flush the buffer.

This comment has been minimized.

Copy link
@mikeodr

mikeodr Nov 9, 2017

Author Contributor

-2 was used for the other timeout below, I could make it something else also.

This comment has been minimized.

Copy link
@mikepurvis

mikepurvis Nov 10, 2017

Member

I guess this actually works for a user even if they don't explicitly flush the queue— all it does is ensure that spinOnce only takes up to a certain amount of time, so if there's a burst of messages which arrive all at once, those will get processed over a series of spin invocations, so long as there's the buffer space to accommodate them.

In any case, yes, please define SPIN_OK and SPIN_TIMEOUT consts and return them from this function in the appropriate spots.

@mikeodr mikeodr force-pushed the mikeodr:spin-once-timeout branch 3 times, most recently from 8905561 to bd9cf18 Nov 10, 2017
if ((hardware_.time() - c_time) > spin_timeout_)
{
// Reset back to first mode.
mode_ = MODE_FIRST_FF;

This comment has been minimized.

Copy link
@mikeodr

mikeodr Nov 10, 2017

Author Contributor

@mikepurvis, if we intend to allow for just a continuation of processing, setting this would be detrimental. This kind of forces a buffer flush.

This comment has been minimized.

Copy link
@mikepurvis

mikepurvis Nov 10, 2017

Member

Might be better to avoid doing so then. All the deserializing state is maintained between spins— if there's outside logic that drops portions of the buffer, the next spin will realise this and resynchronize anyway.

@mikeodr mikeodr force-pushed the mikeodr:spin-once-timeout branch from bd9cf18 to 45666a6 Nov 10, 2017
Reading may block for to long, while there is a check
for a timeout, it is not at the global scope of the while(true)
statement.
@mikeodr mikeodr force-pushed the mikeodr:spin-once-timeout branch from 45666a6 to 887fa24 Nov 10, 2017
@@ -64,6 +64,10 @@ class NodeHandleBase_
namespace ros
{

const int SPIN_OK = 0;
const int SPIN_ERR = -1;
const int SPIN_TIMEOUT = -2;

This comment has been minimized.

Copy link
@mikepurvis

mikepurvis Nov 13, 2017

Member

There's a case to be made that these should be static const int members of NodeHandle, but it's probably fine for them just to be here.

@mikepurvis

This comment has been minimized.

Copy link
Member

mikepurvis commented Nov 13, 2017

Looks like a great change, thanks for the contribution!

@mikepurvis mikepurvis merged commit 99da697 into ros-drivers:jade-devel Nov 13, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mikeodr mikeodr deleted the mikeodr:spin-once-timeout branch Nov 13, 2017
romainreignier pushed a commit to 1r0b1n0/rosserial that referenced this pull request Nov 7, 2018
…am to jade-devel

* commit 'a707373c7e9f9ad40ad9440550840e29960c9290':
  Changed hardcoded pin 13 to LED_BUILTIN (ros-drivers#328)
  Re-attempting rosserial for VEX Cortex (ros-drivers#380)
  Added service to force an Arduino hard reset in serial_node.py (ros-drivers#349)
  Fix compiling on boost > 1.66 (ros-drivers#362)
  Use the ! prefix introduced in gcc4mbed for mbed examples (ros-drivers#304)
  Add support for boolean parameters (ros-drivers#355)
  Change Travis to use clang-5.0.0 (ros-drivers#363)
  [python] fix an unboundlocalerror (ros-drivers#346)
  Added ESP32 support (ros-drivers#345)
  Retry opening the serial port every 3 seconds (ros-drivers#342)
  In rosserial_arduino, changed embedded type size for ROS uint64 to 8 bytes from 4. (ros-drivers#312)
  0.7.7
  Changes
  Copy src/examples to install dir so make_libraries.py doesn't fail (ros-drivers#336)
  Add overall spin timeout to rosserial read. (ros-drivers#334)
  Fixing formatting on files. (ros-drivers#333)
  Fix catkin lint errors (ros-drivers#296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.