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

Check system type for SET_ATTITUDE_TARGET attitude publising #13141

Merged
merged 10 commits into from
Oct 26, 2019

Conversation

irsdkv
Copy link
Contributor

@irsdkv irsdkv commented Oct 9, 2019

Check if system type is VTOL and publish attitude to proper uORB topic

Describe problem solved by the proposed pull request
Attitude target publish in wrong uORB topic in case of VTOL system type/

Test data / coverage
Tested on VTOL UAV and MAVSDK.

Describe your preferred solution
Check is system type is VTOL (VTOL system types from is_vtol()).

@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 9, 2019

This is a potential duplicate of #13122

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch 2 times, most recently from c92d8b8 to 2949452 Compare October 9, 2019 13:32
@julianoes julianoes requested a review from Jaeyoung-Lim October 9, 2019 13:50
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

IMO this should be handled in the mc_att_control and fw_att_control modules rather than the mavlink receiver.

src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.h Outdated Show resolved Hide resolved
@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 918eb8e to 7ed8f8c Compare October 9, 2019 15:04
Copy link
Contributor

@jlecoeur jlecoeur left a comment

Choose a reason for hiding this comment

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

I think this is almost good, except:

  • the attitude setpoint will no longer be sent if thrust ignore flag is set, I made code recommendation for this but did not test.
  • rates setpoint should be handled the same way

src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved
@Jaeyoung-Lim
Copy link
Member

@irsdkv Just curious, what is the main usecase for sending offboard attitude setpoints? Do you also want to control fixed wing with such setpoint?

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 47a82a7 to d8b3e77 Compare October 10, 2019 08:10
@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 10, 2019

Thanks, @jlecoeur

@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 10, 2019

@irsdkv Just curious, what is the main usecase for sending offboard attitude setpoints? Do you also want to control fixed wing with such setpoint?

At this time we use this only for MC mode. But in future it is most likely that we will test in FW mode also.
At least in Gazebo HITL Simulation.

@Jaeyoung-Lim
Copy link
Member

Jaeyoung-Lim commented Oct 10, 2019

@irsdkv My question is more on the decision on selecting attitude setpoints for VTOL control. Is there a specific reason? why not use high level setpoints such as position?

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from d8b3e77 to 0c40906 Compare October 10, 2019 08:24
@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 10, 2019

@Jaeyoung-Lim oh, I understand now. This needed in two cases for us:

  • Testing mechanics of our VTOL model
  • Testing our control algorithms

Copy link
Contributor

@jlecoeur jlecoeur 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 to me, it should be flight tested before merging.

@jlecoeur
Copy link
Contributor

@ThomasRigi can you please check that this solves your issue?

@ThomasRigi
Copy link
Member

ThomasRigi commented Oct 14, 2019

@ThomasRigi can you please check that this solves your issue?

It works in SITL (only tested in MC mode), not in HIL. In HIL the attitude setpoint remains the last one from before set_attitude() is called (in my case set from takeoff or hold). I still have no idea why this happens.

Can someone else test the HIL simulation? Maybe something is wrong with my setup (even though take off command and many other things (offboard NED or set_actuator_control(), for example) work). I'm using a Pixhawk 4 which has never been connected to a real drone before.

(Edit : for as long as it flies in the real world I don't care too much about HIL, but it's still weird)

@jlecoeur
Copy link
Contributor

Thanks @ThomasRigi , is it working in HIL with a multirotor?

@ThomasRigi
Copy link
Member

@jlecoeur yes, it works with HIL Quadcopter X

@jlecoeur
Copy link
Contributor

Ok then this is not directly related, can you open a separate issue to track this @ThomasRigi ?

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 4dfb58a to 4dda73a Compare October 17, 2019 17:27
jlecoeur
jlecoeur previously approved these changes Oct 18, 2019
TSC21
TSC21 previously approved these changes Oct 26, 2019
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

LGTM

@TSC21 TSC21 added this to the Release v1.10.0 milestone Oct 26, 2019
@TSC21
Copy link
Member

TSC21 commented Oct 26, 2019

[2019-10-26T11:42:14.960Z] /opt/gcc/bin/../lib/gcc/arm-none-eabi/7.2.1/../../../../arm-none-eabi/bin/ld: px4_fmu-v2_fixedwing.elf section `.data' will not fit in region `flash'
[2019-10-26T11:42:14.960Z] /opt/gcc/bin/../lib/gcc/arm-none-eabi/7.2.1/../../../../arm-none-eabi/bin/ld: region `flash' overflowed by 64 bytes

Really?! 64 bytes?... @irsdkv is there some gymnastic you can do here to reduce the footprint?

@TSC21 TSC21 requested a review from dagar October 26, 2019 12:31
@irsdkv irsdkv dismissed stale reviews from TSC21 and jlecoeur via a6ce730 October 26, 2019 12:53
@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 26, 2019

Hm. Lets see what we can do.

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from a6ce730 to 05b2abe Compare October 26, 2019 13:07
@jlecoeur
Copy link
Contributor

I suggest to remove a few drivers from px4_fmu-v2_fixedwing

https://github.com/PX4/Firmware/blob/05b2abe57a06dd32ccb54e32a5bf4a78ffa406fe/boards/px4/fmu-v2/fixedwing.cmake#L28

For example distance sensor drivers are built for v2_fixedwing but not v2_default

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 05b2abe to 52e8655 Compare October 26, 2019 13:30
@irsdkv
Copy link
Contributor Author

irsdkv commented Oct 26, 2019

[2019-10-26T13:39:42.443Z] region `flash' overflowed by 8 bytes

image

@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 52e8655 to 6e608a1 Compare October 26, 2019 13:49
@irsdkv irsdkv force-pushed the pr-check_vtol_in_set_attitude_target branch from 6e608a1 to 66d5786 Compare October 26, 2019 14:04
@jlecoeur
Copy link
Contributor

It should fit now 🤞

@TSC21 TSC21 merged commit 0326a8e into PX4:master Oct 26, 2019
@jlecoeur
Copy link
Contributor

thanks for all the updates @irsdkv 👍

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

Successfully merging this pull request may close these issues.

6 participants