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

Rename vehicle-specific RC_Channel and GCS_MAVLink files #28934

Merged

Conversation

peterbarker
Copy link
Contributor

@peterbarker peterbarker commented Dec 23, 2024

We are somewhat inconsistent with our file naming for these derived classes.

So, for example, we have GCS_Plane and GCS_Copter which contain the classes GCS_Plane and GCS_Copter, but we use GCS_Mavlink.h for both Plane and Copter to hold GCS_MAVLINK_Plane and GCS_MAVLINK_Copter.

This PR renames the RC_Channel and GCS_MAVLink files for the vehicles to be closer to the class names they contain. The class name defines in GCS_MAVLink_Plane.cpp, however, is still GCS_MAVLINK_Plane rather than GCS_MAVLink_Plane. There are 1500 lines to change to correct the case, and that's going to make backports difficult. Probably something to do closer to the middle of a release cycle.

@tpwrules
Copy link
Contributor

tpwrules commented Dec 23, 2024

I like this. It would be nice to interleave the commits to avoid so much of a bisect pit (i.e. do each vehicle's commit in sequence rather than two blocks for all vehicles).

I assume the copter case would also require the same order of magnitude of changes?

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.

The renaming of files will make backports tricky but besides that the changes themselves are pretty small, thanks!

@peterbarker peterbarker force-pushed the pr/rename-files-to-match-classes branch from 0c97598 to 2290611 Compare December 23, 2024 04:50
@peterbarker
Copy link
Contributor Author

I like this. It would be nice to interleave the commits to avoid so much of a bisect pit (i.e. do each vehicle's commit in sequence rather than two blocks for all vehicles).

Good point, I've interleaved the commits now.

I assume the copter case would also require the same order of magnitude of changes?

Not sure what you mean here.

The "1500" was a git grep 'GCS_MAVLINK' in the base directory of the checkout. That gets the base class and its derivatives.

@tpwrules
Copy link
Contributor

I assume the copter case would also require the same order of magnitude of changes?

Not sure what you mean here.

You mentioned only Plane in the context of that 1500 number. It looks like the problem affects both Plane and Copter so that number is for both? Not quibbling about the count, just want to accurately know what vehicles would be affected.

@peterbarker
Copy link
Contributor Author

I assume the copter case would also require the same order of magnitude of changes?

Not sure what you mean here.

You mentioned only Plane in the context of that 1500 number. It looks like the problem affects both Plane and Copter so that number is for both? Not quibbling about the count, just want to accurately know what vehicles would be affected.

The entire codebase.

Primarily 14 files containing the 7 classes, but there are going to be statics floating around, probably.

@peterbarker peterbarker force-pushed the pr/rename-files-to-match-classes branch from 2290611 to c42258b Compare December 24, 2024 23:24
@peterbarker peterbarker merged commit 42f1aea into ArduPilot:master Dec 26, 2024
97 checks passed
@peterbarker peterbarker deleted the pr/rename-files-to-match-classes branch December 26, 2024 21:53
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.

5 participants