Merge DVL into master, Fix Bug

ES-Alexander commented 2 years ago

I believe I did this correctly, but I don't think I've had to merge a rebase before.

[x] Spoke with @Williangalvani and turns out there was an existing bug in dvl.py that allowed invalid VISION_POSITION_DELTA messages to be sent. Apparently ArduSub still consumes data that has a bad confidence, so it's better to avoid sending them at all. https://github.com/bluerobotics/companion/pull/425/commits/eabcc7aacca9cdb38eeb6133da8460d384e6764c fixes that bug.
[x] The rangefinder reading option is used by topside observers rather than the internal EKF, so it can be useful to send a message that the latest reading was invalid. Have set it to send a 0 range, which is below the minimum of 5cm for the WL DVL, so should hopefully show up in QGC as --.--, which is preferable to just a really stable value from the last valid reading. Included in eabcc7a.
~There may be an unrelated issue with the rangefinder being sent in m instead of cm, as described here.~

Williangalvani commented 2 years ago

looks good. the mavlink2rest change could cause issues elsewhere. we need to check where else it is used in the code and make sure everything is ok. IIRC there was a significant change from 0.7 to 0.8.

ES-Alexander commented 2 years ago

@Williangalvani 29 and 30 haven't changed anything related to mavlink2rest (they're just the mirrordirector -> legacy and the stop ssl certificate checking for pixhawk firmware downloads updates), so any issues with that will already be in the existing dvl branch. Perhaps still worth checking, but they at least won't be new issues (at least not since August when we rebased dvl onto 0.0.28) :-)

If you end up doing some testing, I believe the dvl-30 branch should be identical to the result of this PR, so best to test there :-)

ES-Alexander commented 2 years ago

I've just noticed that the WaterLinked dvl-protocol page says that the returned altitude is measured in meters, but the DISTANCE_SENSOR mavlink message (used for rangefinder) reckons it wants distances in cm ~(and we just pass the alt value in directly). Not sure if the WL docs are incorrect or something, or if that's likely to cause issues if it is actually a bug, but perhaps worth noting (and fixing if relevant).~

Correction: we pass the distance (in m) directly to the send_rangefinder method of Mavlink2RestHelper, which does the conversion from m to cm, so not an issue.

bluerobotics / companion

Merge DVL into master, Fix Bug #425