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

AP_RPM: Fix to SITL RPM driver instance #12909

Merged
merged 1 commit into from Dec 9, 2019

Conversation

@Gone4Dirt
Copy link
Contributor

Gone4Dirt commented Nov 27, 2019

This PR resolves this #issue:
#12891

This image demonstrates the issue:
image

I believe that because the nullptr check was added:

if (drivers[i] == nullptr) {
drivers[i] = new AP_RPM_SITL(*this, i, state[i]);
}

It prevented the pointer to the SITL driver from being created as the RPM pin driver was already created. Added ifdef statement to prevent the RPM_pin drivers from being allocated when using SITL.

I have tested this fix in SITL. I have not tested on a board.

@Gone4Dirt Gone4Dirt force-pushed the Gone4Dirt:SITL_RPM_Pin_Patch branch from 34049f6 to 5791b04 Nov 27, 2019
@bnsgeyer bnsgeyer requested a review from tridge Nov 28, 2019
Copy link
Contributor

tridge left a comment

need EFI in SITL

if (type == RPM_TYPE_PWM) {
// PWM option same as PIN option, for upgrade
type = RPM_TYPE_PIN;
}
if (type == RPM_TYPE_PIN) {
drivers[i] = new AP_RPM_Pin(*this, i, state[i]);
}
#if EFI_ENABLED

#if EFI_ENABLED

This comment has been minimized.

Copy link
@tridge

tridge Dec 3, 2019

Contributor

this breaks EFI in SITL

This comment has been minimized.

Copy link
@Gone4Dirt

Gone4Dirt Dec 3, 2019

Author Contributor

I have now moved the EFI driver instantiation outside of the check for HAL_BOARD_SITL.

This comment has been minimized.

Copy link
@tridge

tridge Dec 3, 2019

Contributor

that still doesn't look right, as it will allocate AP_RPM_SITL, then overwrite it with AP_RPM_EFI object?

This comment has been minimized.

Copy link
@Gone4Dirt

Gone4Dirt Dec 8, 2019

Author Contributor

Ah ok, sorry didn't see that. I have now changed it again. I think it is correct now.

@CraigElder CraigElder removed the DevCallTopic label Dec 3, 2019
@Gone4Dirt Gone4Dirt force-pushed the Gone4Dirt:SITL_RPM_Pin_Patch branch from 5791b04 to ce1e876 Dec 3, 2019
@Gone4Dirt

This comment has been minimized.

Copy link
Contributor Author

Gone4Dirt commented Dec 3, 2019

I have moved EFI driver instantiation out of the check for the SITL board. Rebased. Tested in SITL.

@Gone4Dirt Gone4Dirt force-pushed the Gone4Dirt:SITL_RPM_Pin_Patch branch from ce1e876 to 75bf911 Dec 8, 2019
@tridge
tridge approved these changes Dec 9, 2019
@tridge tridge merged commit 5f11afd into ArduPilot:master Dec 9, 2019
4 checks passed
4 checks passed
ArduPilot.ardupilot #20191208.18 succeeded
Details
ArduPilot.ardupilot (Cygwin SITL build) Cygwin SITL build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.