Add joystick/controller configuration wizard for ROVs #2043
Conversation
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
521a818 to
7a281f5
Compare
Signed-off-by: Arturo Manzoli <[email protected]>
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
7a281f5 to
6d6215f
Compare
There was a problem hiding this comment.
Structurally speaking the implementation is good. I don't see any architectural changes to be done.
In general, I think more commenting could be added, specially in the JoystickWizard.vue file, which is very dense in functionality. I tried to read the logic and understand what it was doing, but at some point I started to feel overwhelmed and stopped.
I'm going to test it now, but wanted to leave the comments before so you could start on it sooner.
| class="relative mx-2" | ||
| size="small" | ||
| class="rounded-lg text-md" | ||
| :class="{ | ||
| 'bg-[#FFFFFF22]': controllerStore.protocolMapping.name === functionMapping.name, | ||
| 'text-sm': interfaceStore.isOnSmallScreen, | ||
| }" | ||
| @click="controllerStore.loadProtocolMapping(functionMapping)" |
There was a problem hiding this comment.
Is this a fix on something that is currently broken? If so, could you put it in a separate commit?
There was a problem hiding this comment.
This is resizing the area so the Wizard mapping profile button will fix with the others.
Its a necessary part of the implementation for the wizard feature. Without it, the buttons broke the x overflow limit.
Do you think it belongs anyway to a separate commit?
There was a problem hiding this comment.
This is adding a click listener to the element, not only changing its interface, isn't it?
There was a problem hiding this comment.
Indeed it's adding back the @click listener that was used before the changes you implemented to the profile changing routine.
I'll replace that by the newer implementation.
src/types/joystick.ts
Outdated
| /** | ||
| * Interface representing a single step in the joystick wizard | ||
| */ | ||
| export interface JoystickWizardStep { |
There was a problem hiding this comment.
Is the idea for this type to be used somewhere else besides the joystick wizard component? If not, it better fits the joystick wizard component itself. This types file was created to provide structural logical around the joystick behavior that needs to be shared across components and libraries.
There was a problem hiding this comment.
The idea was to free some lines on the joystick wizard component, that was extensive already. But your'e correct. It will only be used over there and should be kept on the component itself.
Moving it back.. :)
There was a problem hiding this comment.
I think that one is still missing.
| watch(showWizard, (newVal) => { | ||
| interfaceStore.isJoystickWizardVisible = newVal | ||
| if (!newVal) resetWizard() | ||
| }) |
There was a problem hiding this comment.
A general recommendation: it gets a lot easier to read the code if instead of "newVal" we call it taking the context into account:
watch(showWizard, (isShowing, wasShowing) => {
interfaceStore.isJoystickWizardVisible = isShowing
if (!isShowing) resetWizard()
})The "wasShowing" there is not needed. I've put it there just to exemplify how it would be easier to understand in cases where we have logical comparisons between the current state and the previous one.
|
|
||
| watch( | ||
| () => interfaceStore.isJoystickWizardVisible, | ||
| (val) => { |
There was a problem hiding this comment.
The same comment about the "newVal" below applies here.
| nextWizardStep() | ||
| } | ||
|
|
||
| const detectInputs = (state: JoystickState): void => { |
There was a problem hiding this comment.
This is a very dense function. It would benefit from some comments here and there about what exactly its doing. The same applies to the overwriteConflictingMapping and resetCurrentInputDetection methods.
| isLoading.value = true | ||
| try { | ||
| await controllerStore.mergeFunctionsMapping(wizardJoystickMapping.value) | ||
| openSnackbar({ |
There was a problem hiding this comment.
Those openSnackbar calls would benefit a lot from being one-liners when possible, instead of broke into several lines. This reduces the overall workload of reading the code. From what I've saw, all of those can be one-liners.
| return false | ||
| } | ||
|
|
||
| wizardJoystickMapping.value.axesCorrespondencies[idx].action = { |
There was a problem hiding this comment.
Also better as a one-liner.
There was a problem hiding this comment.
Agreed, but this one has overflown the 120 char limit by one character.
| /* eslint-disable-next-line jsdoc/require-jsdoc */ | ||
| const isNoFunction = (action: { id: string }): boolean => action.id === otherAvailableActions.no_function.id | ||
|
|
||
| const mapStepInput = (stepIdFinished: number): boolean => { |
There was a problem hiding this comment.
This function would also benefit from some extra commenting.
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
6d6215f to
03ca4b9
Compare
rafaellehmkuhl
left a comment
rafaellehmkuhl
left a comment
There was a problem hiding this comment.
I had one problem:
I have this joystick which has buttons that are enabled by default. Because of that, when I run the wizard, I can't map the buttons, because this button is always detected as pressed.
Can we detect changes in the buttons' state instead of checking which ones are pressed? This way one can map a joystick even if it has a faulty button (or one that is on by default, like mine).
| /** | ||
| * Standard axis profile using Blue Robotics recommended default mappings for ROV controllers. | ||
| */ | ||
| export const axisConfigMapROV: JoystickAxisActionCorrespondency = { | ||
| 3: { | ||
| action: { id: 'axis_z', name: 'Axis Z', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| min: 1000, | ||
| max: 0, | ||
| }, | ||
| 4: { | ||
| action: { id: 'axis_y', name: 'Axis Y', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| min: -1000, | ||
| max: 1000, | ||
| }, | ||
| 5: { | ||
| action: { id: 'axis_x', name: 'Axis X', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| min: 1000, | ||
| max: -1000, | ||
| }, | ||
| 6: { | ||
| action: { id: 'axis_r', name: 'Axis R', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| min: -1000, | ||
| max: 1000, | ||
| }, | ||
| } | ||
|
|
||
| /** | ||
| * Standard button profile using Blue Robotics recommended default mappings for ROV controllers. | ||
| */ | ||
| export const buttonConfigMapROV: JoystickButtonActionCorrespondency = { | ||
| 7: { action: { id: 'no_function', name: 'Shift (modifier)', protocol: JoystickProtocol.Other } }, | ||
| 9: { action: { id: 'servo_1_max_momentary', name: 'Gripper Open', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 10: { | ||
| action: { id: 'servo_1_min_momentary', name: 'Gripper Close', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| }, | ||
| 12: { action: { id: 'camera-zoom-increase', name: 'Camera Zoom In', protocol: JoystickProtocol.CockpitAction } }, | ||
| 13: { action: { id: 'camera-zoom-decrease', name: 'Camera Zoom Out', protocol: JoystickProtocol.CockpitAction } }, | ||
| 14: { action: { id: 'camera-focus-increase', name: 'Camera Focus Near', protocol: JoystickProtocol.CockpitAction } }, | ||
| 15: { action: { id: 'camera-focus-decrease', name: 'Camera Focus Far', protocol: JoystickProtocol.CockpitAction } }, | ||
| 16: { action: { id: 'btn_auto_focus', name: 'Auto Focus', protocol: JoystickProtocol.CockpitAction } }, | ||
| 17: { action: { id: 'btn_auto_wb', name: 'Auto White Balance', protocol: JoystickProtocol.CockpitAction } }, | ||
| 18: { action: { id: 'gain_inc', name: 'Pilot Gain +', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 19: { action: { id: 'gain_dec', name: 'Pilot Gain –', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 20: { action: { id: 'Arm', name: 'Arm', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 21: { action: { id: 'Disarm', name: 'Disarm', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 22: { action: { id: 'mount_tilt_up', name: 'Camera Tilt Up', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 23: { action: { id: 'mount_tilt_down', name: 'Camera Tilt Down', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 24: { action: { id: 'mount_center', name: 'Camera Tilt Center', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 25: { action: { id: 'lights1_brighter', name: 'Lights Brighter', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 26: { action: { id: 'lights1_dimmer', name: 'Lights Dimmer', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 27: { action: { id: 'trim_pitch_inc', name: 'Trim Pitch Forward', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 28: { | ||
| action: { id: 'trim_pitch_dec', name: 'Trim Pitch Backward', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| }, | ||
| 29: { action: { id: 'trim_roll_inc', name: 'Trim Roll Right', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 30: { action: { id: 'trim_roll_dec', name: 'Trim Roll Left', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 31: { action: { id: 'mode_manual', name: 'Manual Mode', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 32: { action: { id: 'mode_depth_hold', name: 'Depth-hold Mode', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 33: { action: { id: 'mode_stabilize', name: 'Stabilize Mode', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 34: { action: { id: 'input_hold_set', name: 'Toggle Input Hold', protocol: JoystickProtocol.MAVLinkManualControl } }, | ||
| 35: { | ||
| action: { id: 'roll_pitch_toggle', name: 'Roll & Pitch Toggle', protocol: JoystickProtocol.MAVLinkManualControl }, | ||
| }, |
There was a problem hiding this comment.
Isn't those the same we have in joystick-profiles.ts? If there were changes in the recommended mapping, we should update and consume from there, so we don't have to maintain two codebases.
Sure, it's a better way to detect the inputs. I'll make the change |
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
03ca4b9 to
91e491c
Compare
|
@rafaellehmkuhl The initial plan was, like you suggested, to detect on any state changes. |
rafaellehmkuhl
left a comment
rafaellehmkuhl
left a comment
There was a problem hiding this comment.
When this 4th mapping is added, the current UI of the joystick config view is breaking.
This is with the active selected:
This is with the Wizard one selected:
Fixed! select_profile.mp4 |
rafaellehmkuhl
left a comment
rafaellehmkuhl
left a comment
There was a problem hiding this comment.
That session is still overflowing. Can we put an horizontal scroll on it if necessary? Or maybe arrow buttons on the sides when there are hidden mappings?
Also I think the "start wizard" button was removed. I cannot see it even if I delete the wizard profile from the local storage.
src/types/joystick.ts
Outdated
| /** | ||
| * Interface representing a single step in the joystick wizard | ||
| */ | ||
| export interface JoystickWizardStep { |
There was a problem hiding this comment.
I think that one is still missing.
That's funny, I have technically the same content you are displaying, but it doesn't overflow here. Might be an OS specific difference. Ill increase a little the JoyConfigView page width and include a horizontal scrollbar for the small screen cases. 
|
|
@ArturoManzoli as discussed externally the only thing that is not intuitive today is the direction of the axes when defining which one goes "up" and "down". The rest is working perfectly! |
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
9289b60 to
1c737e9
Compare
Signed-off-by: Arturo Manzoli <[email protected]>
ArturoManzoli
force-pushed
the
1805-add-joystick-wizard
branch
from
1c737e9 to
289b520
Compare
|
@rafaellehmkuhl Added the binding indication arrows! |
rafaellehmkuhl
left a comment
rafaellehmkuhl
left a comment
There was a problem hiding this comment.
- When there are no Wizard profiles, the existing profiles are not centralized:
- In the forward/backward step, the arrows are out of proportion:
- The profile selection could have a little more bottom margin so the scroll bar is not over the cards:
- There's no way to invert the axes currently. If I have a joystick where pulling the stick is generating positive values, and that is considered forward, I cannot invert this behavior so pulling is backward and pushing is forward.
Besides those problems, the wizard itself is super cool, specially for those using flight sticks.
3 participants
Add a controller configuration wizard for ROVs. Other vehicle types to be supported in the future.
MAV_TYPE_SUBMARINE;Tests Required to Merge (by dev and/or testers)
Closes #1805