send signal_quality via mavlink #34

Williangalvani · 2023-09-21T13:26:47Z

Fixes #32

ES-Alexander

Seems ok in principle, although we may need to check how ArduSub handles it (especially in older versions) - IIRC we were initially reporting a confidence value but switched to a return for the no-confidence case because ArduSub was ignoring the confidence value and treating it as a valid reading, but it's quite possible I'm just misremembering / mixing this up with something else.

dvl-a50/mavlink2resthelper.py

Williangalvani · 2023-09-21T21:33:52Z

Seems ok in principle, although we may need to check how ArduSub handles it (especially in older versions) - IIRC we were initially reporting a confidence value but switched to a return for the no-confidence case because ArduSub was ignoring the confidence value and treating it as a valid reading, but it's quite possible I'm just misremembering / mixing this up with something else.

This will be required once we have ArduPilot/ardupilot#24418 and ArduPilot/ardupilot#23435

I'm not sure if we should check the ardusub version to enable the signal quality, though...

ES-Alexander · 2023-09-23T17:43:07Z

This will be required once we have ArduPilot/ardupilot#24418 and ArduPilot/ardupilot#23435

Yes, I think it's good to include, even just because it's part of the established MAVLink message specification. That said, it's a problem if doing so reduces performance for currently supported vehicle firmwares.

Speaking of, we'll also need to check/update this for the Ping Sonar integration in BlueOS.

I'm not sure if we should check the ardusub version to enable the signal quality, though...

Perhaps a more robust solution would be to add a toggle switch to the UI that determines whether or not to send MAVLink messages for zero (/low?) confidence distance measurements, with an info hover thingo that informs it should be turned off for firmwares that don't consider the confidence value (and an "e.g. ArduSub <= 4.x.y")?

That would be a bit more development work, but fixes the problem in a more general way, and avoids breaking compatibility with other firmwares that we don't have suitable feature support information for.

ES-Alexander · 2023-09-23T17:46:57Z

dvl-a50/mavlink2resthelper.py

+            data = self.rangefinder_template.format(0, 0)
+        else:
+            data = self.rangefinder_template.format(int(distance * 100), int(quality))


We likely need to limit the minimum quality to 1, to avoid a repeat of bluerobotics/BlueOS#1862

Good catch @ES-Alexander !

ES-Alexander

A few small changes and comments/questions.

More generally, did anyone end up checking whether this will cause issues with old versions of ArduSub, or is the idea that it's resolved by not sending low confidence messages using the cutoff?

ES-Alexander · 2024-02-16T03:08:09Z

dvl-a50/dvl.py

@@ -62,6 +63,7 @@ class DvlDriver(threading.Thread):
    last_temperature_check_time = 0
    temperature_check_interval_s = 30
    temperature_too_hot = 45
+    confidence_cutoff = 0


I'm a bit confused as to why this is 0 if the default is 80 - I think it could make more sense to specify a meaningful value here and then use that as the fallback when loading settings.

ES-Alexander · 2024-02-16T03:11:44Z

dvl-a50/dvl.py

+            if confidence >= self.confidence_cutoff:
+                self.mav.send_rangefinder(alt, confidence)


Could this not be another and in the line before, and drop an indentation level?

ES-Alexander · 2024-02-16T03:14:21Z

dvl-a50/main.py

@@ -35,6 +35,12 @@ def set_enabled(self, enabled: str) -> bool:
            return self.dvl.set_enabled(enabled == "true")
        return False

+    def set_cutoff(self, cutoff: int) -> bool:
+        """
+        Enables/Disables the DVL driver


Suggested change

Enables/Disables the DVL driver

Sets the required confidence for sending rangefinder distance estimates.

ES-Alexander · 2024-02-16T03:18:00Z

dvl-a50/mavlink2resthelper.py

@@ -152,7 +152,7 @@ def __init__(self, vehicle: int = 1, component: int = 1):
    "type": "DISTANCE_SENSOR",
    "time_boot_ms": 0,
    "min_distance": 0,
-    "max_distance": 5000,
+    "max_distance": 50000,


If possible it would be helpful to specify the unit here in a comment, and some reasoning behind the value, but ideally the number would be settable from the frontend and/or dynamically determined by the device type or something (rather than hardcoding something arbitrary).

Hmmm... the correct value for the A50 is 5000 cm, and for the A125 is 12500 cm. Maybe there's a way to get the model # from the data stream? In any case 50000 cm seems too high.

The autopilot code is using min(DISTANCE_SENSOR.max_distance, RNGFNDx_MAX_CM), so the pilot can control the max and min using the RNGFNDx parameters. Same with the minimum. So I don't see huge value in letting the pilot set this value. https://github.com/ArduPilot/ardupilot/blob/9986fb972635016171235750a465adc0d0174ba0/libraries/AP_RangeFinder/AP_RangeFinder_MAVLink.cpp#L48

P.S. the default RNGFNDx_MAX_CM is 700 cm, so the pilot will need to tweak this parameter to get RNG_HOLD to work > 7m above the seafloor.

ES-Alexander · 2024-02-16T03:23:23Z

dvl-a50/static/index.html

@@ -130,10 +136,17 @@ <h2>Water Linked DVL Configuration Page</h2>
 				orientationOptions: {
 					"Downwards (LED pointing forward)": 1,
 					"Forward (Experimental)": 2,
-				}
+				},
+				confidenceCutoff: 10,


Should this perhaps match the DvlDriver default? Or is it immediately overwritten by fetching from the backend or something? It would be weird if the displayed value doesn't match the internal one.

patrickelectric · 2024-02-16T04:19:44Z

dvl-a50/dvl.py

@@ -272,6 +277,14 @@ def set_enabled(self, enable: bool) -> bool:
        self.save_settings()
        return True

+    def set_confidence_cutoff(self, cutoff: int) -> bool:


does it make sense to return a value at all ?

patrickelectric · 2024-02-16T04:21:08Z

dvl-a50/mavlink2resthelper.py

@@ -352,12 +352,12 @@ def send_vision_position_estimate(
        )
        logger.info(post(MAVLINK2REST_URL + "/mavlink", data=data))

-    def send_rangefinder(self, distance: float):
+    def send_rangefinder(self, distance: float, quality: float):


quality_percentage ?

clydemcqueen · 2024-02-17T17:24:50Z

More generally, did anyone end up checking whether this will cause issues with old versions of ArduSub, or is the idea that it's resolved by not sending low confidence messages using the cutoff?

Hi, @ES-Alexander -- signal_quality was added to Rangefinder drivers in this fairly recent PR: ArduPilot/ardupilot#25550

So no releases prior to Nov 2023 will be affected by setting signal_quality to something other than 0.

To make this 100% backward-compatible you could default the confidence cutoff to 0 so all messages get sent even if the confidence is very low. That way they are logged in tlog and BIN logs, they flow from the autopilot to the GCS so the pilot can see them, etc. But I can see the value in setting the confidence to something like 80 as well.

Edit: SURFTRAK PR has a RNGFND_SQ_MIN parameter, defaults to 90.

ES-Alexander · 2024-02-29T19:24:08Z

To make this 100% backward-compatible you could default the confidence cutoff to 0 so all messages get sent even if the confidence is very low. That way they are logged in tlog and BIN logs, they flow from the autopilot to the GCS so the pilot can see them, etc. But I can see the value in setting the confidence to something like 80 as well.

If I'm remembering correctly there were issues with using the Ping Sonar as a rangefinder when low/zero quality messages were being sent, so having a reasonable threshold in the code here should prevent a similar issue when using the DVL with older ArduSub versions.

That's the only issue I remember, so I think this is likely fine (and setting a non-zero default confidence cutoff is likely advisable) - I just thought it was probably worth checking whether there are any obvious issues to providing the value to firmwares that previously weren't receiving it, but if they're just completely ignoring it then it shouldn't have an effect.

clydemcqueen · 2024-10-11T17:59:57Z

I pushed this PR to my dockerhub for testing later today, and I'm seeing errors on my bench Pi running BlueOS 1.3.1:

2024-10-11 17:56:06.200 | DEBUG    | dvl:report_status:77 - waiting for cable-guy to come online...
2024-10-11 17:56:07.203 | WARNING  | blueoshelper:request:13 - Error in request: http://127.0.0.1/cable-guy/v1.0/ethernet: <urlopen error [Errno 111] Connection refused>
2024-10-11 17:56:07.204 | DEBUG    | dvl:report_status:77 - waiting for cable-guy to come online...
2024-10-11 17:56:08.207 | WARNING  | blueoshelper:request:13 - Error in request: http://127.0.0.1/cable-guy/v1.0/ethernet: <urlopen error [Errno 111] Connection refused>
...

Does this PR need a rebase?

Williangalvani · 2024-10-11T20:24:04Z

@clydemcqueen rebased! I can`t test it right now, though

clydemcqueen · 2024-10-12T15:01:47Z

@Williangalvani Thanks! We have a test scheduled for the week of 20-Oct, so hopefully I'll finally get this done then. :-)

Williangalvani requested review from patrickelectric and ES-Alexander September 21, 2023 13:26

ES-Alexander approved these changes Sep 21, 2023

View reviewed changes

dvl-a50/mavlink2resthelper.py Outdated Show resolved Hide resolved

Williangalvani force-pushed the signal_quality branch from b7f01ab to fa447d9 Compare September 21, 2023 21:27

ES-Alexander requested changes Sep 23, 2023

View reviewed changes

clydemcqueen mentioned this pull request Oct 11, 2023

DISTANCE_SENSOR.signal_quality is always 0 #32

Open

Williangalvani force-pushed the signal_quality branch 4 times, most recently from dadaddd to b31c6da Compare February 16, 2024 00:40

Williangalvani requested a review from ES-Alexander February 16, 2024 00:40

ES-Alexander requested changes Feb 16, 2024

View reviewed changes

patrickelectric reviewed Feb 16, 2024

View reviewed changes

Williangalvani added 3 commits October 11, 2024 17:20

send signal_quality via mavlink

2a6016e

expose confidence cutoff api

61d962c

increase max range in DISTANCE_SENSOR message template

dbdb04a

Williangalvani force-pushed the signal_quality branch from b31c6da to 12f67b9 Compare October 11, 2024 20:21

update frontend to handle confidence cutoff

5f497aa

Williangalvani force-pushed the signal_quality branch from 12f67b9 to 5f497aa Compare October 11, 2024 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send signal_quality via mavlink #34

send signal_quality via mavlink #34

Williangalvani commented Sep 21, 2023 •

edited by ES-Alexander

Loading

ES-Alexander left a comment

Williangalvani commented Sep 21, 2023

ES-Alexander commented Sep 23, 2023

ES-Alexander Sep 23, 2023

clydemcqueen Oct 11, 2023

ES-Alexander left a comment •

edited

Loading

ES-Alexander Feb 16, 2024

ES-Alexander Feb 16, 2024

ES-Alexander Feb 16, 2024

ES-Alexander Feb 16, 2024

clydemcqueen Feb 17, 2024

ES-Alexander Feb 16, 2024

patrickelectric Feb 16, 2024

patrickelectric Feb 16, 2024

clydemcqueen commented Feb 17, 2024 •

edited

Loading

ES-Alexander commented Feb 29, 2024

clydemcqueen commented Oct 11, 2024

Williangalvani commented Oct 11, 2024

clydemcqueen commented Oct 12, 2024

		if confidence >= self.confidence_cutoff:
		self.mav.send_rangefinder(alt, confidence)

	Enables/Disables the DVL driver
	Sets the required confidence for sending rangefinder distance estimates.

send signal_quality via mavlink #34

Are you sure you want to change the base?

send signal_quality via mavlink #34

Conversation

Williangalvani commented Sep 21, 2023 • edited by ES-Alexander Loading

ES-Alexander left a comment

Choose a reason for hiding this comment

Williangalvani commented Sep 21, 2023

ES-Alexander commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ES-Alexander left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clydemcqueen commented Feb 17, 2024 • edited Loading

ES-Alexander commented Feb 29, 2024

clydemcqueen commented Oct 11, 2024

Williangalvani commented Oct 11, 2024

clydemcqueen commented Oct 12, 2024

Williangalvani commented Sep 21, 2023 •

edited by ES-Alexander

Loading

ES-Alexander left a comment •

edited

Loading

clydemcqueen commented Feb 17, 2024 •

edited

Loading