Skip to content

Refactor crazyswarm cpp#781

Open
WillWu88 wants to merge 1 commit intoIMRCLab:mainfrom
USC-ACTLab:header-refactor
Open

Refactor crazyswarm cpp#781
WillWu88 wants to merge 1 commit intoIMRCLab:mainfrom
USC-ACTLab:header-refactor

Conversation

@WillWu88
Copy link

@WillWu88 WillWu88 commented Nov 5, 2025

This major refactor:

  • separates the original implementation into proper header/source files
  • lifts implementations of CrazyflieROS and CrazyflieServer into a separate library
  • adds missing rcl_interfaces dependency to package
  • moves inline functions into anonymous namespace for translation-unit level access; this supports C++ best practice for local functions
  • reformats code with google C++ format style; add clang format file

Since I am linking crazyflie_cpp library to the new crazyflie_ros2 library, CMakeLists.txt from crazyflie_cpp needs to include an additional line:

set_target_properties(crazyflie_cpp PROPERTIES POSITION_INDEPENDENT_CODE TRUE)

Please advise on:

  • testing
  • best formatting style moving forward (or if the current clang format options are fine)

This major refactor:
- separates the original implementation into proper header/source files
- lifts implementations of CrazyflieROS and CrazyflieServer into a
  separate library
- adds missing `rcl_interfaces` dependency to package
- reformats code with google C++ format style; add clang format file
@WillWu88
Copy link
Author

WillWu88 commented Nov 5, 2025

@knmcguire for vis. I don't have permission to add reviewers.

@whoenig
Copy link

whoenig commented Nov 5, 2025

Thanks! Could you perhaps comment a bit on what you try to achieve?

  • I can see that a formatting style can be helpful, although I feel that generally this is only useful if there is a "global" standard (like pep8 in Python or rustfmt) or the project at hand is very big.
  • I am not sure about header/cpp separation. It can improve compile times, but also leads to annoying code duplication in C++.
  • What's the point of the separate library - would there ever be another "user"?

@WillWu88
Copy link
Author

WillWu88 commented Nov 5, 2025

Thank you for taking a look! To your points:

  • The formatting style started as a personal preference, so can be omitted from the PR. I did it to improve readability, but I also recognize that this can be highly personal
  • Header/cpp separation was done for readability and modularity. crazyflie_ros is archived, and seems like a lot of functionalities are now fulfilled by CrazyflieROS. It also is a huge hassle to read and work with the code base when everything is in one giant file.
    • As far as I was aware of the only duplicating code across the refactor is the type aliases for the service messages, and I believe there would be ways to avoid this duplication, unless I am missing something here?
  • Separate library was made so future nodes may reuse CrayzflieROS or CrazyflieServer. Since there is only one node we can omit this and just add all source files to the executable target.

Please let me know if I am missing some context here.

@knmcguire knmcguire requested a review from whoenig November 6, 2025 08:39
@knmcguire
Copy link
Collaborator

I've added wolfgang as reviewer as the cpp backend is his turf. However, such a big potentially breaking change to the cpp backend will require some discussion on the maintenance team though. So we will need to come back to you on that in the next couple of weeks with suggestion of approach.

@WillWu88
Copy link
Author

WillWu88 commented Nov 6, 2025

Yeah absolutely! Please let me know if I can help with anything. Appreciate your time.

@whoenig
Copy link

whoenig commented Feb 12, 2026

Sorry for the long delay! I had some discussion with @knmcguire with the following outcome:

We generally feel that the PR is too big, especially given that it's a single commit. A big refactor like that would require pre-approval and then either multiple small PRs, or a big PR where the logical changes are split in separate commits.

I see little benefit in separate libraries as this limits compiler/linker optimizations and there is no use-case in using these libraries in a second project executable. There might be a small benefit in terms of compile times, but those haven't been a real pain point.

Clang format and the missing dependencies do make sense. I'll leave this PR open until we have at least added the missing dependency. A new PR with smaller changes (e.g., clang code formatting) would be welcome.

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.

3 participants