chore: move position correction code to lib #8

eduardacoppo · 2025-01-23T12:06:29Z

Depends on:

What is this PR for

[sc-67374]
This PR moves the position correction code here

How I did it

Moved the code that was previously in drivers/orogen/nmea0183 and made some improvements in the code.

Results, How I tested

Unit tests

Checklist

Assign yourself in the PR
Write unit tests (when relevant)
Run rubocop and rake tests locally

and make some improvements in the code.

src/AIS.cpp

use std::string instead of constexpr char and fix tests

src/AIS.hpp

src/AIS.cpp

test/test_AIS.cpp

and log the message directly instead of saving it to a variable

and fix wrong documentation about it

src/AIS.cpp

and avoid using null values in converter

doudou

Style: https://docs.google.com/document/d/16zBGRa58zu7wgRBcGb6IVVQtCfiDqDDhu4vjUVnVSu0/edit?tab=t.0#heading=h.kagqk4knrk4k

Please fix those ... there's quite a few.

src/AIS.cpp

floating point types are already implicitly converted into double, if necessary

pass utm_converter as const& and write const& instead of const <TYPE>&, for consistency. Pass position as std::optional

because they're expected behaviors, we shouldn't treat them as errors

src/AIS.cpp

doudou · 2025-02-04T14:27:03Z

Documentation is necessary when the code is not sufficiently expressive, and rewriting function signature in plain english is not documentation for me and I think it is just noisy. It is not a complaint about you specifically, I see this practice in other team members too. Just using this opportunity to raise this discussion

@wvmcastro agreed. I'm complaining about this regularly on reviews too.

doudou

All agreed with Wellington. I think you're good after that and that having both me, jhonas and wellington is a bit too much. I'm accepting the PR to get myself out of the loop.

src/AIS.cpp

wvmcastro

Also, write a test that assures the applyPositionCorrection output time field is equal to the one of position parameter

src/AIS.cpp

Having "_gps" as a suffix for the sensor and vessel positions in GPS coordinates was intended to distinguish GPS coordinates from UTM ones. That was a confusing nomenclature, but as I had no better idea, I just went on with it. After discussing the matter, I agreed naming these variables simply "sensor_pos" and "vessel_pos" were a satisfactory way to make this distinction, as the type "ais_base::Position" is known to have coordinates in the GPS system. So this solved the nomenclature problem. Part of the transformation to get the vessel position in the world frame was left inside the "convertUTMToGPS" function. This was fixed by moving it to the now called "computeVesselPositionInWorldFrame".

and rename vessel2world_gps to vessel_pos, as is done in the code

src/AIS.cpp

src/AIS.hpp

src/AIS.cpp

test/test_AIS.cpp

wvmcastro · 2025-02-07T13:09:00Z

Approving it since it does not have nothing actually wrong, just some minor enhancements

chore: set cpp17 version in CMakeLists

87d41c1

eduardacoppo self-assigned this Jan 23, 2025

eduardacoppo mentioned this pull request Jan 23, 2025

feat: make position in world to be the vessels tidewise/drivers-orogen-nmea0183#10

Merged

5 tasks

eduardacoppo force-pushed the position_correction branch from 389fdd3 to 6e295cc Compare January 23, 2025 13:53

chore: move position correction code to lib

ae7baa6

and make some improvements in the code.

eduardacoppo force-pushed the position_correction branch from 6e295cc to ae7baa6 Compare January 23, 2025 14:09

eduardacoppo added 2 commits January 23, 2025 12:57

chore: make speed threshold configurable

f7a3fde

test: add tests and LOG_ERROR messages

f770128

eduardacoppo requested review from doudou, seixasxbr and jhonasiv January 23, 2025 18:55

jhonasiv reviewed Jan 27, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

wvmcastro reviewed Jan 27, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

fix: do not throw error if no correction

cbf14f3

use std::string instead of constexpr char and fix tests

eduardacoppo requested review from wvmcastro and jhonasiv January 28, 2025 16:04

wvmcastro reviewed Jan 28, 2025

View reviewed changes

eduardacoppo added 2 commits January 28, 2025 15:01

chore: use getRad() when checking for nan

defb0b6

and log the message directly instead of saving it to a variable

chore: change variable name to match convention

4c8e3b4

and fix wrong documentation about it

jhonasiv reviewed Jan 28, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

fix: ref position relative to the vessel's center

9d1e750

and avoid using null values in converter

eduardacoppo force-pushed the position_correction branch from 5f9fc60 to 9d1e750 Compare January 28, 2025 20:11

doudou reviewed Jan 29, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

eduardacoppo added 4 commits January 29, 2025 14:28

chore: give speed thresh a clearer name

8752944

fix: remove static_cast for ref pos elements

abb2680

floating point types are already implicitly converted into double, if necessary

fix: fix passing arguments,

22bb4d3

pass utm_converter as const& and write const& instead of const <TYPE>&, for consistency. Pass position as std::optional

chore: change log type to debug instead of error

aa9795f

because they're expected behaviors, we shouldn't treat them as errors

eduardacoppo requested review from doudou, wvmcastro and jhonasiv January 30, 2025 15:22

seixasxbr reviewed Jan 30, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

seixasxbr approved these changes Jan 30, 2025

View reviewed changes

chore: remove unnecessary suffix in sensor2vessel_pos

d33ceec

jhonasiv reviewed Jan 30, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

eduardacoppo requested a review from jhonasiv January 30, 2025 18:43

wvmcastro reviewed Jan 30, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

seixasxbr self-requested a review January 31, 2025 12:44

doudou approved these changes Feb 4, 2025

View reviewed changes

doudou reviewed Feb 4, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.cpp Outdated Show resolved Hide resolved

eduardacoppo added 2 commits February 4, 2025 13:37

fix/chore: fix passing arguments and refactor code

bb37d99

fix: write more robust tests and add tests

926f3f7

eduardacoppo force-pushed the position_correction branch from 062f045 to 926f3f7 Compare February 4, 2025 16:39

eduardacoppo requested a review from wvmcastro February 5, 2025 13:31

eduardacoppo commented Feb 5, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

eduardacoppo added 2 commits February 5, 2025 11:07

chore: use snake_case for variables

6bfcd04

chore: propagate status

dd6fb6d

wvmcastro reviewed Feb 5, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

eduardacoppo added 2 commits February 5, 2025 15:16

test: add checks for timestamp in the tests

899444b

and rename vessel2world_gps to vessel_pos, as is done in the code

eduardacoppo force-pushed the position_correction branch from 09e37e3 to 899444b Compare February 5, 2025 18:17

eduardacoppo requested a review from wvmcastro February 7, 2025 11:19

wvmcastro approved these changes Feb 7, 2025

View reviewed changes

src/AIS.cpp Outdated Show resolved Hide resolved

src/AIS.hpp Show resolved Hide resolved

src/AIS.hpp Outdated Show resolved Hide resolved

src/AIS.cpp Show resolved Hide resolved

test/test_AIS.cpp Outdated Show resolved Hide resolved

test/test_AIS.cpp Show resolved Hide resolved

chore: minor fixes

3541563

jhonasiv approved these changes Feb 11, 2025

View reviewed changes

eduardacoppo merged commit 41c6fdc into master Feb 13, 2025
1 check passed

eduardacoppo deleted the position_correction branch February 13, 2025 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move position correction code to lib #8

chore: move position correction code to lib #8

eduardacoppo commented Jan 23, 2025 •

edited

Loading

doudou left a comment

doudou commented Feb 4, 2025

doudou left a comment

wvmcastro left a comment

wvmcastro commented Feb 7, 2025

chore: move position correction code to lib #8

chore: move position correction code to lib #8

Conversation

eduardacoppo commented Jan 23, 2025 • edited Loading

What is this PR for

How I did it

Results, How I tested

Checklist

doudou left a comment

Choose a reason for hiding this comment

doudou commented Feb 4, 2025

doudou left a comment

Choose a reason for hiding this comment

wvmcastro left a comment

Choose a reason for hiding this comment

wvmcastro commented Feb 7, 2025

eduardacoppo commented Jan 23, 2025 •

edited

Loading