Skip to content

Add abstractions and minimal implementation of rcl events #4

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.

Sign up for GitHub

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

Jump to bottom
Merged
ivanpauno merged 9 commits into from
Aug 12, 2020
Merged

Add abstractions and minimal implementation of rcl events #4

ivanpauno merged 9 commits into from
Aug 12, 2020

Conversation

@ivanpauno
Copy link
Collaborator

@ivanpauno ivanpauno commented Aug 7, 2020

This is going to be a big PR ....

I don't want to send something massive, this at least compiles though it's not yet usable.
@jacobperron I would like some early feedback, does the proposed abstractions look good?

ivanpauno
ivanpauno commented Aug 7, 2020
View reviewed changes
@ivanpauno
Copy link
Collaborator Author

ivanpauno commented Aug 7, 2020

@jacobperron I would like to have a feature branch in this repo (e.g. feature/event_handlers_support).

I prefer merging this yet unusable (but compiling) code there, than update this PR repeatedly. Reviewing massive PRs is painful.
When that branch reaches an usable state, we should merge it into master (and delete it).

@jacobperron
Copy link

jacobperron commented Aug 7, 2020

@ivanpauno I've given you maintainer access. You should be able to create branches now.

@ivanpauno ivanpauno self-assigned this Aug 7, 2020
@jacobperron
Copy link

jacobperron commented Aug 7, 2020

@ivanpauno Also, you should base your changes on the foxy-devel branch. Sorry the confusion with all the branches.

@ivanpauno
Add support for rcl events, first attempt
c914f4f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Collaborator Author

ivanpauno commented Aug 10, 2020

@ivanpauno Also, you should base your changes on the foxy-devel branch. Sorry the confusion with all the branches.

Ups, fixed.

@ivanpauno ivanpauno changed the base branch from jacob/foxy to foxy-devel August 10, 2020 16:38
jacobperron
jacobperron reviewed Aug 11, 2020
View reviewed changes
@ivanpauno @jacobperron
nit
55a5f8c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
jacobperron
jacobperron reviewed Aug 11, 2020
View reviewed changes
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! Some questions and minor comments below.

rcljava/src/main/java/org/ros2/rcljava/events/EventStatus.java
* @param handle A pointer to the underlying event status.
*/
void fromRCLEvent(long handle);
}
Copy link

@jacobperron jacobperron Aug 11, 2020

Choose a reason for hiding this comment

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

Does it make sense to also have a "get event type" method as part of this interface?

Copy link
Collaborator Author

@ivanpauno ivanpauno Aug 11, 2020

Choose a reason for hiding this comment

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

That's part of PublisherEventStatus and SubscriptionEventStatus.

ivanpauno and others added 7 commits August 11, 2020 14:55
@ivanpauno @jacobperron
nit
83296b9 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
@ivanpauno @jacobperron
nit
3fa5de8 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
@ivanpauno @jacobperron
nit
81ca9b1 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
@ivanpauno @jacobperron
nit
41c5fb1 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
@ivanpauno
Actually execute the registered callback
2348044 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Make everything compile again
b963362 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Rename Event to EventHandler
51c640f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Collaborator Author

ivanpauno commented Aug 11, 2020 •
edited
Loading

@jacobperron if you think this approach is fine, I will merge this in a feature branch and continue working on a few follow ups:

  • Add methods in publisher/subscription to actually create an event handlers.
  • Make the executor collect event handlers and execute them.
  • Create one event status, and start writing up tests.
  • Add support to the other event statuses.

@ivanpauno ivanpauno marked this pull request as ready for review August 12, 2020 13:09
@ivanpauno ivanpauno changed the base branch from foxy-devel to feature/events August 12, 2020 13:09
@ivanpauno ivanpauno changed the title Add support for rcl events Add abstractions and minimal implementation of rcl events Aug 12, 2020
@ivanpauno ivanpauno merged commit dbbb162 into osrf:feature/events Aug 12, 2020
@ivanpauno
Copy link
Collaborator Author

ivanpauno commented Aug 12, 2020

@jacobperron I have merged this into the feature branch, so I can continue with one of the other tasks listed here #4 (comment).

Let me know if there are any further comments here.

@ivanpauno ivanpauno deleted the ivanpauno/foxy branch August 12, 2020 13:12
ivanpauno added a commit that referenced this pull request Aug 20, 2020
@ivanpauno
Add abstractions and minimal implementation of rcl events (#4)
c8ea478 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno mentioned this pull request Aug 28, 2020
Merged
ivanpauno added a commit that referenced this pull request Aug 31, 2020
@ivanpauno
Add abstractions and minimal implementation of rcl events (#4)
c95ec23 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit that referenced this pull request May 17, 2021
@ivanpauno
Add abstractions and minimal implementation of rcl events (#4)
d09dd27 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 3, 2021
@ivanpauno
Add abstractions and minimal implementation of rcl events (osrf#4)
2f31e37 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno mentioned this pull request Dec 3, 2021
Merged
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 6, 2021
@ivanpauno
Add abstractions and minimal implementation of rcl events (osrf#4)
c3cf76f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 6, 2021
@ivanpauno
Add abstractions and minimal implementation of rcl events (osrf#4)
cc1bacf 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jacobperron jacobperron jacobperron left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanpauno ivanpauno

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ivanpauno @jacobperron