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

Started working on jump service feature #810

Closed
wants to merge 1 commit into from

Conversation

Kaju-Bubanja
Copy link
Contributor

I would like to implement Expose jump services on the Player from #696. I checked how it was done for the set_rate service #767, but the whole package is quite complex and I'm not sure how rosbag2_cpp interacts with rosbag2_transport. I'm not sure if one can be seen as the backend and the other as the frontend and if I should expose the jump service in rosbag2_transport, like the others or if it should reside in rosbag2_cpp like I did now. I currently can't get it to build, but before investing more time, I would be thankful for some advice, if this is the correct package to add the service.

@Kaju-Bubanja Kaju-Bubanja requested a review from a team as a code owner July 5, 2021 12:47
@Kaju-Bubanja Kaju-Bubanja requested review from emersonknapp and Karsten1987 and removed request for a team July 5, 2021 12:47
@emersonknapp
Copy link
Collaborator

I'm not sure if one can be seen as the backend and the other as the frontend

rosbag2_cpp is the generic rosbag2 C++ API, it doesn't interact with "communications", it only deals with the "bag" portion. rosbag2_transport is the package that has Publishers and Subscribers, and manages actual "transport" of messages.

if I should expose the jump service in rosbag2_transport, like the others

Yes, this service should be with the others, for a cohesive design. It is not good practice to split up related functionality into different places - if we migrate it, it would all be at once.

In this case, though, the service belongs to the Player, which owns a PlayerClock that controls time. So, the service to interact with this clock will also live on the Player in rosbag2_transport.

Additionally, ROS 2 Services are a "transport"/"communications" concept, which we want to keep isolated to the rosbag2_transport package

@MichaelOrlov
Copy link
Contributor

@Kaju-Bubanja @emersonknapp Unfortunately Player::jump(..) API have not yet fully implemented and I am afraid it will be a blocker for relevant service implementation.

@emersonknapp
Copy link
Collaborator

I'll close this for now as stale until the relevant underlying features are ready to expose

@MichaelOrlov MichaelOrlov linked an issue Aug 9, 2021 that may be closed by this pull request
@Kaju-Bubanja Kaju-Bubanja deleted the add_jump_service branch October 1, 2021 08:08
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.

Add jump/seek API for the Player class
3 participants