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_Compass: move param to table per instance #19123

Merged
merged 20 commits into from
Nov 29, 2021

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Nov 2, 2021

I was looking at changing COMPASS_MOTCT for battery instances and decided that the whole compass param tree is a mess. So I have converted to a per instance table.

The names have changes a little since the number has to come before the suffix.

COMPASS_ORIENT changed to COMPASS1_ORIENT
COMPASS_ORIENT2 changed to COMPASS2_ORIENT
ect....

Will need some GCS change to look for the new param names, I do think the naming is a improvement. COMPASS1_ now gets you all compass 1 params rather than having to try and pick them out of the list maunaly.

COMPASS_ENABLE has also been moved to the top of the table and been given a enable flag.

Once this is in I would like to move COMPASS_MOTCT to the new instance param so we can compensate different compasses for different things.

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 2, 2021

This one actually does save flash,

Binary           text             data         bss            total
---------------  ---------------  -----------  -------------  ---------------
arduplane        -104 (-0.0107%)  0 (0.0000%)  0 (0.0000%)    -104 (-0.0095%)
antennatracker   -112 (-0.0171%)  0 (0.0000%)  0 (0.0000%)    -112 (-0.0142%)
arducopter-heli  -92 (-0.0100%)   0 (0.0000%)  4 (+0.0031%)   -88 (-0.0084%)
arducopter       -72 (-0.0077%)   0 (0.0000%)  0 (0.0000%)    -72 (-0.0068%)
ardurover        -80 (-0.0096%)   0 (0.0000%)  0 (0.0000%)    -80 (-0.0083%)
blimp            -96 (-0.0142%)   0 (0.0000%)  0 (0.0000%)    -96 (-0.0119%)
ardusub          -100 (-0.0125%)  0 (0.0000%)  -4 (-0.0031%)  -104 (-0.0111%)

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 2, 2021

I should probably add back the param descriptions for the old param names.

@peterbarker
Copy link
Contributor

Need @rmackay9 to weigh in here (and maybe @meee1 )

This is going to cause a great deal of issues for MissionPlanner - dependence on current parameter names.

May also cause issues with the priority table if we take this opportunity to move from COMPASS_*1 to MAG1_

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.

I like the change so I've approved it but I worry we will regret the impact on users.

Let's see what @meee1 says about the required MP changes before we merge this.

@davidbuzz davidbuzz requested a review from meee1 November 8, 2021 23:08
@CraigElder CraigElder assigned meee1 and unassigned meee1 Nov 8, 2021
@IamPete1
Copy link
Member Author

@meee1 How much effort is it for mission planner to deal with these changes?

@tridge
Copy link
Contributor

tridge commented Nov 15, 2021

need to ask @meee1 to do the MissionPlanner changes before we merge this. Also need to test with QGC. I think we should also check with UGCS

@IamPete1
Copy link
Member Author

Mission Planner Beta now supports this. QGCs does not, but these are the options we have:
image
Since it does not do priority and ordering at all I don't think it really makes any difference.

@tridge
Copy link
Contributor

tridge commented Nov 24, 2021

i'll wait for @rmackay9 to be on a dev call for final decision

@tridge tridge removed the DevCallEU label Nov 24, 2021
@rmackay9
Copy link
Contributor

Sorry I missed the call, it's a bit borderline but I"m ok with merging this.

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

Successfully merging this pull request may close these issues.

None yet

6 participants