-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
crsf_rc: add bind command #23294
base: main
Are you sure you want to change the base?
crsf_rc: add bind command #23294
Conversation
8a47282
to
17aa86a
Compare
@dagar I've tested and verified that this works. |
bd9560b
to
e0f70a6
Compare
The https://mavlink.io/en/messages/common.html#MAV_CMD_START_RX_PAIR message that this hooks into assumes you're going to specify a type of RC. If it is possible to have multiple radios that might pair on a vehicle then you should add ELRS as an option to https://mavlink.io/en/messages/common.html#RC_TYPE and check the command for the type being set. |
QGC assumes Spektrum RC because that is the only thing that is supported by the message. Do you plan to update that section of QGC to allow ELRS to to be configured too? If not, how were you expecting this to be called? Either way, we should document that this can be used and how in https://docs.px4.io/main/en/config/radio.html#additional-radio-setup |
Yeah I agree. Right now it accepts any bind message since most vehicles just have one RC receiver, but I can imagine having multiple. Not sure when I'll get around to it
Either with the command-line or with a QGC button. Currently the spektrum bind button works, but it's a little confusing. I'm also not sure when I'll get around to it |
e0f70a6
to
8b0d2c9
Compare
@hamishwillee could you take another look at these changes? I think I addressed all your concerns |
src/drivers/rc/crsf_rc/CrsfRc.cpp
Outdated
uint8_t cmd_ret = vehicle_command_ack_s::VEHICLE_CMD_RESULT_UNSUPPORTED; | ||
|
||
if (!_is_singlewire && !_armed) { | ||
if ((int)vcmd.param1 == 1) { |
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.
We should change this to not be a magic number. I'll discuss the MAVLink PR this evening.
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.
P.S. I won't be reviewing this other than the MAVLink aspects (not a developer)
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.
I agree, if the mavlink PR goes in, will the enum be available here?
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.
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.
@hamishwillee I was able to use the updated mavlink enum in QGC, however it doesn't appear available in PX4.
mavlink/qgroundcontrol@2a8bcdf
I've seen duplicate enum definitions copied from the common.xml into header files for different modules. Should I do the same here, or is there a way to map to the mavlink common.xml?
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.
@dagar I've updated vehicle_command with the mavlink enums. Let me know how this looks
77c14e7
to
4a530ac
Compare
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
70f8e2e
to
ad16fd8
Compare
ad16fd8
to
1336fb2
Compare
Solved Problem
Ability to bind using the crsf protocol
Fixes #23281
Solution
Changelog Entry
For release notes:
Alternatives
Add to rc_input module if needed
Test coverage
Waiting on:
Finally update docs: