Teleop panel #561

jreyesr · 2020-12-16T16:42:37Z

(First time contributor, so apologies for any mistakes)

Summary

This PR adds a Teleoperation panel which operates in a similar way to the teleop_twist_keyboard node. It can be controlled by clicking buttons or with the arrow keys.

The main motivation of this change would be its use in remotely controlled (i.e., non-autonomous) robots. Such robots can't currently be easily controlled via Webviz (at least, I don't know of a simple way), and it would be nice to have the teleoperation interface next to, for example, an Image panel showing the robot cameras, or a Map panel showing the robot location (or both!).

This PR started by copying the Publish panel, since its functionality is similar. The major difference is that the Teleop panel sends specific messages to a fixed (configurable) topic with a fixed (non-configurable) datatype, instead of arbitrary mesages to any topic. Unnecessary settings were deleted, and the UI was edited to only contain a few buttons which trigger a message publication via a Publisher component. A global KeyListener element was also added, to allow keypresses to also trigger the messages.

Test plan

The new panel was manually tested on a local installation, with ROS running on a Docker container and the Websockets server exposed via port 9090. This change makes no sense when using local or remote bags.
The test files from the Publish panel were also copied, adapted and run. All tests pass successfully.

Versioning impact

This PR should only require a minor semver update.

Addendum: Known limitations & questions

At present, the panel suffers from issue Error when Publisher component is rendered before rosClient connection hook runs #534 which is also present in the Publish panel, since they both use the same Publisher component. However, unlike the Publish panel, this one displays an error message over the panel, with a "Reload Panel" button. Clicking on the button (seems to) fix the error. A fix to the linked issue should (?) remove this bug.
The panel only allows four directions (up, down, left, right) plus a Stop command. More complex robots can use up to nine keys for direction, plus a few extra to change rotation and translation speed. Should those additional keys be implemented?
Currently, the four directions are bound to the arrow keys, and Stop is bound to the Space key. Should the keybindings be configurable? If so, how? Maybe a dropdown for every command, with a list of valid keys?

vidaaudrey

Thanks for your contribution. Added a few comments. There are some lint errors in CI. If you are using vscode, you could install eslint to auto format the code. Let me know if you need any help.

packages/webviz-core/src/panels/Teleop/index.js

vidaaudrey · 2020-12-16T17:46:35Z

packages/webviz-core/src/panels/Teleop/index.js

+  return { error, parsedObject };
+}
+
+class Teleop extends React.PureComponent<Props, PanelState> {


Could you convert this component to functional component. We've converted the majority of components but still a few left. Would be nice to use the new syntax.

Done (I think so, at least, I don't really work on React and so I had to google what a functional component is). Can you confirm that I actually converted the component to functional style, please?

packages/webviz-core/src/panels/Teleop/index.js

vidaaudrey · 2020-12-16T17:49:53Z

packages/webviz-core/src/panels/Teleop/index.js

+            type="text"
+            value={config.topicName}
+            onChange={(event) => {
+              saveConfig({ topicName: event.target.value });


Would need some data validation to ensure the saved topic name is valid

Not sure about what would be required to validate the topic. Would it require checking that the topic name exists and it's of the correct datatype? Doing that would limit use a lot, I think, since it may very well happen that the topic doesn't "exist" since no other nodes have ever published to it, and the Teleop panel will be the only one publishing. Any pointers?
Perhaps a) if it doesn't exist, just accept it, and b) if it already exists, ensure that the datatype is actually "geometry_msgs/Twist"?

Sorry about the late reply. A simple validation like this should be ok:

name is not empty

name should start with /

name should be at least 2 characters

You could refer to some of the validators here: https://github.com/cruise-automation/webviz/blob/master/packages/webviz-core/shared/validators.js#L102-L107

I've added some validation on 671f133. It shows an error message on the panel if those conditions are not met. It also disables all the buttons and keypresses do nothing.

Remove color setting for buttons Add help popup

jaguardo · 2020-12-22T01:42:33Z

@jreyesr I tried the changes (granted, I might be missing something as well!) but I couldn't get the pop up panel for the add panel or change panel to list the teleop as an option... is there something else that has to changes after making those changes? thanks! From the pictures, I love the functionality, great!

Edit to add: I think it works if you run

npm run build
npm run docs

but I don't see it when I try

npm run build-static-webviz

and host the static_webviz directory, must be some nuance between the 2 that I haven't picked up on yet.

Edit to add: Working on Chrome, but not Edge (not a big deal!) the option doesn't show up in Edge... and getting an error

Error stack:
TypeError: Class constructor KeyListener cannot be invoked without 'new'
    at World (http://192.168.1.181:8080/hooks_bundle.js:1253:13120)
    at wh (http://192.168.1.181:8080/webvizCoreBundle.js:245:69605)
    at Zh (http://192.168.1.181:8080/webvizCoreBundle.js:245:76034)
    at Vj (http://192.168.1.181:8080/webvizCoreBundle.js:245:116007)
    at Uj (http://192.168.1.181:8080/webvizCoreBundle.js:245:99659)
    at Mj (http://192.168.1.181:8080/webvizCoreBundle.js:245:99588)
    at xj (http://192.168.1.181:8080/webvizCoreBundle.js:245:96588)
    at http://192.168.1.181:8080/webvizCoreBundle.js:245:56116
    at exports.unstable_runWithPriority (http://192.168.1.181:8080/webvizCoreBundle.js:245:132024)
    at Yf (http://192.168.1.181:8080/webvizCoreBundle.js:245:55825)
    ```

jreyesr · 2020-12-22T16:48:59Z

@jaguardo Weird. I just ran build-static-webviz from scratch and it does show me the new panel (on Chrome, Linux). Also tested it on Edge 87.0.664.60, and it works nicely. No idea where that error came from, I do use a KeyListener, but I use it in the same way that other panels (e.g., the Plot panel) use it.

jreyesr · 2020-12-22T17:14:57Z

@vidaaudrey I added a few commits fixing the linter errors and converting to a functional component. CI still fails, but the error now happens when attempting to upload something (screenshots, I think) to S3; the error says Missing credentials in config.
Still unsure about the data validation (see #561 (comment)).

vidaaudrey · 2020-12-22T20:09:55Z

@jreyesr we are aware of the CI issue and we have a task to migrate the screenshot testing to GCP soon. At the same time, I just made a copy of your branch at #567 so you could check the CI output in 1 hour or so.

amacneil · 2021-08-17T16:37:32Z

For anyone watching this issue - we recently added a teleop panel to Foxglove Studio (webviz fork) in #1429.

pringithub · 2021-10-08T21:16:48Z

@jaguardo Weird. I just ran build-static-webviz from scratch and it does show me the new panel (on Chrome, Linux). Also tested it on Edge 87.0.664.60, and it works nicely. No idea where that error came from, I do use a KeyListener, but I use it in the same way that other panels (e.g., the Plot panel) use it.

@jreyesr Doesn't work for me either. I did a clean build and it doesn't show up with serve-static-webviz. Something I'm doing wrong? It seems

jreyesr added 3 commits December 16, 2020 10:56

Add teleop panel (topic not configurable)

aa0787f

Add tests

15b7bad

Add topic setting to publish Teleop msg

396c92a

vidaaudrey reviewed Dec 16, 2020

View reviewed changes

jreyesr added 5 commits December 16, 2020 16:07

Fix linter errors

54c156a

Change to functional (?) component

5403325

Remove color setting for buttons Add help popup

Fix linter error

5575c22

Fix linter error, second attempt

4691c7d

Fix Flow errors

00fa329

jreyesr requested a review from vidaaudrey December 22, 2020 17:15

vidaaudrey mentioned this pull request Dec 22, 2020

Jreyesr teleop panel (copy of #561) #567

Open

Add validations for topic name

671f133

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teleop panel #561

Teleop panel #561

jreyesr commented Dec 16, 2020

vidaaudrey left a comment

vidaaudrey Dec 16, 2020

jreyesr Dec 17, 2020

vidaaudrey Dec 16, 2020

jreyesr Dec 17, 2020

vidaaudrey Jan 4, 2021

jreyesr Jan 5, 2021

jaguardo commented Dec 22, 2020 •

edited

Loading

jreyesr commented Dec 22, 2020

jreyesr commented Dec 22, 2020

vidaaudrey commented Dec 22, 2020

amacneil commented Aug 17, 2021

pringithub commented Oct 8, 2021

Teleop panel #561

Are you sure you want to change the base?

Teleop panel #561

Conversation

jreyesr commented Dec 16, 2020

Summary

Test plan

Versioning impact

Addendum: Known limitations & questions

vidaaudrey left a comment

Choose a reason for hiding this comment

vidaaudrey Dec 16, 2020

Choose a reason for hiding this comment

jreyesr Dec 17, 2020

Choose a reason for hiding this comment

vidaaudrey Dec 16, 2020

Choose a reason for hiding this comment

jreyesr Dec 17, 2020

Choose a reason for hiding this comment

vidaaudrey Jan 4, 2021

Choose a reason for hiding this comment

jreyesr Jan 5, 2021

Choose a reason for hiding this comment

jaguardo commented Dec 22, 2020 • edited Loading

jreyesr commented Dec 22, 2020

jreyesr commented Dec 22, 2020

vidaaudrey commented Dec 22, 2020

amacneil commented Aug 17, 2021

pringithub commented Oct 8, 2021

jaguardo commented Dec 22, 2020 •

edited

Loading