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

add page for PX4 ROS library #2740

Merged
merged 20 commits into from Nov 23, 2023
Merged

add page for PX4 ROS library #2740

merged 20 commits into from Nov 23, 2023

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Sep 20, 2023

Documentation for PX4/PX4-Autopilot#20707.

Some bits are still missing & it needs integration with the other pages.

@hamishwillee if you want you can already take a first pass. @tstastny will go through the steps to see if anything is missing.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Sep 21, 2023

@bkueng Housekeeping

  1. I've done some formatting changes on this to my taste.
    • Ran prettier and did my own normal layout changes. FYI now using VSCode and prettier to format by default - makes things a little less arbitrary.
    • Moved doc to a /ros2/ sub folder. I'm intending to separate/move ROS2 sources into own folder over time, but might as well start with this. It just makes things a little easier to manage.
    • Images are shrunk using lossless compressor and all grouped in same ros2 folder. Again, easier to manage.
    • Took the typo error (interpretted) into main and rebased
  2. Ignore the git bot flaw checker - it thinks some of the output logging is a reference link (a bug). I'll fix that eventually.
  3. @beniaminopozzan Should review this doc as well when it is done.
  4. As an aside, can we also have a separate additions on PX4 modes outside of ROS2? I can probably do this if I know:

@hamishwillee
Copy link
Collaborator

hamishwillee commented Sep 21, 2023

Top level comments on this feature.

  1. It's pretty amazing - but then you know that.
  2. I hate the name: "PX4 SDK" - that's an SDK containing tools and libraries for developing PX4 modules. This is the "PX4 ROS SDK" or "PX4 ROS2 SDK" - a library for developing PX4 ROS2 applications.
  3. My criteria for success on a doc is "Do I think I might be able to do this task". I don't think this is far off, though we could do a lot more.
  4. It's quite monolithic. We might consider splitting it when the docs are complete.
  5. The tutorial is not a tutorial - it's an overview. I think a tutorial might be very useful for this particular case as well - perhaps adding a custom smart shot a'la solo - but you could make it much simpler than that.
    • Note we don't have that many tutorials in the docs because they create duplication and require additional maintenance. But inherently they serve a useful purpose in showing someone who doesn't know anything the things that we think are important to them. This is perhaps a case where an exception could be made.
  6. There is some versioning here about QGC daily - is there a build we know that will have this coming up for PX4 v1.14. We need such a build anyway, so if we know what it will be, good to list it.
  7. This suggests taking PX4 main. I believe that the ROS2 integration intends to version by PX4 release. Upshot:
    • Good to mention the compatibility requirement for main is "right now" but when this is out in PX4 release you'll take the PX4 messages for that release.
    • How will you manage compatibility for the SDK depending on ROS? Are there topics that can't be modified in PX4 because of this?
    • How will you manage compatibility for users of the SDK? Are we saying that this is stable by PX4 release, or we'll maintain backwards compat?
  8. This currently doesn't explain how you can set the name of your mode and translate it, and any text that might exist in QGC for it. This might be because at this point it is only the English key from the mode name?
  9. Perhaps irrelevant, and perhaps already covered, but consider I have a bunch of different smart shot modes that I want to add to my drone, and I also want to hook in collision avoidance. Do we need to do anything special to bundle and install all of these together on the companion? I.e. is there any special registration stuff and auto startup stuff that we can/should link to for "the big picture"?

I'll add some other comments/suggestions in line.

@@ -0,0 +1,336 @@
# PX4 SDK
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name :-(

en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved

#### Mode

- A mode is a component that can send setpoints to the vehicle, which control its motion (such as velocity or direct actuator commands).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to list the setpoints somewhere below and link to them.

en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
## Mode Tutorial

This section steps through an example for a custom mode class.
For a complete application, check out the [examples in the repository](https://github.com/PX4/px4_sdk/tree/main/examples/cpp).
Copy link
Collaborator

Choose a reason for hiding this comment

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

en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
Comment on lines 84 to 85
To ensure compatibility, use the latest _main_ branches for PX4, _px4_msgs_ and the SDK.
See also [here](https://github.com/PX4/px4_sdk#compatibility-with-px4).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes me uncomfortable - it basically says we ensure compatibility of the SDK by forcing you to use an unstable version of PX4. The linked doc is better in that it says we might change this in future. We should come up with a plan now and let people know.

The linked doc mentions this, which should also appear in this doc (I haven't checked) but it is cool:

The SDK checks for message compatibility on startup when registering a mode. ALL_PX4_SDK_MESSAGES defines the set of checked messages. If you use other messages, you can check them using:

if (!px4_sdk::messageCompatibilityCheck(node, {{"/fmu/in/vehicle_rates_setpoint"}})) {
  throw std::runtime_error("Messages incompatible");
}

en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved

1. [Configure the output](../payloads/#generic-actuator-control-with-mavlink)
2. Create an instance of [px4_sdk::OffboardActuatorControls](https://github.com/PX4/px4_sdk/blob/main/px4_sdk_cpp/include/px4_sdk/control/offboard_actuators.hpp) in the constructor of your mode
3. Call the `set` method to control the actuator(s). This can be done independently of any active setpoints.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code fragment or link to code showing last two points would be awesome.

en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
en/ros2/px4_sdk.md Outdated Show resolved Hide resolved
};
```

- `[1]`: First we create a class that inherits from `px4_ros2::ModeBase`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we could create a "reference" of the public API - just names and a description for the main classes at the end. Starting from a reference tells you what documentation is available/missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we could use doxygen

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am going to assume you have this on your list :-)

The following sections provide a list of commonly used setpoint types.
You can also add your own type by adding a class that inherits from `px4_ros2::SetpointBase`, sets the configuration flags according to what the setpoint requires, and then publishes any topic containing a setpoint.

<!--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Todo

@hamishwillee
Copy link
Collaborator

@bkueng I added some comments. I'm not sure how far this has to go before we decide it is "right". Some things I'd like considered before we merge:

  • Can you add a small section in https://docs.px4.io/main/en/releases/main.html for this. Essentially I'm trying to generate our release notes in advance of the final release. At least including the main features and autopilot PRs.
  • Can we add the video from the dev summit
  • This seems vehicle agnostic. Is it? I.e. can I expect this to work just as well on plane, and in similar ways? For example if my mode executor specifies takeoff will the system use the takeoff for plane on plane and mc on mc? Or to put it another way - can we have a section "Support for different vehicle types" that summarises what you have to think about. Ultimately we'd like examples for each.
  • It would be good to have API reference at top level for the public API - classes, topics, setpoint types etc. That way even if we don't document them all, people can infer their purpose and we can expand out over time.

You might not want to do any of this right now. That's OK if you think this is useful as is and have a cunning plan going forward?

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
@bkueng
Copy link
Member Author

bkueng commented Nov 22, 2023

Can you add a small section in https://docs.px4.io/main/en/releases/main.html for this. Essentially I'm trying to generate our release notes in advance of the final release. At least including the main features and autopilot PRs.

Will do.

Can we add the video from the dev summit

Yes later. It's not up yet and I wanted to add the link to the docs to the video first.

This seems vehicle agnostic. Is it? I.e. can I expect this to work just as well on plane, and in similar ways? For example if my mode executor specifies takeoff will the system use the takeoff for plane on plane and mc on mc? Or to put it another way - can we have a section "Support for different vehicle types" that summarises what you have to think about. Ultimately we'd like examples for each.

Yes agnostic, but we'll have to see how much we can abstract a mode to be generic. I don't think they will be completely generic.

You might not want to do any of this right now. That's OK if you think this is useful as is and have a cunning plan going forward?

Cunning certainly goes too far, but yes, there's a lot of things to be done.

Comment on lines +60 to +62
- [Experimental] [PX4 ROS 2 Interface Library](../ros2/px4_ros2_interface_lib.md): A new C++ library that simplifies controlling PX4 from ROS 2.
Supports adding flight modes in ROS 2 that are peers of the PX4 modes running on the flight controller.
Added to PX4 in [PX4-Autopilot#20707](https://github.com/PX4/PX4-Autopilot/pull/20707) (initial support).
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI the important thing here is to set expectation. So I have put a prefix experimental and added a link to the initial PR.
As you enhance this you can add new PR links.
If you consider significant parts of this stable, we can remove "Experimental".

Comment on lines 3 to 13
:::warning Experimental
At time of writing parts of the PX4 ROS 2 Interface Library is experimental, and hence subject to change:

- The architecture and core interfaces for defining modes in ROS 2 modes are largely stable.
The library offers significant benefits over using offboard mode in its current state.
- Only a few setpoint types have settled (the others are still under development).
You may need to use internal PX4 topics which may not remain backwards-compatible over time.
- The API is not fully documented.
- Testing in CI is still limited.

:::
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI I added section "Experimental" up the top. What I'm trying to do here is explain that the mode stuff works now and already gives you a benefit over using offboard mode, but that many of the abstractions we provide for working with ROS 2 do not exist yet. So you can use it but likely things will change.

I've done it in bullets so you can extend easily if you want. I'm open to any text which sets the level of expectation appropriately (i.e. this is "for discussion").

Comment on lines +374 to +382
Telemetry topics include:

- [OdometryGlobalPosition](https://github.com/Auterion/px4-ros2-interface-lib/blob/main/px4_ros2_cpp/include/px4_ros2/odometry/global_position.hpp): Global position
- [OdometryLocalPosition](https://github.com/Auterion/px4-ros2-interface-lib/blob/main/px4_ros2_cpp/include/px4_ros2/odometry/local_position.hpp): Local position, velocity and acceleration.

:::note
These topics provide a wrapper around the internal PX4 topics, allowing the library to maintain compatibility if the internal topics change.
Check [px4_ros2/odometry](https://github.com/Auterion/px4-ros2-interface-lib/tree/main/px4_ros2_cpp/include/px4_ros2/odometry) for new topics, and of course you can use any ROS 2 topic published from PX4.
:::
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs checking. I wanted to add a little information on what we support now at this level, and a reason why we bother.

@hamishwillee
Copy link
Collaborator

@bkueng I've addressed most of my own concerns with this - setting expectation - by adding an experimental header. Can you please check that is OK. Perhaps there is a better way of explaining the state of the SDK.

The only one thing missing that I think might block people from using this is this question on the mode executor: https://github.com/PX4/PX4-user_guide/pull/2740/files#r1402837201

I'll merge tomorrow if you think this is reasonable.

- Only a few setpoint types have settled (the others are still under development).
You may need to use internal PX4 topics which may not remain backwards-compatible over time.
- The API is not fully documented.
- Testing in CI is still limited.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is ok, but just fyi most of the things that are there are tested. Mostly the setpoint types (with their expected behavior) that are not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's say that :-)

@bkueng
Copy link
Member Author

bkueng commented Nov 23, 2023

Thanks Hamish. Looks good to me.

Copy link

TODO: Error duplicate reference to print
TODO: Error duplicate reference to print
TODO: Error duplicate reference to print
TODO: Error duplicate reference to print
TODO: Error duplicate reference to print
TODO: Error duplicate reference to print
TODO: Error duplicate reference to print

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

OK @bkueng made a few minor changes to address your responses to my feedback. I'm going to merge. I wish we could do a lot more, but good to get this out there and visible.

@hamishwillee hamishwillee merged commit 3ca0a59 into main Nov 23, 2023
2 checks passed
@hamishwillee hamishwillee deleted the px4_sdk branch November 23, 2023 21:19
@hamishwillee hamishwillee restored the px4_sdk branch November 23, 2023 21:20
@hamishwillee hamishwillee deleted the px4_sdk branch November 23, 2023 22:13
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.

None yet

4 participants