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.

Sign up for GitHub

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

Jump to bottom

AP_DDS: Copter takeoff service #26911

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

AP_DDS: Copter takeoff service #26911

wants to merge 4 commits into from
+273 −9

Conversation

snktshrma
Copy link
Contributor

Added a service for copter to accept takeoff commands using native DDS support.

@snktshrma snktshrma force-pushed the dds-copter-arming branch 3 times, most recently from 7e8d6ef to 1d552af Compare April 28, 2024 21:17
@Ryanf55 Ryanf55 added the ROS label Apr 28, 2024
@Ryanf55 Ryanf55 self-assigned this Apr 28, 2024
@Ryanf55 Ryanf55 self-requested a review April 28, 2024 23:58
Ryanf55
Ryanf55 requested changes Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Can you update libraries/AP_DDS/README.md for the new service channel?
Also, if you can, is it possible to add a test for it?

Tools/ros2/ardupilot_msgs/srv/Takeoff.srv Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/Takeoff.srv Outdated Show resolved Hide resolved
@snktshrma
Copy link
Contributor Author

Looks pretty good.

Can you update libraries/AP_DDS/README.md for the new service channel? Also, if you can, is it possible to add a test for it?

Alright! I'll add a test for it

@snktshrma snktshrma force-pushed the dds-copter-arming branch 2 times, most recently from 1c47c93 to 433091c Compare April 29, 2024 09:04
@snktshrma
Copy link
Contributor Author

snktshrma commented Apr 29, 2024 •
edited
Loading

Looks pretty good.

Can you update libraries/AP_DDS/README.md for the new service channel? Also, if you can, is it possible to add a test for it?

I have added the test @Ryanf55

@snktshrma snktshrma requested a review from Ryanf55 April 29, 2024 10:33
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Apr 29, 2024

Can you fix the build errors?
https://github.com/ArduPilot/ardupilot/actions/runs/8877225483/job/24370361196?pr=26911#step:7:2931

@snktshrma snktshrma force-pushed the dds-copter-arming branch 2 times, most recently from cd9a217 to abe32b2 Compare April 30, 2024 05:46
@snktshrma
Copy link
Contributor Author

Can you fix the build errors? https://github.com/ArduPilot/ardupilot/actions/runs/8877225483/job/24370361196?pr=26911#step:7:2931

This build error is fixed @Ryanf55

@rmackay9 rmackay9 mentioned this pull request Apr 30, 2024
Merged
@Ryanf55 Ryanf55 added this to the DDS 4.6 milestone May 1, 2024
@srmainwaring
Copy link
Contributor

@snktshrma, @Ryanf55 - while working on support for AP in Aerostack2 I examined the PX4 msgs and services for comparison (PX4 added a service interface at the end of last year for vehicle commands). The service request message structure is similar to the mavlink equivalent: https://github.com/PX4/px4_msgs/blob/main/msg/VehicleCommand.msg. Wondering whether we want to use something similar rather than add many separate messages for the various commands?

@snktshrma
Copy link
Contributor Author

snktshrma commented Jun 22, 2024 •
edited
Loading

hi @srmainwaring !
That seems a feasible idea. It'll be easier for users also to have command of all the required functions.
Though I think there are some persisting issues with DDS like float issue, which needs to be solved alongside.

@snktshrma snktshrma mentioned this pull request Jun 23, 2024
Merged
@snktshrma snktshrma force-pushed the dds-copter-arming branch 3 times, most recently from 3412aab to a7a3bb9 Compare September 24, 2024 03:47
Ryanf55
Ryanf55 reviewed Sep 24, 2024
View reviewed changes
ArduCopter/Copter.h Outdated Show resolved Hide resolved
Ryanf55
Ryanf55 requested changes Sep 24, 2024
View reviewed changes
libraries/AP_DDS/README.md Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/Takeoff.srv Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/Takeoff.srv Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_dds_tests/ardupilot_dds_tests/copter_takeoff.py Outdated Show resolved Hide resolved
@snktshrma snktshrma force-pushed the dds-copter-arming branch 2 times, most recently from 69ac7e8 to 4ebbf38 Compare September 24, 2024 17:34
Ryanf55
Ryanf55 approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

Looks good, tested in SITL and works with floats now.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Sep 24, 2024

Hi Randy, can you check the copter changes here are good? I tested in SITL, all looks good.

@Ryanf55 Ryanf55 mentioned this pull request Oct 8, 2024
Closed
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 8, 2024

Hey there, sorry this didn't get in earlier. Can you fix conflicts? I'm happy with the PR.

@snktshrma
Copy link
Contributor Author

Hi @Ryanf55 ! I have fixed the conflicts.

@snktshrma snktshrma requested a review from Ryanf55 October 8, 2024 03:41
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 8, 2024 •
edited
Loading

Awesome, thanks for the quick response. If you can attend either of the dev calls, please request a review of the changes.
Make sure to fix your commits and split them by subsytem.
image

IE, you should have unique commits for

  • ArduCopter:
  • `Tools: ros2:
  • AP_DDS:
  • AP_Vehicle

@snktshrma
Copy link
Contributor Author

Got it @Ryanf55 ! I'll try to attend the dev call.
Rest, I'll split the commits as you mentioned.
Thanks

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 14, 2024

The size tests are failing. It might have a conflict, and need a rebase.
https://github.com/ArduPilot/ardupilot/actions/runs/11228146544/job/31211539090?pr=26911#step:9:108

@snktshrma snktshrma force-pushed the dds-copter-arming branch 2 times, most recently from a81f72c to 71bfa83 Compare October 15, 2024 07:52
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 19, 2024 •
edited
Loading

Looks good. The reason the diff looks bad is because you reordered two methods. This introduces noise in the diff.
image

I'm going to force push to put these back to original, which will make it quicker to review/merge.
image

Hopefully during the conference this week.

@Ryanf55 Ryanf55 mentioned this pull request Oct 20, 2024
Draft
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 29, 2024

@snktshrma Now that all the services are wrapped in ifdefs, can you rebase and add that for this PR?
AP_DDS_COPTER_TAKEOFF_SERVICE or something similar could be a good name for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ryanf55 Ryanf55 Ryanf55 approved these changes

@rmackay9 rmackay9 Awaiting requested review from rmackay9

Assignees

@Ryanf55 Ryanf55

Labels
ROS
Projects
DDS/ROS2
Status: 👀 In review
Milestone
DDS 4.6
Development

Successfully merging this pull request may close these issues.
4 participants
@snktshrma @Ryanf55 @srmainwaring @CraigElder