search

GobySoft / goby3

The Goby Underwater Autonomy Project Version 3
Other
24 stars 11 forks source link

Add support for Remote Helm version 5 in iver_driver #279

Closed jturner314-nrl closed 1 year ago

jturner314-nrl commented 1 year ago

This adds a required field to IverConfig to specify the vehicle version and makes the changes necessary for the Iver3 case. So far, the only difference we've found between Iver2 and Iver3 is the units for depth in the $OMS message.

According to Revision 5.3.3 (April 2019) of the Iver3 manual and confirmed by testing on an Iver3 vehicle, the Iver3 remote helm interface expects the depth value of the $OMS message to be in meters.

Revision 3.5 (Feb. 2010) of the Iver2 manual says that the depth value is expected to be in feet, so this appears to be a change between the Iver2 and Iver3 vehicles.

tsaubergine commented 1 year ago

It used to be feet on the Iver3 as well (which is what this driver was written for). We need to get to the bottom of when exactly this change was made to correctly implement it on our end. Seriously OceanServer ... why would you make this kind of API change...?

tsaubergine commented 1 year ago

Actually it looks like a change in Remote Helm major version 5. I thought I'd come across this before, and I did, and it's been fixed in Goby(2). Clearly I didn't forward port it to Goby3: https://github.com/GobySoft/goby/commit/430965ed0306d061a6059ee0762588ac735dcde0

jturner314-nrl commented 1 year ago

Ah, okay. Thanks for figuring out when the change occurred! I've updated the PR to be basically a duplicate of that Goby2 commit.

Edit: I also rebased onto the latest 3.0 branch to resolve merge conflicts.

jturner314-nrl commented 1 year ago

Note that the release notes should warn that the depth field now defaults to meters instead of feet (since the default value for remote_helm_version_major is 5 in the .proto file). Alternatively, we could make remote_helm_version_major be a required field so that users are notified of the change even if they don't read the release notes.

tsaubergine commented 1 year ago

Hi - Thanks. I made remote_helm_version_major required as I'd rather cause a minor inconvenience to people when upgrading than finding out their vehicle dives three times deeper than expected...

I also commented the IverConfig file (which shows up in --help or --example_cfg).

jturner314-nrl commented 1 year ago

I made remote_helm_version_major required as I'd rather cause a minor inconvenience to people when upgrading than finding out their vehicle dives three times deeper than expected...

Yeah, that seems like the best option. Thanks!