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_Arming: Defined by the process used #25207

Closed

Conversation

muramura
Copy link
Contributor

@muramura muramura commented Oct 8, 2023

The array of strings is far from the actual processing used.
Define this array in the process that will use it.

AFTER
Screenshot from 2023-10-08 15-20-19

@peterbarker
Copy link
Contributor

@muramura you're right, but the definition is right next to the definition of the array we are iterating over.

This is purposeful, as you really don't want these arrays to be of different length! Note there's no assertion these are of the same length anywhere, so if you modify one you really do need to modify the other. Having them close together in the code helps that.

@khancyr
Copy link
Contributor

khancyr commented Oct 9, 2023

adding a constant to remove the magic number would solve the size issue !

Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

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

This seems less useful/more confusing. The only thing I'd actually really support here is a static assert that the arrays are the same length.

@magicrub
Copy link
Contributor

magicrub commented Oct 9, 2023

I prefer it defined up where it is now but having all 3 using a define like @khancyr said is the better way to go.

@peterbarker
Copy link
Contributor

Doesn't look like a lot of love for this rearrangement.

https://github.com/ArduPilot/ardupilot/pull/25207/files makes this no longer required. (we stop using indexes, rather a little structure to relate the channel with the name)

Closing this one - thanks anyway!

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.

5 participants