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

DRAFT: Native service client #147

Merged
merged 13 commits into from
Jun 29, 2024
Merged

DRAFT: Native service client #147

merged 13 commits into from
Jun 29, 2024

Conversation

ssnover
Copy link
Collaborator

@ssnover ssnover commented Dec 19, 2023

Description

This introduces ROS1 native service clients. I still need to test it and write an example, and there are a lot of tangential changes which I expect I may break out into separate pull requests.

Checklist

  • Update CHANGELOG.md
  • Integration test with roscpp and rospy implementations
  • Write example for native service client

@ssnover ssnover self-assigned this Dec 19, 2023
Copy link
Collaborator Author

@ssnover ssnover left a comment

Choose a reason for hiding this comment

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

Some additional context.

roslibrust/src/lib.rs Outdated Show resolved Hide resolved

/// For now starting with a central error type, may break this up more in future
#[derive(thiserror::Error, Debug)]
pub enum RosLibRustError {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved these here out of rosbridge module as I start to use concrete domain-specific error types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this conflicts with the other error handling that recently got merged, stuff is getting really messy around errors now and will need some close eyes.

Agreed that we should unify to a single global error type for all clients in an ideal world.

roslibrust/src/ros1/names.rs Outdated Show resolved Hide resolved
Comment on lines +186 to +187
srv_definition: String::from_iter(
[T::Request::DEFINITION, "\n", T::Response::DEFINITION].into_iter(),
),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The integration testing with roscpp and rospy will reveal whether this method of service definition generation is fine. I believe it should be.

roslibrust/src/ros1/node/actor.rs Show resolved Hide resolved
roslibrust/src/ros1/service_client.rs Show resolved Hide resolved
roslibrust/src/ros1/service_client.rs Outdated Show resolved Hide resolved
@@ -118,3 +117,44 @@ impl ConnectionHeader {
Ok(header_data)
}
}

pub async fn establish_connection(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function breaks out the functionality being used elsewhere.

roslibrust/src/ros1/tcpros.rs Show resolved Hide resolved
@ssnover ssnover force-pushed the native-service-client branch from f184540 to 2764c13 Compare December 20, 2023 02:40
@ssnover ssnover force-pushed the native-service-client branch from 2764c13 to 5a29d03 Compare December 20, 2023 02:46
Carter12s pushed a commit that referenced this pull request Feb 8, 2024
@Carter12s
Copy link
Collaborator

@ssnover Do we want to get this one merged?

@Carter12s
Copy link
Collaborator

@ssnover I finally got a chance to take a deeper look at this one today and got it to a basic working state!

image

There were a few small things that needed to be tweaked, but it did work out.

@Carter12s Carter12s changed the base branch from reorg-native-node-module to master May 21, 2024 21:16
@Carter12s
Copy link
Collaborator

I think I'm tempted to merge this as is (or maybe a few more fixes), there is so much I want to overhaul in roslibrust now that I'm looking at it again with fresh eyes...

@ssnover
Copy link
Collaborator Author

ssnover commented May 21, 2024

If it's functional I think it's worth merging and then iterating. I don't currently have time to work on this to a significant degree, but I can review PRs.

@ssnover
Copy link
Collaborator Author

ssnover commented Jun 4, 2024

The unwrap in ROS name creation seems not ideal.

@Carter12s
Copy link
Collaborator

Okay I have successfully working service server that is compatible with our service_client!

This is far from "done", but gets us to a point where we can start writing some round-trip tests of service_client to service_server and actually starting making things robust.

Many major TODOs left, but I can finally add two int together.

@Carter12s
Copy link
Collaborator

AIGHT I'm merging this.

Huge amount of work still to go, but we do have working service servers and service clients, and can start to test them with themselves!

@Carter12s Carter12s merged commit 4f0d0d5 into master Jun 29, 2024
5 checks passed
@Carter12s Carter12s deleted the native-service-client branch June 29, 2024 01:46
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.

2 participants