Skip to content

(RclJava) fix BaseExecutor.spinSome logic - run the executable even i…#134

Merged
esteve merged 1 commit into
ros2-java:dashingfrom
sung-goo-kim:dev/fix-spin-some
Sep 30, 2020
Merged

(RclJava) fix BaseExecutor.spinSome logic - run the executable even i…#134
esteve merged 1 commit into
ros2-java:dashingfrom
sung-goo-kim:dev/fix-spin-some

Conversation

@sung-goo-kim

Copy link
Copy Markdown
Contributor

Currently, executable did not run if getNextExecutable() is not null.

@esteve esteve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but these two look equivalent. Could you show an example where the execution differs? Thanks.

@DongheeYe DongheeYe left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@esteve I think the logics are a little bit different. Original logic does not execute anything even if there is an anyExecutable getting by getNextExecutable(). But, in changed logic, it will be executed.

@jacobperron jacobperron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jacobperron

Copy link
Copy Markdown
Contributor

@esteve The problem is on L332. If the executable is not null, then we're skipping it.

@esteve

esteve commented Sep 30, 2020

Copy link
Copy Markdown
Member

@sung-goo-kim @jacobperron thanks, I didn't catch that

@esteve esteve merged commit 7fb3dfe into ros2-java:dashing Sep 30, 2020
@sung-goo-kim sung-goo-kim deleted the dev/fix-spin-some branch October 5, 2020 02:50
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.

4 participants