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

Arming check duplicate switch options on Rover and Plane problem #14443 #14459

Merged
merged 2 commits into from
Jun 1, 2020
Merged

Arming check duplicate switch options on Rover and Plane problem #14443 #14459

merged 2 commits into from
Jun 1, 2020

Conversation

mmk0102
Copy link
Contributor

@mmk0102 mmk0102 commented May 27, 2020

move rc().duplicate_options_exist() from Copter directory to general library AP_Arming.
(have to create new branch cause CI test problem in prev one)

Tested only in positive way: code print me message when it successfully pass :
if (rc().duplicate_options_exist())
for plane and copter in SITL.
To check it in negative I have to set duplicate options from Mavlink console. I dont know such command yet.

@mmk0102
Copy link
Contributor Author

mmk0102 commented May 28, 2020

looks like I mb know the problem in last case: I should put added check-code after block
"only check if we've recieved some form of input within the last second"

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.

Not really sure why we have three PRs for the same change now :-)

Still need to fix the commit messages - and remove trailing whitespace from this one.

Did you want me to finish this one off for you, or do you want to push through?

@mmk0102
Copy link
Contributor Author

mmk0102 commented May 28, 2020

"Did you want me to finish this one" - sure.

I think maybe this code need some refactoring in future eventually:

  1. if we can change parameters only once (if it want reset soft) should we test
    "if (rc().duplicate_options_exist())" each time ? May be if it happened we will set flag (static bool) in method and pass it next time?

  2. in "duplicate_options_exist()" method we check all params even if "auxsw_option_counts[i] > 1" yet. Why ?
    Wouldn't better move this code to first cycle and remove second at all.
    Now it looks like:
    bool RC_Channels::duplicate_options_exist()
    {
    uint8_t auxsw_option_counts[256] = {};
    for (uint8_t i=0; i<NUM_RC_CHANNELS; i++) {
    const RC_Channel *c = channel(i);
    if (c == nullptr) {
    // odd?
    continue;
    }
    const uint16_t option = c->option.get();
    if (option >= sizeof(auxsw_option_counts)) {
    continue;
    }
    auxsw_option_counts[option]++;
    }

    for (uint16_t i=0; i<sizeof(auxsw_option_counts); i++) {
    if (i == 0) { // MAGIC VALUE! This is AUXSW_DO_NOTHING
    continue;
    }
    if (auxsw_option_counts[i] > 1) {
    return true;
    }
    }
    return false;
    }
    I want change to (in next pull req):
    bool RC_Channels::duplicate_options_exist()
    {
    uint8_t auxsw_option_counts[256] = {};
    for (uint8_t i=0; i<NUM_RC_CHANNELS; i++) {
    const RC_Channel *c = channel(i);
    if (c == nullptr) {
    // odd?
    continue;
    }
    const uint16_t option = c->option.get();
    if (option >= sizeof(auxsw_option_counts)) {
    continue;
    }
    auxsw_option_counts[option]++;
    if ( i!=0 && auxsw_option_counts[i] > 1) return true;
    }
    return false;
    }

@peterbarker
Copy link
Contributor

We want to run this test each time to catch someone doing silly immediately they change a parameter. We don't hook parameter sets, so a periodic prearm check is appropriate.

@mmk0102
Copy link
Contributor Author

mmk0102 commented May 28, 2020

Well, if you can finish this one should I convert this Pull req to draft ?

@peterbarker peterbarker self-requested a review May 28, 2020 13:03
@peterbarker
Copy link
Contributor

Closes #14443

@peterbarker
Copy link
Contributor

Fixed commit messages, removed whitespace, force-pushed it up.

@tridge tridge merged commit 22d052b into ArduPilot:master Jun 1, 2020
@peterbarker
Copy link
Contributor

Thanks @mmk0102 - we merged this!

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

Successfully merging this pull request may close these issues.

None yet

4 participants