feat: converted velocity controller lqr to lifecycle node

[Q3rkses]

made velocity controller into a lifecycle node instead of a normal node for ease of use and to save memory/resources when using the controller in idle mode.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.63%. Comparing base (1adb624) to head (d93fe58).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #585      +/-   ##
==========================================
- Coverage   18.77%   18.63%   -0.14%     
==========================================
  Files          38       38              
  Lines        2376     2372       -4     
  Branches      426      426              
==========================================
- Hits          446      442       -4     
  Misses       1815     1815              
  Partials      115      115              

Flag	Coverage Δ
unittests	18.63% <100.00%> (-0.14%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...city_controller_lqr/velocity_controller_lqr_lib.py	76.85% <100.00%> (-0.75%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jorgenfj]

Sending operation mode as a string is cursed. Should define a vortex msg using enum types.

Is a custom LOS guidance message needed. Custom ros2 messages should only be created when needed. A simple vector3 could accomplish this task. When following the principle of separating business logic from application logic the correct transformation between the ros2 and internal type should be clear enough to follow.

Refactor to cpp pls...

[jorgenfj]

You should take a look at ownership for this project. Does both the controller and node need to own the LQR parameters? Also why has the controller gotten ownership of 'dt' and not the ros node?

[Andeshog]

Some comments 🤓

[Andeshog]

Topics got moved to auv_setup for centralization 🤓 or are you changing that structure?

[jorgenfj]

yes yes. centralized government

[Andeshog]

Make a simulator test (could be just to start up the node in the ros environment and make sure the transitions work, for instance) and add it in this list

[Andeshog]

Would it make more sense to declare the parameters in the constructor (since it only has to happen once) and use the configure callback to get them?

[Andeshog]

These need to be the same type as what they are in the config file. Currently they are Int. Suggest you stick to float, as all the others are also float.

[Andeshog]

The same goes for propulsion.thrusters.max

[Andeshog]

Needs default (or does it? 👀). Turns out you can declare parameters with a type instead of a type instead of passing a default value (makes sense since it uses the default value to deduce the type of the parameter), like this

from rcl_interfaces.msg import ParameterType, ParameterDescriptor
...
self.declare_parameter("inertia_matrix", descriptor=ParameterDescriptor(type=ParameterType.PARAMETER_DOUBLE_ARRAY))

The same applies to the rest of the parameter declarations too, like

self.declare_parameter("topics.your_topic", descriptor=ParameterDescriptor(type=ParameterType.PARAMETER_STRING))

Personally this clings better than zeros and "_" as defaults 🤷

[Q3rkses]

what do you think about defining descriptor=ParameterDescriptor(type=ParameterType.PARAMETER_STRING) as just PARAMETER_STRING maybe in a dataclass or someplace else? Then simply using PARAMETER_STRING so it doesnt take up 90 spaces in one line

[Andeshog]

Better yet, using Parameter from rclpy.parameter just solves this "problem".

from rclpy.parameter import Parameter

self.declare_parameter("LQR_params.q_surge", Parameter.Type.INTEGER)
...
q_surge = self.get_parameter("LQR_params.q_surge").value

If self.get_parameter fails, and you want to handle it, you could use ParameterUninitializedException from rclpy.node to handle it.

[Andeshog]

Resolving these comments should make your node be able to activate, however looks like there could be other issues waiting for you then 🤐

[Andeshog]

Make a simulator test (could be just to start up the node in the ros environment and make sure the transitions work, for instance) and add it in this list

Added standalone node testing, which is more fitting for this case, so write a simple script where the node is launched and transitioned to activated, and test that it is able to correctly publish wrench_input (for example). Add the script path to this list: https://github.com/vortexntnu/vortex-auv/blob/main/.github/workflows/ros-node-tests.yml#L17-L19. Suggest that when LOS is consistently performing as expected (tuned or just improved in other ways), the LOS - velocity controller integration test can be reopened.