-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
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.
Resolve conflicts lol looks good though
}, | ||
|
||
created: function () { | ||
document.addEventListener('keydown', this.keymonitordown); |
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.
You probably don't need these event listener declarations here since you already use v-on statements in the template section
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.
Having issues with v-on event listeners, keeping both for now
this.publish() | ||
}, | ||
|
||
keymonitorup: function(event) { |
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.
Would be great if you could add some quick comments above keymonitor functions, just to explain why they are written the way they are.
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.
Overall looks good and I agree with basically all of Cam's comments. Just needs a few tiny improvements.
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.
Style changes all look good to me, assuming this was teste with rostopic echo this looks all good to me!
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.
Nice work
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.
Same as Joe, looking solid. Please confirm this was tested with echo before merging
Yes, it was tested with echo after the most recent changes |
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