mocap: add SetVisionSpeedEstimate #361
Conversation
protos/mocap/mocap.proto
Outdated
| // Global speed estimate from a vision source. | ||
| message VisionSpeedEstimate { | ||
| uint64 time_usec = 1; // PositionBody frame timestamp UNIX Epoch time (0 to use Backend timestamp) | ||
| SpeedBody speed_body = 2; // Global speed (m/s) |
There was a problem hiding this comment.
Just wondering on the frame here? When you see "Global" do you mean in North, East, Down, or Forward, Left, Down? I'm pretty sure "Body" would imply in body frame, so Forward, Left, Down.
There was a problem hiding this comment.
Hi @julianoes! Thanks for the review!
I used the same description as in the MAVLink doc, but I guess this is a typo.
However, I see in the VisionPositionEstimate a similar case:
PositionBody position_body = 2; // Global position (m)
Should I fix it too?
There was a problem hiding this comment.
Interesting. Might be worth checking what PX4 expects and whether we should update the MAVLink messages description.
There was a problem hiding this comment.
For position, PX4 expects NED:
I don't see vision speed being used in PX4. How are you testing this?
There was a problem hiding this comment.
Actually, we are currently testing with ArduPilot.
We used Intel RealSense T265 as an example (see https://ardupilot.org/copter/docs/common-vio-tracking-camera.html).
I suggest changing the "Global speed (m/s)" comment to "Body speed (m/s)"
There was a problem hiding this comment.
Hi all,
to clear this up a bit, if we follow the mocap position from MAVSDK.
- The mocap function set_vision_position_estimate() expects global position(NED) according to documentation the input parameter for this function calls the position PositionBody so this might be a typo.
- MAVSDK uses the mavlink VISION_POSITION_ESTIMATE message which according to documentation expects the local frame(FRD, not body).
- Ardupilot does not differentiate between local(FRD) and global(NED) frame as it doesn't have a local frame, so it interprets the position in global frame. In fact, it processes the messages VISION_POSITION_ESTIMATE and GLOBAL_VISION_POSITION_ESTIMATE the same way, see here
Ardupilot also expects global(NED) position from the mocap class, and consequently global(NED) velocity. So, if PX4 is the same, I think the only thing that needs to be changed is the variable's name in the VisionPositionEstimate structure, as @Luc-Meunier suggested.
Let me know if you agree
There was a problem hiding this comment.
Ok, so if both PX4 and ArduPilot interpret the values as NED, that's good news. In that case I'm tempted to rename the PositionBody type to PositionNed to make it clear. And then we should update the MAVSDK and MAVLink docs to make it clear.
There was a problem hiding this comment.
Thanks @fed12345 and @julianoes, I added a SpeedNed struct.
There was a problem hiding this comment.
Hello,
Just as a quick follow-up to this:
I'm tempted to rename the PositionBody type to PositionNed to make it clear...
To us it also seems like a good idea, should we do it in another PR or would it fly to add it in this one?
Aside from that, is there anything else that we should take care of?
There was a problem hiding this comment.
Thanks, sorry, I'll get back to this asap.
Luc-Meunier
force-pushed
the
vio
branch
2 times, most recently
from
64239d8 to
f6ec875
Compare
Co-authored-by: miodine <48156138+miodine@users.noreply.github.com>
Hi, we would like to add the VISION_SPEED_ESTIMATE (103) MAVLink message to the mocap proto.
See https://mavlink.io/en/messages/common.html#VISION_SPEED_ESTIMATE
Relates to issue mavlink/MAVSDK#2497