-
Notifications
You must be signed in to change notification settings - Fork 316
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
Rclpy minimal services #140
Conversation
This PR adds service examples with a similar behavior as the cpp examples added in #138 |
|
||
|
||
def test_copyright(): | ||
rc = main(argv=['.', 'test']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be .
and test
? Isn't test
a subfolder of the package root .
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in 4eddd82 see #139 (comment)
1sec wait before sending match cpp prints
97c0bec
to
247edc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
req.a = 41 | ||
req.b = 1 | ||
# wait for connection to be established | ||
# (no wait for service in Python yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend a TODO here then, so we remember to replace this with wait for service later. Or if there is a ticket to add wait for service in Python, you could link to this line of this file (at a specific commit) as a place to update after it is added. You might want to do both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, ticketed here
req.a = 41 | ||
req.b = 1 | ||
# wait for connection to be established | ||
# (no wait for service in Python yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO here as well
def __init__(self, node): | ||
self.cli = node.create_client(AddTwoInts, 'add_two_ints') | ||
# wait for connection to be established | ||
# (no wait for service in Python yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
|
||
def add_two_ints_callback(request, response): | ||
response.sum = request.a + request.b | ||
print('Incoming request\na: %d b:%d' % (request.a, request.b)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: b: %d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done d6cb134
# when calling wait for future | ||
# spin should not be called in the main loop | ||
cli.wait_for_future() | ||
print('Result of add_two_ints: for %d + %d = %d' % ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: wrap directly after parenthesis, e.g.:
print(
'Result of add_two_ints: for %d + %d = %d' %
(req.a, req.b, cli.response.sum))
Same below.
time.sleep(1) | ||
cli.call(req) | ||
while rclpy.ok(): | ||
if cli.response is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not final API I would suggest to put a big todo above this line (and similar lines below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this TODO message? 5306074
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* add add_two_ints examples * rename targets * add READMEs * comment about node destruction * match rclcpp minimal service examples * update description strings * remove local_function script because doesn't add any value * add class examples * move comment to proper location * Null response in rclpy client code 1sec wait before sending match cpp prints * add TODO for wait for service * add TODO for accessing response
Simple service examples