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

Gimbal controls publish with WASD, allow simultaneous key presses #180

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

amszuch
Copy link
Contributor

@amszuch amszuch commented Sep 28, 2022

Summary

Closes #N/A

What features did you add, bugs did you fix, etc?
Adapted WASD controls for gimbal from last year. Now, multiple keys can be pressed simultaneously, and data is sent as an int for left-right (A,D) or up-down (W,S), as either -1 (left, down), 1 (right, up), or 0 (noop). Opposing keys neutralize each other, and we can have non-zero values in both axes.

Did you add documentation to the wiki?

No, I assume we already know gimbal is controlled by WASD

How was this code tested?

Integrated into temp controls GUI and rostopic echoed to check for expected behavior

Did you test this in sim?

No

Did you test this on the rover?

No

Did you add unit tests?

No

Copy link
Contributor

@tabiosg tabiosg left a comment

Choose a reason for hiding this comment

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

Resolve conflicts lol looks good though

msg/GimbalCmd.msg Outdated Show resolved Hide resolved
src/teleop/gui/src/components/ControlGUI.vue Show resolved Hide resolved
src/teleop/gui/src/components/GimbalControls.vue Outdated Show resolved Hide resolved
src/teleop/gui/src/components/GimbalControls.vue Outdated Show resolved Hide resolved
src/teleop/gui/src/components/GimbalControls.vue Outdated Show resolved Hide resolved
src/teleop/gui/src/components/GimbalControls.vue Outdated Show resolved Hide resolved
},

created: function () {
document.addEventListener('keydown', this.keymonitordown);
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't need these event listener declarations here since you already use v-on statements in the template section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having issues with v-on event listeners, keeping both for now

this.publish()
},

keymonitorup: function(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great if you could add some quick comments above keymonitor functions, just to explain why they are written the way they are.

Copy link
Contributor

@jnnanni jnnanni left a comment

Choose a reason for hiding this comment

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

Overall looks good and I agree with basically all of Cam's comments. Just needs a few tiny improvements.

Copy link
Contributor

@jnnanni jnnanni left a comment

Choose a reason for hiding this comment

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

Style changes all look good to me, assuming this was teste with rostopic echo this looks all good to me!

Copy link
Contributor

@tabiosg tabiosg left a comment

Choose a reason for hiding this comment

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

Nice work

Copy link
Contributor

@CameronTressler CameronTressler left a comment

Choose a reason for hiding this comment

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

Same as Joe, looking solid. Please confirm this was tested with echo before merging

@amszuch
Copy link
Contributor Author

amszuch commented Oct 25, 2022

Yes, it was tested with echo after the most recent changes

@amszuch amszuch merged commit 2b5fe35 into master Oct 25, 2022
@amszuch amszuch deleted the ams/gimbal_controls branch October 25, 2022 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants