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 offboard mode switch module for simplifying offboard for ros2 #22530

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

Conversation

Jaeyoung-Lim
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim commented Dec 12, 2023

Solved Problem

When using offboard mode with ros2, the sender needs to be careful to not publish the setpoints while not in offboard mode.

This can be achieved typically by subscribing to a vehicle status message, and publishing the setpoints only when the vehicle enters offboard mode.

However, if the person implementing the offboard mode is not responsible, this may be prone to errors by intefering with other controllers.

Fixes #22512

@ayhamalharbat FYI

Solution

  • Separate the offboard topic into a dedicated topic.
  • Remove the need for publishing offboard control mode separately

Changelog Entry

For release notes:

Bugfix Separate offboard setpoints

Alternatives

We could also keep the interface transparent, and require the ros2 node to be responsible for making sure that it does not interfere with other topics

Test coverage

Tested in Gazebo, with gz_omnicopter

This commit adds a offboard mode switch module to enable using dedicated topics for offboard mode
@beniaminopozzan
Copy link
Member

@Jaeyoung-Lim The idea here is that the new module monitors the offboard_* topic, and if they are valid then it publishes the offboard_control_mode topic and only when the vehicle enter OFFBOARD mode then the offboard_* topics republished standard setpoints? Did I get it right?

@Jaeyoung-Lim
Copy link
Member Author

Jaeyoung-Lim commented Dec 17, 2023

@beniaminopozzan Yes!

@beniaminopozzan
Copy link
Member

Great, is there a reason for having only the logic for trajectory, attitude and rate setpoint? what about thrust and torque and direct?

@Jaeyoung-Lim
Copy link
Member Author

about thrust and torque and direct?

@beniaminopozzan No real reason, just wasnt able to test with it yet

One thing missing from this is sanity checking the combination of setpoints (e.g. you cant set thrust and torque with rates at the same time)

@mrpollo
Copy link
Contributor

mrpollo commented Jan 30, 2024

@MaEtUgR we are still waiting on your review!

Copy link
Member

@beniaminopozzan beniaminopozzan left a comment

Choose a reason for hiding this comment

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

The is no offboard_thrust_setpoint, offboard_torque_setpoint and offboard_actuator_motor_setpoint yet
I suggest to mark the PR as draft until it is ready

@Jaeyoung-Lim
Copy link
Member Author

Jaeyoung-Lim commented Jan 30, 2024

@beniaminopozzan I have intentionally left them out, since I think for those I think it might be just better to publish directly to the actuators due to the time delay introduced by publishing/republishing.

I also wouldnt know how to test it if we have the interface other than SITL, which is not representative of doing this on hardware.

If we intend to add such interface, I would do it in a separate PR

@beniaminopozzan
Copy link
Member

@Jaeyoung-Lim oh I see, that makes sense unless the expected increase in latency is proven wrong by real hardware tests.

Nevertheless, this further increase the complexity of the offboard interface, now users have to keep in mind that some offboard setpoints have to be paired with the offboardControlMode messages while other setpoints don't.

Wouldn't it be cleaner at this point, instead of having a centralized offboard mode swither, to distribute this on every module that uses the "setpoint" messages?
Something like: if mc_att_control subcribes to vehicle_attitude_setpoints, then it will subscribe to offboard_attitude_setpoint too. Internally it will use offboard_attitude_setpoint instead of vehicle_attitude_setpoints if vehicle_control_mode says offboard + attitude.

Conficting modes situations will be addressed at the single controller level, which will check VehicleControlMode for knowing which setpoint is shall use

@Jaeyoung-Lim
Copy link
Member Author

Jaeyoung-Lim commented Feb 5, 2024

Wouldn't it be cleaner at this point, instead of having a centralized offboard mode swither, to distribute this on every module that uses the "setpoint" messages?
Something like: if mc_att_control subcribes to vehicle_attitude_setpoints, then it will subscribe to offboard_attitude_setpoint too. Internally it will use offboard_attitude_setpoint instead of vehicle_attitude_setpoints if vehicle_control_mode says offboard + attitude.

I have been trying to avoid this, as by doing this we would create a new "flight mode" for offboard mode and all controller modules need to be aware of the "offboard" interface. For me the value of offboard mode is that the control modules do not know whether the vehicle is in offboard mode and therefore we can test individual structures by simply turning on and off different levels of control handled by commander. If somebody is implementing a new controller, they do not need to implement a special "offboard" interface to test it.

Regarding completeness, I agree with you that it would be nice to support direct actuator setpoints. However, this PR solves a problem where a lot of people are struggling to use the offboard interface due to the "handshaking" behavior it requires(need to monitor vehicle flight mode before publishing the setpoints). The people struggling to do this are rarely people using direct actuator inputs.

On the other hand, I do not have the capacity to test the direct actuator setpoint interface for this PR. So if anyone would be interested in contributing it would be great. But otherwise, it will be hard to include it as part of this PR.

Note that this PR does not interfere/replace the previous way of engaging offboard mode. It just adds a simplified interface to test it.

@hamishwillee hamishwillee added the Admin: Documentation needed PRs that requires Documentation updates label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin: Documentation needed PRs that requires Documentation updates
Projects
Status: 👀 In review/test
Development

Successfully merging this pull request may close these issues.

[Bug] interference between Position and Offboard mode
6 participants