-
Notifications
You must be signed in to change notification settings - Fork 769
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
[DO_NOT_MERGE_YET] rotors_hil_interface: take advantage of MAVROS hil plugin #438
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
@hitimo @pvechersky @helenol can you please take a look? Thanks |
@@ -25,43 +25,51 @@ namespace rotors_hil { | |||
// Default values | |||
static constexpr bool kDefaultSensorLevelHil = true; | |||
static constexpr double kDefaultHilFrequency = 100.0; | |||
static constexpr double kDefaultBodyToSensorsRoll = M_PI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only question is here: on MAVROS, we apply ENU to NED transform (https://github.com/mavlink/mavros/blob/master/mavros/src/lib/ftf_frame_conversions.cpp#L30) and then base_link to body/aircraft frame (https://github.com/mavlink/mavros/blob/master/mavros/src/lib/ftf_frame_conversions.cpp#L37), while you just seem to apply this last one. Which frame of reference are we using in rotors_gazebo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Rotors uses the standard ROS reference frame of NWU, hence the rotation by 180 degrees around the x axis to convert it to NED in the HIL interface node in this case.
@hitimo @pvechersky @helenol can you give me your thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually, the changes are good. I think as much of MAVROS's built-in functionality should be leveraged by our HIL module, and using the hil plugin is a good step in that direction.
I just have a couple of higher-level comments that the other reviewers might follow-up on:
-
In rotors_hil.rosinstall you change the MAVROS uri to the mavlink repo instead of the ethz-asl one. The switch to ethz-asl repo for MAVROS was done after my time of working on this HIL interface so I'm not sure if the Rotors admins would prefer to stick to the ethz-asl repo or not. But if the ethz-asl repo is preferred then perhaps relevant hil plugin changes in MAVROS can be merged into that repo?
-
Code formatting. Rotors used Google coding standards I believe. See other C++ code in the project for reference.
Thank for integrating these improvements!
Can one of the admins verify this patch? |
ok to test |
Hi, I would like to use HIL and I'm blocked by #367 which I understand would be solved by this. What is needed to finish this PR? |
Hi, anyone still looking into this? I tried this PR and everything mostly works. I have only two issues:
|
@v01d I got too much on my back to finish this, as I am also supporting and maintaining |
Cool thanks. |
Another question, is the mavlink interface plugin required? I'm not really sure what it is for, since the hil interface seems to do all the talking to mavros. |
@TSC21 How should we move forward with this? |
test this please |
With this PR we take (almost) full advantage of MAVROS hil plugin. There, the unit conversions are applied, besides the frame transform to NED. I also applied the uncrustify style for C++. Let me know if you want me to revert this step.