Add PublishDistanceSensor to Telemetry Server

[potaito]

Related PRs

• Proto: Add PublishDistanceSensor to Telemetry Server #310 (this PR)
• MAVSDK: Distance sensor server MAVSDK#1964

Description

The DistanceSensor is already part of telemetry and telemetry_server. The subscriber was implemented in telemetry, but the publisher in telemetry_server is missing.

I would like to add a publisher for DISTANCE_SENSOR to MAVSDK.

The question is also whether we should include the full message spec while at it. The missing fields that exist in mavlink but not in the proto definition of DistanceSensor are:

• Type (MAV_DISTANCE_SENSOR)
• ID
• Orientation (MAV_SENSOR_ORIENTATION)
• Covariance
• Horizontal FOV
• Vertical FOV
• Quaternion for orientation
• Signal Quality

Does it make sense to add all of them right away, or does MAVSDK deliberately leave some fields out that exist in mavlink, because they are not used by 99% of the people?

Question

I noticed a bug in the current implementation of the distance sensor subscriber implementation in MAVSDK. The distance fields in mavlink are specified to be centimeters, whereas the same fields in Proto are specified in meters, but there is no conversion happening. I fixed this in the MAVSDK implementation: https://github.com/mavlink/MAVSDK/pull/1964/files#diff-c6be7609c4729a6ca704109572c7b755aaac050cc429c91a024021d7760faf59R1440-R1442

Alternatively we could also change the unit in Proto to centimeters. But this would change the existing API. Changing the implementation though can just as well cause problems to someone who's already using this MAVSDK function 😅

Which option is better?

[JonasVautherin]

Alternatively we could also change the unit in Proto to centimeters.

Maybe that's an opportunity to rename a field 😅. Say

distance_m = 3; // distance in meters

to

distance_m = 3; // OBSOLETE: this was a typo in the naming
distance_cm = 3; // distance in centimeters

So that it doesn't break anyone. For MAVSDK v2 we can even remove distance_m = 3; (which is a breaking change, but since it will break the build, people will realize it). If we just convert the units without saying anything, then some people will receive wrong numbers suddenly, and that may cause issues during a flight, right? 🙈.

[potaito]

@JonasVautherin thanks for the review. How does the deprecation work? I.e.

distance_m = 3; // OBSOLETE: this was a typo in the naming
distance_cm = 3; // distance in centimeters

Does the second entry override the first definition because of the same index? So it will only be there for cosmetic reasons? What does proto end up doing with the two entries?

Before deciding, I would take a look at the remaining API defined in Proto, and whether it's very uncommon to have centimeters rather than meters. I think many distance sensors output centimeters, because they are often short-ranged. So that might be the more natural unit. But I don't have a strong opinion.

[JonasVautherin]

Does the second entry override the first definition because of the same index? So it will only be there for cosmetic reasons? What does proto end up doing with the two entries?

Oh actually I was thinking about Protobuf, but protoc-gen-mavsdk won't work with that... Still we can rename distance_m to distance_cm, which breaks the build, but in a pretty clear way I think. Whereas converting the values will be quiet, but programs will suddenly receive unexpected data 😅

[potaito]

Still we can rename distance_m to distance_cm, which breaks the build, but in a pretty clear way I think. Whereas converting the values will be quiet, but programs will suddenly receive unexpected data 😅

Ah, that's a good argument. If we are going to break it anyway, we might as well make it obvious. I'll make the changes

[potaito]

Ah I missed that when I tested locally:

Status: Downloaded newer image for uber/prototool:1.9.0
protos/telemetry/telemetry.proto:745:33:Field number 1 has already been used in "mavsdk.rpc.telemetry.DistanceSensor" by field "minimum_distance_m".
protos/telemetry/telemetry.proto:746:33:Field number 2 has already been used in "mavsdk.rpc.telemetry.DistanceSensor" by field "maximum_distance_m".
protos/telemetry/telemetry.proto:747:33:Field number 3 has already been used in "mavsdk.rpc.telemetry.DistanceSensor" by field "current_distance_m".
protos/telemetry_server/telemetry_server.proto:397:33:Field number 1 has already been used in "mavsdk.rpc.telemetry_server.DistanceSensor" by field "minimum_distance_m".
protos/telemetry_server/telemetry_server.proto:398:33:Field number 2 has already been used in "mavsdk.rpc.telemetry_server.DistanceSensor" by field "maximum_distance_m".
protos/telemetry_server/telemetry_server.proto:399:33:Field number 3 has already been used in "mavsdk.rpc.telemetry_server.DistanceSensor" by field "current_distance_m".

So we do have to remove the old field names then completely, right?

[JonasVautherin]

So we do have to remove the old field names then completely, right?

Apparently. But in protobuf that would still be backward compatible (somehow I thought it did support aliases). Anyway that's required for protoc-gen-mavsdk, so...

Sorry I did not catch it in the review, especially because I wrote it the day before 🤣. As you can see we don't do that very often 😁 (which is arguably a good point for an API 😇 )

[potaito]

So we do have to remove the old field names then completely, right?

Apparently. But in protobuf that would still be backward compatible (somehow I thought it did support aliases). Anyway that's required for protoc-gen-mavsdk, so...

Sorry I did not catch it in the review, especially because I wrote it the day before rofl. As you can see we don't do that very often grin (which is arguably a good point for an API innocent )

Come to think of it, I'm not sure it would even break the API if it did work, because then you'd still have two arguments of the same type. So whatever existing apps are passing in their MAVSDK binding would still work without issues I think.

What's the right approach to go about this? 😅

If i just rename the variables, users of MAVSDK with an existing app that uses this most likely won't notice. But then again the implementation does not change, so it should not matter at all.

Do you agree that we should just rename the variables from _m to _cm to match the implementation @JonasVautherin ?

[julianoes]

Whereas converting the values will be quiet, but programs will suddenly receive unexpected data sweat_smile

Sorry to join late here but if something is wrong it just needs to be fixed, in my opinion. So the field stays _m but now it's correct.

[potaito]

All good. I reverted all the changes in this PR related to that bug now that mavlink/MAVSDK#1971 was merged.

[potaito]

Thank youuu!