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: set Heli frame default at compile-time #11707

Merged
merged 2 commits into from
Jul 4, 2019

Conversation

peterbarker
Copy link
Contributor

This should avoid us sending heartbeats out with the incorrect frame type.

Our run-time defaulting of the frame happened really, really late in the boot process - no wonder we were sending multicopter....

Note that this is very much a functional change. This allows someone to explicitly set their frame class to any value and have that persist across reboots. But then, they could already set it to heli-dual or heli-quad and have that persist. We could add a pre-arm check that the frame class chosen makes sense for this build (would also save the multicopter users who have heli aspirations and decide to start with setting frame_class).

OTOH. Better to unify the builds anyway.

This was tested by flashing onto PixHawk. Note that since FRAME_CLASS is usually explicitly set, you do need to reset the stored parameters to test this.

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 3, 2019

Great! thanks for this! So I guess the potential issue we might see is users moving from multicopter to Heli firmware who have set the FRAME_CLASS parameter already. These users will now need to explicitly set FRAME_CLASS = 7 while previously it would have been set for them. I think we may already have a pre-arm check.. we certainly do when moving the other way (i.e. Heli to Multicopter)

@peterbarker
Copy link
Contributor Author

@rmackay9 Yes, absolutely - except it's frame class 6: https://github.com/ardupilot/ardupilot/blob/master/libraries/AP_Motors/AP_Motors_Class.h#L39

@magicrub
Copy link
Contributor

magicrub commented Jul 3, 2019

This is a much better solution than mine. Thank you!

@peterbarker
Copy link
Contributor Author

Added a pair of prearm checks.

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 3, 2019

I wonder if these prearm checks should instead go into the Motor library's parameter_check() function.

@peterbarker
Copy link
Contributor Author

@rmackay9 I did consider that - but I'd need to make the parameters-check a virtual function on the base motors class library then (so we can do the multicopter check in the appropriate place). Seemed a bit much for a sanity check we should do away with ;-)

The other thing is that we allocate the motors backend based on this parameter. Going and doing the sanity check in that object we've just allocated (IOW, that we just did something sensible) doesn't seem entirely right to me. A more-correct fix along those lines would be to fail in allocate_motors - but the only fail path we have in there is panic() - so that's a chunk of work to do nicely.

@rmackay9 rmackay9 merged commit bf6f10e into ArduPilot:master Jul 4, 2019
@rmackay9
Copy link
Contributor

rmackay9 commented Jul 4, 2019

OK, merged. OK, yes, so it's probably the allocate motors are that we should have the check. anyway, something for the future maybe.

@peterbarker peterbarker deleted the pr/heli-frame-fix branch July 4, 2019 01:06
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

3 participants