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

Offboard Review #9273

Closed
Stifael opened this issue Apr 9, 2018 · 17 comments
Closed

Offboard Review #9273

Stifael opened this issue Apr 9, 2018 · 17 comments

Comments

@Stifael
Copy link
Contributor

Stifael commented Apr 9, 2018

The offboard implementation is very confusing for several reason:

  • not everything is documented (in PX4 and mavlink)
  • race condition is not taking care of (messages are sent to commander and mc_pos_control at the same time)
  • triplet message is misused

I wanted to do some offboard refactor for flighttask, but then I realized that there are a few code implementation that are difficult to understand.

A few missing pieces:

@AlexisTM
Copy link
Contributor

@Stifael

For the outer loop, it's that the attitude control needs the yaw/yaw_rate as input.

@Stifael
Copy link
Contributor Author

Stifael commented Apr 10, 2018

For the outer loop, it's that the attitude control needs the yaw/yaw_rate as input.

Ok, but still don't understand why position and velocity is required...
The input to the attitude control is an attitude quaternion and a scalar thrust.

@AlexisTM
Copy link
Contributor

AlexisTM commented Apr 10, 2018

You can send "body_rates", "roll pitch yaw thrust" or even "actuator control" commands over OFFBOARD mode.

Therefore, depending on the input, we have to enable controls depending on the lowest level control. If you receive body_rate from the offboard companion,

You will not run the attitude control which outputs body_rate setpoints or lower.
You will not run the body_rate controller if you receive actuator setpoints or lower.
You will not run position controller if you receive velocity setpoints or lower.

Indeed there is one problem. If you ignore everything in your offboard message (SET_TARGET_LOCAL_NED) with a flag ignoring everything, the copter just shuts down the motors, because the different controls are not in one message, so we cannot check the consistency of the command.

A solution to it was proposed here, to only allow 4 degrees of freedom in the fancy you prefer: http://discuss.px4.io/t/idea-for-a-more-versatile-offboard-control/4721 Yet, it did not go further as the offboard mode is currently reworked using Flight Tasks (see the PR about it https://github.com/PX4/Firmware/pulls?utf8=%E2%9C%93&q=is%3Apr+flighttask)

The idea is to give a single channel where people use OFFBOARD commands, thus you don't have race conditions on the control and you can only control 4 degrees at the same time (X, Y, Z, Yaw).

@Stifael
Copy link
Contributor Author

Stifael commented Apr 10, 2018

You will not run the attitude control which outputs body_rate setpoints or lower.
You will not run the body_rate controller if you receive actuator setpoints or lower.
You will not run position controller if you receive velocity setpoints or lower.

I think we are talking about the same thing, but I am questioning the implementation.
For instance, in the current firmware for attitude control from offboard, all outer loops have to be true (or not ignored)
https://github.com/PX4/Firmware/blob/master/src/modules/commander/commander.cpp#L3743-L3746

Yet, it did not go further as the offboard mode is currently reworked using Flight Tasks (see the PR about it https://github.com/PX4/Firmware/pulls?utf8=%E2%9C%93&q=is%3Apr+flighttask)

I know that there is a PR because I opened it. However, there is no reason to not continue to clean up the offboard implementation as you suggested here http://discuss.px4.io/t/idea-for-a-more-versatile-offboard-control/4721, although with Flighttask it can be farther simplified.

@hamishwillee
Copy link
Contributor

@Stifael At the end of this process can we please review and update:

@FaboNo
Copy link

FaboNo commented Apr 14, 2018

I am trying to understand the offboard mode on my side as well ans I would like to clarify the following points:
1- looks like you can only send SET_TARGET_LOCAL_NED mavlink msg (which corresponds to SETPOINT_RAW mavros msg), Am I correct? Because the example given in https://dev.px4.io/en/ros/mavros_offboard.html is using setpoint_position/local mavros msg and there is no velocity in it?
2. you can send mavlink/mavros msg only on TELEM2 for the offboard mode? Otherwise (TELEM1 or USB), they will not be taken into account?
Thank you for helping me to understand the offboard mode

@AlexisTM
Copy link
Contributor

  1. All the mavros topics are working. They'll be converted to the same Mavlink message on the backend.

  2. I already tested the USB and TELEM2. I never tested TELEM1 as we use a GPS. I'm sure you can use TELEM1, but with the lowest rates.

@FaboNo
Copy link

FaboNo commented Apr 14, 2018

Thank you Alexis. I am using the USB port to communicate with the FCU. I am trying to understand how the offboard is working because so far, I do not succeed in my experiments.
I have a SLAM which is sending poses thru /mavros/vision_pose/pose and I can see that LPE and the attitude_estimator_q are taking into account the inputs because the updated poses are available thru /mavros/local_position/pose BUT when I am activating the offboard mode and let'say send a 0,0,2 to take off and hovering, the mav is drifting like it does not lock on the position(s) issued by the slam.
At least this is my understanding so far, like an updated pose is computed but not sent to "low "level" controllers.
Do you have any idea of what I am missing here?

@colineRamee
Copy link

Hi,
I am also trying to understand the offboard mode and there's something that has been escaping me, and I would really appreciate some clarification.
In modules/mavlink/mavlink_receiver.cpp at line 874 I don't understand why these variables are defined like that:

bool is_takeoff_sp = (bool)(set_position_target_local_ned.type_mask & 0x1000);
bool is_land_sp = (bool)(set_position_target_local_ned.type_mask & 0x2000);
bool is_loiter_sp = (bool)(set_position_target_local_ned.type_mask & 0x3000);
bool is_idle_sp = (bool)(set_position_target_local_ned.type_mask & 0x4000);

Doesn't this lead to is_loiter_sp being true when the type mask starts with 0x1000 (= 0b1 00...) or 0x2000 (= 0b10 00..) (take_off or land), and to is_land_sp and is_take_off_sp being true when the type mask is 0x3000(= 0b11 00..)?
If I understand correctly this is somehow handled later line 913 by an if else if statement where a comment states that the order matter, but since loiter mode is first and is true for any message with 0x1000, 0x2000 or 0x3000, I don't understand how the setpoint type can ever be set to land or takeoff.
Am I missing something obvious? Please let me know.
Why not do something like:

bool is_takeoff_sp = !(bool)((set_position_target_local_ned.type_mask & 0xf000)^0x1000);
bool is_land_sp = !(bool)((set_position_target_local_ned.type_mask & 0xf000)^0x2000);
bool is_loiter_sp = !(bool)((set_position_target_local_ned.type_mask & 0xf000)^0x3000);
bool is_idle_sp = !(bool)((set_position_target_local_ned.type_mask & 0xf000)^0x4000);

Which would only return true when the leading 4 bits are exactly identical.

@azmat-bilal
Copy link

i would confirm that the is_takeoff_sp and is_land_sp dont work becuase of the reason mentioned above i.e. the '& 0x3000' condition gets true whenever either landing or takeoff bits are set, and since loiter is the first condition in the if else statement, it always put the drone in that state. as work around i have disabled the loiter functionality by

bool is_loiter_sp = (bool)(set_position_target_local_ned.type_mask & 0x0000);
but this is just to make landing/takeoff work. for proper implementation, there is work in progress in mavlink repo where loiter mask bit is moved to 0x4000 and idle to 0x8000. ref. https://github.com/mavlink/mavlink/blob/45a756eedabb9d8e83322575dd3384735541dbb5/message_definitions/v1.0/common.xml

@stale stale bot closed this as completed Jul 8, 2019
@julianoes julianoes reopened this Jul 10, 2019
@stale stale bot removed the Admin: Wont fix label Jul 10, 2019
@PX4 PX4 deleted a comment from stale bot Jul 10, 2019
@PX4 PX4 deleted a comment from stale bot Jul 10, 2019
@PX4 PX4 deleted a comment from stale bot Jul 10, 2019
@stale
Copy link

stale bot commented Oct 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 8, 2019
@Jaeyoung-Lim
Copy link
Member

@julianoes Is this still relevant?

@stale stale bot removed the stale label Nov 1, 2021
@julianoes
Copy link
Contributor

No idea.

@sstroobants
Copy link

sstroobants commented Nov 30, 2021

@julianoes , @Jaeyoung-Lim , I do think it is still relevant or at least I have some questions about the implementation...
As is discussed above:

You can send "body_rates", "roll pitch yaw thrust" or even "actuator control" commands over OFFBOARD mode.

Therefore, depending on the input, we have to enable controls depending on the lowest level control. If you receive body_rate from the offboard companion,

You will not run the attitude control which outputs body_rate setpoints or lower. You will not run the body_rate controller if you receive actuator setpoints or lower. You will not run position controller if you receive velocity setpoints or lower.

Here it is stated by @AlexisTM that you can send actuator control commands, and that all controllers won't run if you do so. It is quite interesting because in the .msg file for offboard_control_mode there is actually a field actuator but in the firmware nothing seems to be done with this in the latest version (this field is missing in the px4_ros_com package .msg file...). Is that correct?

@Jaeyoung-Lim
Copy link
Member

@sstroobants I don't think this issue is relevant to the current offboard architecture

In the new offboard architecture, the actuator setpoint support was only added back in #18581

@sstroobants
Copy link

@Jaeyoung-Lim, Thanks for the quick response, I've just found that commit 20 minutes ago... Sorry.
From what I see you can use the MAVLink messages to set actuator values, is it also possible to set the actuator values via RTPS by publishing to fmu/actuator_controls0/in in a similar way as with publishing to fmu/trajectory_setpoint/in in the offboard example?
I assume I would need to change the message definition in px4_ros_com as well to mirror the change in offboard_control_mode.msg.

@Jaeyoung-Lim
Copy link
Member

@sstroobants I don't think this issue is relevant anymore for this discussion. Could you either post a new issue or post the question on discuss.px4.io ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants