Handle GoPro mavlink heartbeat, shutter toggle, and capture mode toggle

[Pedals2Paddles]

This enables the use of a Solo gimbal on a vehicle other than a Solo. Also has some use on the Solo itself to allow mission/gcs camera triggers to work out of the box.

Updated 2/11/2020 after much debate over library location

GCS_Mavlink:

• Routing of the GoPro/Gimbal heartbeat to the AP_Camera library.
• This gives us the camera's status, mode, and recording flag to be consumed by other new functions.
• This was not previously handled or consumed, but has always been there going nowhere.

AP_Camera:

• Creates AP_Camera/AP_Camera_SoloGimbal to handle the camera on the gimbal.
• Parse the gimbal's heartbeat for status, mode, and recording state.
• Added function to toggle the shutter command. This will take a photo, or start/stop video depending on what mode you're in.
• Added a function to toggle the capture mode between photo and video.
• GCS text tells the operator what it's doing.
• Added option 2 for parameter CAM_TRIGG_TYPE. Will trigger the GoPro shutter toggle rather than servo or relay shutters.
• This also ends up getting called by anything using mav_cmd_do_digicam_control, such as auto mode missions and GCS camera trigger buttons. So those functions will now work on the Solo as well.

RC_Channel:

• Added RC aux switch option 102 to toggle the GoPro mode. So for example a momentary switch on RC8 could be set with RC8_OPTION=102
• Momentary switch calls into the mount library to toggle modes between photo and video.

Tested and working on my quad with a solo gimbal strapped to the bottom of it.

[khancyr]

Hello,
Why put this into ap_mount and not all in ap_camera?

[andyp1per]

Yes, at some point I would like to support mavlink commands in the RunCam driver and a refactoring of this kind would make it much easier!

…

Sent from my iPhone

On 9 Feb 2020, at 09:57, Pierre Kancir ***@***.***> wrote:  Hello, Why put this into ap_mount and not all in ap_camera? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[Pedals2Paddles]

Mainly because it's all a function of the gimbal, not the camera. The mount library is already doing lots of exising mavlink communication with this gimbal. It is also not communication that would ever take place without that gimbal. So the mount library is the appropriate place for it.

[khancyr]

wait ? That isn't to control the gopro but a gimbal? Then the name isn't the right one... that is confusing. If we aren't controling a gopro, that shouldn't be named gopro ... and it should then belong to AP_Mount_SoloGimbal and not the main class

[Pedals2Paddles]

It controls the GoPro, and only a GoPro, which is a function of the Solo Gimbal, and only the Solo Gimbal. The title is correct because all these mavlink messages are gopro messages, such as mavlink_msg_gopro_set_request_send. Everything is in the correct libraries.

[khancyr]

Solo and gopro stuff shouldn't be spread on main class.

Solo gopro control has nothing to do with AP_mount, the control should be in ap_camera or restrain to ap_mount_sologimbal

[Pedals2Paddles]

Solo and gopro stuff shouldn't be spread on main class.

It is required to be in the main class. The main class is how anything gets into the mount instances. As is the case for all the existing mount library functions accessed from outside the library...

Solo gopro control has nothing to do with AP_mount, the control should be in ap_camera or restrain to ap_mount_sologimbal

As previously described, it has everything to do with it. Everything is in the appropriate place. Mount drivers do not go in the camera library.

[khancyr]

You add 3 functions and any of them is related to mount...
If it is just because ap_mount can handle mavlink, add mavlink handle on ap_camera but don't spread unessary code on ap_mount.

[Pedals2Paddles]

Pierre, it is absolutely 100% related to and specific to the mount library instance. There is no other place where it could or should be. I did this programatically exactly the same as all the other existing functions for that gimbal in the mount library driver instance. I think maybe you're just misunderstanding how this hardware works since it is rather unique.

AP_Mount > Mavlink > Solo Gimbal > process mavlink into controls > GoPro data bus & gimbal stabilization/control.

If this is still unclear, I ask that you take my word for it after many years of working with this hardware and this library, that I've done it the right way.

[peterbarker]

This is definitely a case of "device has a foot in two worlds" problem.

Notionally you could have a Solo gimbal with no motors on it which you use just to trigger the camera.

Notionally you could also have a Solo gimbal that has a really bright light on it rather than a camera.

What this probably means is that if you want to have a "AP_TypicalSoloGimbal" - the thing you find on the bottom of a Solo with a camera in it - then that device should NOT be under AP_Mount because that's only half its job - the other half being taking video or photos. We don't cover these dual-backend-objects very well in ArduPilot.

Even in the case we did have an AP_TypicalSoloGimbal thing - we'd need a place in AP_Camera for the GoPro stuff.

We don't have a frontend/backend split for Camera. But an AP_Camera_SoloGoPro would appear to be appropriate.

@Pedals2Paddles you've obviously been playing in here. You've come to the conclusion that it's appropriate to be controlling a camera in the mount library. But what does an implementation look like of an AP_Camera_SoloGimbal? What coupling is there between that and AP_Mount_SoloGimbal? I'm guessing they'll both want the same mavlink channel, and that's about it.

[Pedals2Paddles]

One can't exist without the other, so it's not really a relevant thing IMO. The AP_Mount driver instance communicates with the gimbal and only the gimbal, including these gopro mavlink messages. AP does not and cannot communicate with the camera directly. The gimbal communicates with the camera directly using closed source proprietary licensed methods. There isn't really an "other than typical solo gimbal". It is what it is.

Since this has no application at all beyond the Solo Gimbal, spreading this out among multiple libraries, and re-engineering the camera library to accommodate it, is just not a reasonable or practical path.

[peterbarker]

@Pedals2Paddles Perhaps an accessor on AP_Mount to explicitly get an AP_Mount_SoloGimbal would make this a bit cleaner.

[Pedals2Paddles]

I'm not sure how to do that without guidance. But that would be handling this differently than everything else in the mount library, including all the other existing things that are specific to the solo gimbal working the same way. I would rather keep everything consistent then start making one-offs.

[peterbarker]

@Pedals2Paddles Something like this: https://github.com/peterbarker/ardupilot/tree/gopro

That branch isn't mergeable as-is, it's just a refactoring of what you've done - and I've bodged things in with statics rather than creating a singleton, that sort of thing.

[Pedals2Paddles]

Making a whole new library just for this? That all seems significantly unnecessary for what is a very simple enabling of existing gimbal functionality.

Honestly, I just want to keep it consistent and simple with what already exists. This is a 5-year-old camera, on a 5-year-old gimbal, with proprietary communication, that will never ever change or grow into anything more than it is. So reinventing and re-engineering things around it is just not a good use of my time or anyone else's.

Unless there is some compelling reason that doing it the way I've done it is forming a tear in space-time, I'm not going to go down the road of reinventing the wheel for this. So right now, I intend to keep it the way it is.

[peterbarker]

The PR as it currently stands introduces unnecessary coupling between the Mount and Camera libraries.

It also pollutes both the AP_Mount and AP_Mount_Backend interfaces with code which is specifically introduced solely for the Solo Gimbal (but not marked as such).

An alternative approach which removes these issues has been supplied.

[Pedals2Paddles]

Extending existing functionality in the location where it already exists, using the same methods that are already in use is pollution? But making it an entire new library to do the same thing is not pollution?

TBH I'm really perplexed by this and I just don't know what to say about this anymore.

[peterbarker]

These three methods:
https://github.com/ArduPilot/ardupilot/pull/13528/files#diff-480ce6943f90e91366072d695ad164d3R694

and these three methods:

https://github.com/ArduPilot/ardupilot/pull/13528/files#diff-da28ad33a674524936fe2b85f3fe968dR78

[peterbarker]

.... having said that, it appears that the gimbal and camera aspects of the SoloGimbal are actually completely disparate; there's no use of the solo-mount-backend-getter left in the library I created (which surprised me a little).

That being the case, it could probably all be contained within AP_Camera until there's some linkage between the Solo mount functionality and its camera functionality.

[peterbarker]

I've reworked the branch I created to make this trigger type just another way AP_Camera triggers the camera.

@andyp1per You thoughts on manipulating whatever stuff we can on a Solo Camera using the RunCam-style controls?

[Pedals2Paddles]

The existing methods handle_gimbal_report and handle_param_value in mount library work the same way, which is the code path I followed to make this consistent with the way the library already works. Several others exist as well, used by other mount drivers universally.

Adding this to AP_Camera instead is adding something that doesn't exist without this mount driver to begin with. All the communication in that new camera library is talking to the mount, not the camera. And it's more drivers filling up flash for every vehicle, including those without solo gimbals... which is most of them.

[peterbarker]

Generally looks pretty good.

[peterbarker]

On Wed, 12 Feb 2020, Matt wrote: In the use case I'm developing this around, momentary buttons are aplenty and there are no real toggle or 3 way switches (a herelink controller). I did it as a 3-way originally and it was torture in actual operation trying to count 1.5 seconds holding a button down and letting go at just the right time to get video mode instead of camera mode. Momentary triggering a cycle between modes was orders of magnitude more usable in reality. In the case of a traditional RC with an array of latching switches instead of momentary buttons, I acknowledge this is less than perfect.

Cool. I was wondering if it was something like this. We need a proper "button" interface in here somehow. The Sub guys cooked their own to a certain extent. We could add a switch-style version for this function - but I'm a little afraid it will set a trend and we'd end up with half-a-trillion near-enough-to-duplicate options differing only in button/switch assumption. We could consider having an RC3_CONFIG bitmask parameter whose zeroth bit would be "consider to be a button interface" bit. Downside is 16 more parameters (perhaps 18 if muramura pushes through with his PR). We don't have a set of parameters for RC as a whole (at this level). As said, 'though - not going to block this one - but I'm worried we'll start to see "button-style" duplicate rc channel options popping up all over the place.

[peterbarker]

On Wed, 12 Feb 2020, Matt wrote: + gopro_is_recording = report_msg.flags & GOPRO_FLAG_RECORDING; I'm not sure how to otherwise obtain that information from within other methods that use the information? Also, how would it react in the absence of a heartbeat without these variables which exist no matter what?

remove the variable, insert the calculation code you're assigning into the variable. Keeping two copies of the same state around is generally not a good idea is they have a habit of getting out of sync. Might not happen here, but it's a pattern thing. Other ways to solve the same problem include having a "gopro_recording()" method which does the bitmask operation.

[peterbarker]

On Wed, 12 Feb 2020, Matt wrote: OK. So would the status instead something along the lines if the status is GOPRO_HEARTBEAT_STATUS.GOPRO_HEARTBEAT_STATUS_CONNECTED? https://mavlink.io/en/messages/ardupilotmega.html#GOPRO_HEARTBEAT_STATUS

You don't need the namespacing - just GOPRO_HEARTBEAT_STATUS_CONNECTED. Because C.

[peterbarker]

On Wed, 12 Feb 2020, Matt wrote: Not sure what to check since it is coming straight from the mavlink message of that type?

Solo is a bit of a special case in that that hardware/software isn't changing. But generally you don't asume the other end is going to give you sensible values. In this case you'd: switch (report_msg.status) { case UNDERSTOOD1: case UNDERSTOOD2: case UNDERSTOOD3: gopro_status = (castgoesinhere)report_msg.status; break; default: gcs().send_text(MAV_SEVERITY_WARNING, "Curious status received from gopro"); break; } This way gopro_status never takes on a value outside the enumeration - when switching() on it later you can omit the default clause, handle each case, let the compiler tell you you've stuffed up - and not have to worry about the case where the value isn't *actually* in the enumeration. We have this issue in several enumerations in ArduPilot - all of the "type" parameters we take from parameters usually accord with an enumeration defined in our headers (e.g. RNGFND1_TYPE). Thing is - that parameter can get out-of-date with the code, or just mis-set by the user. So we must guard against that. // get rangefinder type for an ID Type get_type(uint8_t id) const { return id >= RANGEFINDER_MAX_INSTANCES? Type::NONE : Type(params[id].type.get()); } That sanity check isn't actually so crash hot - we can have holes in the enumeration so get_type *still* isn't guaranteed to return you a valid ID!

[Pedals2Paddles]

Alright I think this is in good shape now. I made the RC Aux option a generic camera mode toggle, so @andyp1per can use it for the runcam too. Any other camera that may need it in the future.

@khancyr and @peterbarker can you give this a quick once over again?

[peterbarker]

Should this new code go behind HAL_MINIMIZE_FEATURES? Do you know how much this new code impacts code size?

/me is wondering why the Solo Gimbal code isn't behind minimize-features - I believe the CubeSolos don't have the 1MB flaws.

[peterbarker]

On Thu, 13 Feb 2020, Matt wrote: I did then realized it would be sending the warning over and over at whatever hz the heartbeat is at. For how unlikely it is and how few users there are...?

OK, good call.

[rmackay9]

this has been added to Copter-4.0.4-rc1

[Hwurzburg]

@Pedals2Paddles I need details for the wiki on this....how is it connected to the Solo gimbal, what params must be setup to use it....I ran across this just by accident trying to update the wiki on RC switch options which became very out of date, very fast recently....