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

Copter: choose RC channel for Transmitter Based Tuning #25203

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

be-ska
Copy link
Contributor

@be-ska be-ska commented Oct 7, 2023

This PR add the ability to choose which RC channel controls the Transmitter Based Tuning, instead of the hard coded channel 6.
If a user want to use channel x for transmitter based tuning, the parameter RCx_OPTION should be set to 177 (Transmitter Tuning), as discussed in issue #25186.

The changes has been tested in SITL after setting parameters:

  • TUNE 21
  • TUNE_MIN 0
  • TUNE_MAX 0.03
  • RC9_OPTION 177

Changing the PWM value of CH8 results in different ATC_RAT_RLL_D values.

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Oct 7, 2023
@Hwurzburg
Copy link
Collaborator

why not just adopt the tx tune used by plane? adding a single vehicle aux function seems like the wrong approach to me

@peterbarker
Copy link
Contributor

why not just adopt the tx tune used by plane? adding a single vehicle aux function seems like the wrong approach to me

Because that's a different issue, and Randy was opposed last time it came up. tridge's oldest PR adds that support: #4065

This PR adds flexibility we don't currently don't have, and the problem vexed the author of this PR enough that he made a PR to fix it. I think this PR is well-justified.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Looks right. Just a stylistic thing of doing an early-return rather than complicating a compound expression.

Please state what testing has been done.

ArduCopter/tuning.cpp Show resolved Hide resolved
ArduCopter/tuning.cpp Outdated Show resolved Hide resolved
@be-ska
Copy link
Contributor Author

be-ska commented Oct 8, 2023

@peterbarker I've applied the style fix as requested.
At the moment I've only tested in SITL: I've tried assigning different channels changing the RCx_OPTION to 177 and it works as it supposed to: I can see the ATC_* parameters changing and the controller overshoot if setting gains too high via transmitter. I also tested the return when rc_tuning == nullptr and it works.

@be-ska
Copy link
Contributor Author

be-ska commented Oct 8, 2023

@Hwurzburg Sorry, I'm not a Plane user and I didn't know there was a different transmitter based tuning for Plane. Just my personal opinion, but I think it would be more intuitive if all the RC input settings were in the same place. This could avoid confusion: just looking at all the RCx_OPTION values, users know which transmitter inputs are enabled and for which function. So maybe also Plane could use the same RCx_OPTION value instead of the TUNE_CH parameter.

@tpwrules
Copy link
Contributor

Are you still interested in working on this? I would like it as well but it needs to be rebased and some of the comments cleaned up.

@be-ska
Copy link
Contributor Author

be-ska commented Jul 20, 2024

@tpwrules sorry for the late reply, I was quite busy on other projects this month. I can easily rebase it, but as @Hwurzburg pointed out Transmitter Tuning for Plane has a different way to choose the RC channel, so it doesn't seem right to have a different way to set the same function on Copter.

@rmackay9
Copy link
Contributor

I agree with others that this PR looks correct. The only change I'd recommend is that the enum used be changed to 219 because that's where we are putting the items that are really sliders/knobs instead of switches.

@be-ska
Copy link
Contributor Author

be-ska commented Jul 22, 2024

I've rebased it and changed the used enum to 219 as @rmackay9 suggested.

Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

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

LGTM, nice, thanks!

@rmackay9 rmackay9 dismissed peterbarker’s stale review July 22, 2024 23:00

PeterB's requests have been addressed

@Georacer
Copy link
Contributor

@rmackay9
Copy link
Contributor

It looks like it's failing the Ch6TuningWPSpeed autotest and the code is here. We will just need to add a new parameter setting to the table, probably like this:

        self.set_parameters({
            "RC6_OPTION": 219,  #RC6 used for tuning
            "TUNE": 10,  # 10 is waypoint speed
            "TUNE_MIN": 0.02,  # 20cm/s
            "TUNE_MAX": 1000,  # 10m/s
            "AUTO_OPTIONS": 3,
        })

@tridge
Copy link
Contributor

tridge commented Jul 24, 2024

@be-ska this can be merged when you make the one line change in Tools/autotest/arducopter.py to set the parameter

@be-ska
Copy link
Contributor Author

be-ska commented Jul 24, 2024

Hey @tridge, I've rebased it and modified the autotest script, it should now pass the test

@be-ska
Copy link
Contributor Author

be-ska commented Jul 24, 2024

I believe we'll need a wiki update on https://ardupilot.org/copter/docs/common-transmitter-tuning.html#common-transmitter-tuning

I've just updated the wiki with PR ArduPilot/ardupilot_wiki#6137

@peterbarker peterbarker merged commit 9acd23d into ArduPilot:master Jul 24, 2024
93 checks passed
@peterbarker
Copy link
Contributor

Fantastic, thanks @be-ska !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants