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

Joystick: Support extended MANUAL_CONTROL message (buttons2 + s&t axes) #559

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Nov 10, 2023

Needs mavlink2rest and ardusub versions that support it. Currently, both BlueOS and ArduSub have support for it on master, but not on stable versions.

Cockpit will keep working for controlling the vehicle even if one of those two do not support the extended messages, and will just use the non-extended fields (buttons2 and x+y+z+r).

Fix #182

@ES-Alexander
Copy link
Contributor

Since this is based on an optional extension of the MAVlink protocol, we should likely support both options, with a toggle-switch to go between 4 axes + 16 buttons and 6 axes + 32 buttons, located in the same place as the controls suggested in #644 (likely above the tabbed section from #617, since these settings are global and not per-joystick).

If we do make this optional, we can probably merge it in already. The extension has been around long enough that MAVLink2REST probably includes it already, and the latest ArduSub master already supports the additional buttons and axes, so it would be useful to make this available to the people who want to test the functionality.

@ES-Alexander ES-Alexander changed the title Add support for the buttons2 field in the MAVLink ManualControl message Joystick: Support extended MANUAL_CONTROL message (buttons2 + s&t axes) Dec 18, 2023
@rafaellehmkuhl
Copy link
Member Author

Since this is based on an optional extension of the MAVlink protocol, we should likely support both options, with a toggle-switch to go between 4 axes + 16 buttons and 6 axes + 32 buttons, located in the same place as the controls suggested in #644 (likely above the tabbed section from #617, since these settings are global and not per-joystick).

If we do make this optional, we can probably merge it in already. The extension has been around long enough that MAVLink2REST probably includes it already, and the latest ArduSub master already supports the additional buttons and axes, so it would be useful to make this available to the people who want to test the functionality.

Humn, didn't thought about people going back and forth. Should we support it? In this case, what do we do if they opt out after opting in? Clear the buttons/axes that use extra slots?

PS: to merge this, we have to rebase it over the last joystick pipeline update and solve te conflicts.

@ES-Alexander
Copy link
Contributor

Humn, didn't thought about people going back and forth. Should we support it? In this case, what do we do if they opt out after opting in? Clear the buttons/axes that use extra slots?

Clearing/disabling would indeed be the most straightforward implementation I can think of, perhaps with a warning/confirmation popup first with an option to save their current joystick profile as a backup.

If we want to be "smart" then the most ideal approach I can think of would be to

  1. attempt to re-map where possible (e.g. for the motion axes when switching between MANUAL_CONTROL and RC_CHANNELS_OVERRIDE, since ArduSub has some pre-defined defaults for the RC channels), then for everything else
  2. keep the extra functionalities stored but just not use them, in which case they'd need to be formatted differently to display that meaningfully to the user (e.g. greyed out (or red/orange?) and with a strikethrough, possibly also with a hover text that explains it's not available with the current axes protocol)

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Dec 18, 2023

Humn, didn't thought about people going back and forth. Should we support it? In this case, what do we do if they opt out after opting in? Clear the buttons/axes that use extra slots?

Clearing/disabling would indeed be the most straightforward implementation I can think of, perhaps with a warning/confirmation popup first with an option to save their current joystick profile as a backup.

If we want to be "smart" then the most ideal approach I can think of would be to

  1. attempt to re-map where possible (e.g. for the motion axes when switching between MANUAL_CONTROL and RC_CHANNELS_OVERRIDE, since ArduSub has some pre-defined defaults for the RC channels), then for everything else
  2. keep the extra functionalities stored but just not use them, in which case they'd need to be formatted differently to display that meaningfully to the user (e.g. greyed out (or red/orange?) and with a strikethrough, possibly also with a hover text that explains it's not available with the current axes protocol)

I think the most sane solution would be to just not remap anything and let the user know if 32buttons+6axis is not supported.

With the possibility of creating different joystick profiles, users can just have one for each firmware, if needed.

@ES-Alexander
Copy link
Contributor

I think the most sane solution would be to just not remap anything and let the user know if 32buttons+6axis is not supported.

I believe that's what I described in step 2, although skipping step 1 would likely have issues if we do start supporting additional axes protocols (like RC_CHANNELS_OVERRIDES).

@rafaellehmkuhl
Copy link
Member Author

I think the most sane solution would be to just not remap anything and let the user know if 32buttons+6axis is not supported.

I believe that's what I described in step 2, although skipping step 1 would likely have issues if we do start supporting additional axes protocols (like RC_CHANNELS_OVERRIDES).

Yeah...
Maybe we should be more naive and just allow mapping only part of the channels.
The problem with that is that we would need to know which is the zero position for that vehicle. If all zero positions were actually zero, that wouldn't be a problem.

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Dec 21, 2023
@rafaellehmkuhl rafaellehmkuhl force-pushed the add-buttons2 branch 2 times, most recently from 72ff11f to b07cdc8 Compare January 10, 2024 21:45
@rafaellehmkuhl rafaellehmkuhl marked this pull request as ready for review January 10, 2024 21:45
@patrickelectric
Copy link
Member

Needs: bluerobotics/BlueOS#2295

@rafaellehmkuhl rafaellehmkuhl force-pushed the add-buttons2 branch 2 times, most recently from 8922a40 to c1ede53 Compare January 12, 2024 18:50
@rafaellehmkuhl
Copy link
Member Author

@Williangalvani added a warning on the joystick configuration page for when the m2r/vehicle does not support the extended message.

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

I haven't tested, because I'm away this week, but here are a couple of code/language suggestions.

If there isn't already one I still think it would be useful to have a user option to toggle extended on/off (potentially enabled as an override via a checkbox), to better support vehicle and firmware types Cockpit doesn't already know about (e.g. if someone has a MAVLink vehicle that's not running ArduPilot).

src/libs/joystick/protocols/mavlink-manual-control.ts Outdated Show resolved Hide resolved
src/views/ConfigurationJoystickView.vue Outdated Show resolved Hide resolved
@Williangalvani
Copy link
Member

the axis worked as expected here. I couldn't get the warning to show by using Sub 4.1.1

In the case all the button slots are already taken, we try to change the last button slots first, as in the case of vehicles with 32 buttons available, we prevent changing the first 16, which are usually used by other ground control stations, preventing clashs.
Wrapper library for `fetch`, with support for typescript and timeouts.
@rafaellehmkuhl
Copy link
Member Author

the axis worked as expected here. I couldn't get the warning to show by using Sub 4.1.1

Strange. I have 4.1.1 (STABLE) here and received the warning:

image

…k2rest/ardupilot that do not support extended `MANUAL_CONTROL` message
@Williangalvani
Copy link
Member

all good now, I think I had an earlier version checked out already. re-rested and it seems to be all good.

@patrickelectric patrickelectric merged commit 14cc7c2 into bluerobotics:master Jan 31, 2024
7 checks passed
@rafaellehmkuhl
Copy link
Member Author

Just to register, I changed the warning layout a little bit on the last push so it get's less ugly:

image

@rafaellehmkuhl rafaellehmkuhl deleted the add-buttons2 branch January 31, 2024 21:44
@ES-Alexander ES-Alexander added docs-complete Change documentation has been completed and removed docs-needed Change needs to be documented labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-complete Change documentation has been completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joystick does not use s, t and buttons2 field in MAVLink
4 participants