search

gazebosim / gz-launch

Run and manage programs and plugins.
https://gazebosim.org
Apache License 2.0
12 stars 17 forks source link

Add pause and stop to Websocket Server #187

Closed mabelzhang closed 1 year ago

mabelzhang commented 2 years ago

🎉 New feature

Add ability to pause and stop simulation in Websocket Server.

Summary

This may not be the best scheme: In operation field, specify sim. In message_type field (might not be the best place), specify pause play or stop. In topic_name field, specify /server_control service for stop, and the WorldControl service for pause and play, which should look like /world/empty/control, replacing empty with the world name.

If we don't want to put those action words in the message_type field, another alternative might be to specify pause/play/stop in the operation field. Then there wouldn't be a high level sim operation, it'd just be 3 separate operations. That seems less organized though.

Test it

I didn't see any tests for this class, so I guess we have to test it using a web application? @german-e-mas could you try and let me know your feedback?

Compile:

colcon build --merge-install --packages-select ignition-launch5

Launch a simulation world and the websocket:

ign gazebo -v 4 <world.sdf>
ign launch -v 4 ~/ign_fortress/src/ign-launch/examples/websocket.ign

Then, send the commands above through the websocket (I'm not sure how to do that on my end). For example,

# Pause and play
['sim', '/world/challenge/control', 'pause', '']
['sim', '/world/challenge/control', 'play', '']

# These two should do the same thing:
['sim', '', 'stop', '']
['sim', '/server_control', 'stop', '']

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

codecov[bot] commented 2 years ago

Codecov Report

Merging #187 (7d2cf03) into ign-launch5 (cdc6270) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 7d2cf03 differs from pull request most recent head 6b78cbf. Consider uploading reports for the commit 6b78cbf to get more accurate results

@@             Coverage Diff              @@
##           ign-launch5     #187   +/-   ##
============================================
  Coverage        58.83%   58.83%           
============================================
  Files                3        3           
  Lines              396      396           
============================================
  Hits               233      233           
  Misses             163      163

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

nkoenig commented 2 years ago

This approach can be generalized to a service interface. The websocket message would be 

['service', 'SERVICE_TOPIC', 'SERVICE_REQ_TYPE', 'PROTOBUF_MESSAGE']

When the websocket server receives this message, it would call the specified service.

mabelzhang commented 2 years ago

Generalized to service interface in 5858257 I'm not sure if I did it right / if it is going to work though. @german-e-mas could you give it a try? The 4th field is expected to be a serialized string:

    /// The `payload` component is mandatory for the "pub" operation and
    /// optional for the "service" operation. If present, it contains a
    /// serialized string of an Ignition Message.

One thing I ran into is that, in order to construct a message with the type specified as a string, I used the ignition::msgs::Factory::New(), which returns the generic ignition::transport::ProtoMsgPtr type. All the service Request() functions are templated. In order to call Request() with a request being the generic ProtoMsgPtr, it seems I had to use the generic type for the reply as well, otherwise it didn't compile.

But there aren't enough fields in the 4 fields to specify a service response type. So currently, I hardcoded the response type to ignition::msgs::Boolean, which is the type that we need. But this will probably error when we call a service that doesn't respond with a Boolean.

german-e-mas commented 2 years ago

I'm getting an error when I try to send a message.

[ign-8] [libprotobuf ERROR google/protobuf/text_format.cc:307] Error parsing text-format ignition.msgs.WorldControl: 1:1: Expected identifier, got: {

I'm trying to send the following:

[
  'service',
  '/world/bumpy/control',
  'ignition.msgs.WorldControl',
  '{"pause":true}',
]
german-e-mas commented 2 years ago

Note: I also tried sending "pause: true", without the brackets, and also got the error:

[ign-8] [libprotobuf ERROR google/protobuf/text_format.cc:307] Error parsing text-format ignition.msgs.WorldControl: 1:1: Expected identifier, got: "pause: true"

I'm not sure if this is an issue in the code, or in the message I need to send.

nkoenig commented 2 years ago

@german-e-mas , did you serialize the protobuf message that you sent to the websocket server?

german-e-mas commented 2 years ago

@german-e-mas , did you serialize the protobuf message that you sent to the websocket server?

I believe I did not. I'll try that and let you know how it goes!

german-e-mas commented 2 years ago

I also tried the following. Were you referring to this @nkoenig ?

const msgDef = this.root!.lookupType('ignition.msgs.WorldControl');
const msg = msgDef.create({ pause: true });

const buf = msgDef.encode(msg).finish();
const strBuf = new TextDecoder().decode(buf);

Sending strBuf as payload results in:

[ign-8] [libprotobuf ERROR google/protobuf/text_format.cc:307] Error parsing text-format ignition.msgs.WorldControl: 1:1: Invalid control characters encountered in text.

I know we should be sending a string, but I'm not sure what does the Factory.New method requires.

mabelzhang commented 2 years ago

The string should be a binary string. I don't know on the web side how you encode it. On the C++ side, it's something like SerializeToString (or I believe we have SerializeAsString like in this line

https://github.com/gazebosim/gz-launch/blob/47595085bb9bc51f1b9f70a68dfd9784dd86df12/plugins/websocket_server/WebsocketServer.cc#L795

german-e-mas commented 2 years ago

Gzweb changes: https://github.com/gazebo-web/gzweb/pull/32

mabelzhang commented 2 years ago

@nkoenig Could we get another review on the new implementation?

nkoenig commented 2 years ago

@german-e-mas , I've changed this to use serialized protobuf messages. You now need to pass in a serialized request message instead of a text string.

This also requires: https://github.com/gazebosim/gz-transport/pull/351

german-e-mas commented 2 years ago

@german-e-mas , I've changed this to use serialized protobuf messages. You now need to pass in a serialized request message instead of a text string.

This also requires: gazebosim/gz-transport#351

I checked out gz-transport and this branch and built both, but I haven't been able to play or pause the simulation.

What I do see is that the message is being sent back to the client (["service", "/world/bumpy/control", "ignition.msgs.Boolean", "\u0010\u0001"]) which I'm not sure if we should either use it, or take it as a confirmation that the message was received by the server.

I'll keep trying.

german-e-mas commented 2 years ago

I checked out gz-transport and this branch and built both, but I haven't been able to play or pause the simulation.

What I do see is that the message is being sent back to the client (["service", "/world/bumpy/control", "ignition.msgs.Boolean", "\u0010\u0001"]) which I'm not sure if we should either use it, or take it as a confirmation that the message was received by the server.

I'll keep trying.

Got it working! I'll change the gzweb branch to reflect the required changes.

nkoenig commented 1 year ago

This has been updated to match the latest changes in https://github.com/gazebosim/gz-transport/pull/351.

nkoenig commented 1 year ago

@osrf-jenkins run tests please.