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

ArduSub integration #5655

Closed
wants to merge 502 commits into from

Conversation

@jaxxzer
Copy link
Member

commented Feb 2, 2017

New Vehicle Type: Sub

ArduSub has been in development for just over a year. In that time, we have come a long way. It started by simply copying the ArduCopter directory and poking around to see what we needed to change in order to make our vehicle move around underwater. Once we had accomplished that, and as we became accustomed to the extensive codebase, we progressed by increasing and improving functionality. We had our first stable release right at the end of 2016. We versioned the release as 3.4, in line with where we picked up from Copter. We are currently working on 3.5-dev.

We ship our BlueROV2 running ArduSub on a Pixhawk, and the response from professionals in the marine industry has been overwhelmingly positive. In addition to the BlueROV2, we’ve designed ArduSub to be very flexible, and we have DIY ROV users around the world with different ROV designs and motor configurations. ArduSub is thoroughly documented at ArduSub.com, and we have a very active ArduSub Gitter Channel.

bluerov2-1
The BlueROV2 is the world's most affordable high-performance ROV

From ArduCopter to ArduSub

The first hurdle was in figuring out how to make our vehicle actually move around underwater. The original development platform, the BlueROV1, has 6DOF, and while it can pitch and roll, it does not need to do so in order to translate in the x and y axes. Our solution was to subclass AP_MotorsMatrix with AP_Motors6DOF, overriding add_motor_raw to include the forward and lateral DOF that multicopters lack.

The second hurdle was acheiving the tantalizing prospect of holding depth with a positive or negatively buoyant vehicle. The onboard barometer is in a sealed compartment, and the pressure will obviously not correspond with altitude. The Bar30 pressure sensor, incorporates the MS5837 waterproof pressure sensor from Measurement Specialties, the same people who brought you the familiar MS5611. This sensor has almost exactly the same interface as the MS5611, which was a welcomed coincidence in the very early stages of development, when we were still learning how everything in ardupilot worked. We use the MS5611 driver to drive the external MS5837, and added a few members to the AP_Baro class in order to distinguish between an 'air' barometer and a 'water' barometer. Fortunately for us (and thanks to you guys), there was already support for multiple barometers and an option to set the primary barometer to use with the EKF. We also added a method to the EKF in order to internally set the baro_alt_noise parameter to a low value, because the pressure measurements underwater are very precise.

ArduSub Testing
One of the earliest tests of ArduSub, on an APM 2.6 (YouTube Link)

We have three supported flight modes, Manual (no stabilization), Stabilize, and Depth Hold. We have made progress in implementing more advanced position-enabled modes; we've even executed short missions in auto mode. We have also managed to create a working rudimentary model in SITL.

GPS receivers will not work underwater, so we have added an AP_GPS_MAVLINK class in order to support marine industry localization sensors. This class inherits AP_GPS_NMEA, and works by receiving raw NMEA sentence data from the telemetry connection in the form of the GPS_INJECT_DATA message. This was implemented before the AP_GPS_MAV type was added, and there is some overlap in terms of functionality. The advantage of AP_GPS_MAVLINK over AP_GPS_MAV is that the serial data (in the form of NMEA sentences) from a GPS system connected to a topside or companion computer can be sent directly over the MAVLink connection to the vehicle and parsed by the autopilot, with no need to parse the data at the origin before finally formatting the output as a GPS_INPUT MAVLink message. AP_GPS_MAVLINK also eliminates the requirement of reserving a UART for GPS input.

There are a few other minor additions to note:

  • The AP_JSButton library was added to handle joystick button mapping to various vehicle functions. - It is supported by QGC as well.
  • PosControl and Fence: added a minimum z limit in order to limit maximum depth
  • Added a leak detector library
  • Added a temperature sensor library

screenshot 2017-02-01 21 59 46
Joystick Button Configuration in QGC

Hardware

ArduSub is used in conjunction with a hard-wired telemetry connection over a tether. This connection is implemented via a RS422 interface directly to the autopilot, or via UDP with MAVProxy running on a companion computer. Pilot input is expected to come over MAVLink via MANUAL_CONTROL messages, and RC input is not supported because RC signals will not penetrate water. Support for ArduSub has been integrated into QGroundControl, and we continue to contribute to QGroundControl in order to improve support for ArduSub as well as other features common to all vehicles.

We have tested ArduSub primarily on the Pixhawk 1, but we have some users on other autopilots including the Navio2 and BBBmini.

screenshot 2016-12-31 10 09 45
ArduSub Frame Configurations in QGC

Where We’re Headed

ArduSub is being actively developed with a full time developer and several contributors around the world. We plan to continue adding new features and improvements and it’s very important to us to stick with ArduPilot’s original goal of being open source and highly capable. We think that ArduSub is already more capable and extensible than most other ROV control systems.

Robosub Competition
BlueROV1 running ArduSub, navigating with computer vision and no pilot input (YouTube Link)

Dare to Explore
BlueROV2 launch (YouTube Link)

rjehangir and others added 30 commits Apr 18, 2016
Sub: Updated Correction Factors for Vectored6DOF
These are the corrected factors Vectored6DOF. The other ones don’t work
at all. I forgot to push this a long time ago.

@OXINARF OXINARF added the Sub label Feb 2, 2017

@OXINARF

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

In honour of this PR, I've created the Sub label! I hope I've captured the color correctly 😃

I share @tridge's concern about the merge commits. I've not gone through it yet, but if the issue with a rebase is keeping git history, I understand it might be hard -if that's not a big concern, then it shouldn't be very hard to rebase. I'm available to help in either case.

@OXINARF OXINARF requested review from WickedShell, rmackay9 and peterbarker Feb 2, 2017

@OXINARF OXINARF added the Tools label Feb 2, 2017

@OXINARF OXINARF requested review from lthall and lucasdemarchi Feb 2, 2017

@OXINARF OXINARF added the PX4 label Feb 2, 2017

@magicrub

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

@jaxxzer

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

Thanks all for the kind words. I don't mind doing a rebase, but I would need some help. I'm not sure how to handle the rebase with our merge commits. I'm going to try it now though, and see how it goes.

@tridge

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

I've done a semi-automated rebase here:
https://github.com/tridge/ardupilot/tree/sub-merge2
I only attempted to preserve the commits in ArduSub, and even so it needed a bunch of fixup commits at the end to bring it inline with your branch
It still needs some work, but at least the basic rebase is done, giving you a starting point

@WickedShell
Copy link
Contributor

left a comment

I've only really reviewed the changes to AP_GPS area, outside of a large selection of tabs/vs spaces things that shouldn't hold everything up, but by the same time will need to be fixed at some point.

Outside of that I have a couple of questions: If you are inputing a crosstrack error through the AP_GPS instance like this doesn't that make the GPS a navigational controller/input rather then just a locating device? I assume your input that is coming in is a SBL device of some sort? It's a clever use, but the fact that the GPS is aware of the desired track seems a bit surprising to me.

if(_type[instance] == GPS_TYPE_MAVLINK) {
// check for if user selected MAVLINK GPS
// Also not possible to autodetect, and does not require a UART
//_broadcast_gps_type("MAVLINK", instance, -1); //baud rate isn't valid

This comment has been minimized.

Copy link
@WickedShell

WickedShell Feb 3, 2017

Contributor

It's still worth emitting a broadcast message, it's quite useful for assessing exactly which GPS driver has been selected for debug purposes.

Tabs/spaces as well, although that is a fairly minor note here.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

Done.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 8, 2017

Author Member

All changes to AP_GPS have been taken out.

@@ -72,7 +72,8 @@ class AP_GPS
GPS_TYPE_QURT = 12,
GPS_TYPE_ERB = 13,
GPS_TYPE_MAV = 14,
GPS_TYPE_NOVA = 15,
GPS_TYPE_NOVA = 15,

This comment has been minimized.

Copy link
@WickedShell

WickedShell Feb 3, 2017

Contributor

Not to obsess about it but tab/spaces here as well :)

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 3, 2017

Author Member

Is the preference for the project spaces only, or just make sure everything lines up either way?

This comment has been minimized.

Copy link
@WickedShell

WickedShell Feb 3, 2017

Contributor

4 spaces rather then tab is the standard. http://ardupilot.org/dev/docs/style-guide.html#spaces

{
for(int i = 0; i < len; i++) {
char c = data[i];
#ifdef NMEA_LOG_PATH

This comment has been minimized.

Copy link
@WickedShell

WickedShell Feb 3, 2017

Contributor

I assume this isn't enabled in a normal build? The NuttX platforms struggle with multiple files open at once, but it's a very valid debug tool for other platforms.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 3, 2017

Author Member

Yes, it is disabled by default, but I need to add some things in this file to enable it. Copy/paste error, thanks.

*/

//
// NMEA parser, adapted by Michael Smith from TinyGPS v9:

This comment has been minimized.

Copy link
@WickedShell

WickedShell Feb 3, 2017

Contributor

This pair of comments seems stale? Copy/paste error?

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

Yes, fixed.

@guludo
Copy link
Contributor

left a comment

Wow! It's great to have a new vehicle added to ArduPilot. Congrats!

I just did a review from the build system perspective, which looks good to me. However, thanks to warnings during the build I noticed that the function report_frame() could/should be removed.

print_blanks(2);
}

void Sub::report_frame()

This comment has been minimized.

Copy link
@guludo

guludo Feb 3, 2017

Contributor

A believe this function could be removed. This is applicable to copter, but not for this vehicle, I think. At least not for now. My guess is that this is a leftover from adapting this code from ArduCopter, which by the way has only MULTICOPTER_FRAME and HELI_FRAME now.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 3, 2017

Author Member

Fixed, thanks! We do have different frames :).

This comment has been minimized.

Copy link
@guludo

guludo Feb 3, 2017

Contributor

We do have different frames :)

Oh, nice :-)

@OXINARF
Copy link
Member

left a comment

I've made a couple of comments (45? I'm crazy, sorry 😄). Many of them are just "Tabs" so we can keep track of where it should be changed before merge - I've only added that to existing files, the ones where a regression isn't wanted. In the new files there are also a lot of places with tabs but since those aren't regressing I didn't add a comment.

All other comments are fairly simple and only maybe one or two are really merge blockers.

Some general notes:

  • nobody else complained (specially @tridge), so it isn't a merge blocker, but I really dislike the way the baro situation is handled. I think we should go the same way we do for compass, with separate parameters for all three baros, namely chip type (for external ones where that can't be detected automatically), intended usage type (air or water), expected noise (I think that EKF should ask sensors what noise is expected, not a vehicle telling EKF that), etc.
  • I'm also not particularly fond of the MAVLink NMEA GPS type, but a better solution would require big changes in the GPS library. Can we at least change the name to MAVLinkNMEA? It's much more clear. By the way, the new type is missing in parameter documentation.
  • Regarding JSButton: I guess you introduced that because of MANUAL_CONTROL message? I think it's too late to go back now, but I think it would have been better to convert the button pushes to an RC override value - in the end a button is just like a two position switch. Two reasons for this:
    • RCX_OPTION and buttons are basically a duplication
    • it's quite hard to make a generic library when vehicles have so different options; the proof for this is that you felt the need to add custom options, which for a user will mean nothing unless he goes look in the wiki what it does for a specific vehicle
  • I wish the TemperatureSensor library had the frontend/backend split already, but it can obviously be done in the future

I think it's guaranteed that you will get commit access to the ArduSub folder after the merge, but, if I can offer a piece of advice, try to do PRs as much as possible (unless it's really small changes); other people will review and bugs can be catched earlier in the process.

This was long, sorry about that 😊 Great to see this really!

@@ -0,0 +1,44 @@
// -*- tab-width: 4; Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil -*-

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Just a note (the ArduSub directory can be corrected later): we have removed this comments in exchange of a global file in root.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

Done.

along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
/*
* ArduCopter Version 3.0

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Again, just a note, but these comments below should probably be removed, I think that a note saying ArduSub was originally copied from Copter is enough.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

Done.

## License ##
[Overview of license](http://dev.ardupilot.com/wiki/license-gplv3)

[Full Text](https://github.com/bluerobotics/ardupilot/blob/master/COPYING.txt)

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Is all the info in this README needed?

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

Removed the README.

@@ -0,0 +1,2488 @@
// BUILDROOT=/Users/rustyj110/git/APMROV/Build.ArduSub HAL_BOARD=HAL_BOARD_PX4 HAL_BOARD_SUBTYPE= TOOLCHAIN=NATIVE EXTRAFLAGS=-DGIT_VERSION="744a4568" -I/Users/rustyj110/git/APMROV/libraries/AP_Common/missing -DMAVLINK_PROTOCOL_VERSION=2 -DNUTTX_GIT_VERSION="579e82d4" -DPX4_GIT_VERSION="2a31d796" -DUAVCAN=1 -D__STDC_FORMAT_MACROS -DHAVE_STD_NULLPTR_T=0 -DHAVE_ENDIAN_H=0 -DHAVE_BYTESWAP_H=0 -I/Users/rustyj110/git/APMROV/Build.ArduSub/libraries/GCS_MAVLink/include/mavlink -DFRAME_CONFIG=VECTORED_FRAME

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

This build.log file isn't needed, right?

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 4, 2017

Author Member

Removed.

//////////////////////////////////////////////////////////////////////////////

#ifndef CONFIG_HAL_BOARD
#error CONFIG_HAL_BOARD must be defined to build ArduCopter

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

ArduCopter -> ArduSub

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 10, 2017

Author Member

Done, as well as elsewhere in ArduSub/

// @Increment: 1
// @User: Advanced
// @RebootRequired: True
AP_GROUPINFO("FORWARD", 4, RCMapper, _ch_forward, 6),

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Is the default really supposed to be 6, not 5?

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

We left input 5 as the mode switch.

// @Increment: 1
// @User: Advanced
// @RebootRequired: True
AP_GROUPINFO("LATERAL", 5, RCMapper, _ch_lateral, 7),

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Is the default really supposed to be 7, not 6?

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 6, 2017

Author Member

As above.

uint8_t forward() const { return _ch_forward; }

/// strafe - return input channel number for stafe input
uint8_t lateral() const { return _ch_lateral; }

This comment has been minimized.

Copy link
@OXINARF
state.distance_cm = distance_cm;
update_status();

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Was this changed for some specific reason? I think it was done this way so that a MAVLink rangefinder didn't need any min/max limit configured.

This comment has been minimized.

Copy link
@jaxxzer

jaxxzer Feb 4, 2017

Author Member

Yes, that's why I did it. The Min/max parameters for the rangefinder were being ignored.

This comment has been minimized.

Copy link
@OXINARF

OXINARF Feb 4, 2017

Member

Ok. @rmackay9 any thoughts on this?

@@ -27,6 +27,7 @@
#define APM_BUILD_AntennaTracker 4
#define APM_BUILD_UNKNOWN 5
#define APM_BUILD_Replay 6
#define APM_BUILD_ArduSub 7

This comment has been minimized.

Copy link
@OXINARF
@jaxxzer

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

I'm working out of a different rebased branch (thanks Tridge) applying the suggested changes.
You can find that here:
https://github.com/bluerobotics/ardusub/tree/sub-merge

@magicrub

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

This PR is getting messy with lots of fixes. Should we start a new one with the new branch @jaxxzer and @tridge worked on together?

@tridge

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

@jaxxzer you could just to a forced push of the new branch over this one and keep the same PR

@magicrub

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

yes, that works to. I'm in favor of that method but I know some people like to keep all the inline comments which a force push nukes

@jaxxzer

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

That was my concern, was the comments being invalidated.
I've addressed almost everything so will force push soon.

@jaxxzer

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

Whoops, I thought you could edit the PR to change the branch to merge, and that was what you guys meant. I was mistaken and you can only edit the base branch that you want to merge into.

I don't want to force push to our master.

I can open another PR with the reworked branch.

@jaxxzer jaxxzer referenced this pull request Feb 7, 2017
@OXINARF

This comment has been minimized.

Copy link
Member

commented Feb 18, 2017

I'm closing this one now, since we should be close to merge the newer one. In any case, comments can still be made here if needed.

@OXINARF OXINARF closed this Feb 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.