Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

436 task guidance node for autopilot #502

Open
wants to merge 54 commits into
base: develop
Choose a base branch
from

Conversation

AbubakarAliyuBadawi
Copy link

No description provided.

abubakaraliyubadawi and others added 23 commits October 9, 2024 18:51
…ilot' into 436-task-guidance-node-for-autopilot
* ✨ feat: added the submodule for the perception

* ci: update clang-formatter and add python-formatter

* ci: explicitly checkout branch for black-format commit

* Committing black-format changes

* chore(pylint): add .pylintrc configuration file

* refactor: fix pylint warning W1510

* refactor: fix pylint warning C0116

* refactor: fix pylint warning C0411

* refactor: fix pylint warning R1731

* refactor: fix pylint warning R1705

* refactor: fix pylint warning W1514

* refactor: fix pylint warning C0200

* refactor: fix pylint warning W0611

* refactor: fix pylint warning W0702

* refactor: fix pylint warning C0121

* refactor: fix pylint warning W0107

* refactor: fix pylint warning R1714

* refactor: fix pylint warning C0201

* refactor: fix pylint warning C0303 and W0613

* refactor: fix pylint warning W0621

* refactor: fix pylint warning R0402

* refactor: fix pylint warning W0212

* chore(pylint): update rule adjustments and formatting

* style: format code with black

* style: format imports with isort

* ci: add isort and pylint to Python pipeline

* Committing black-format changes

* chore: add pyproject.toml for project configuration

* ci: update order of pipeline jobs

* ci: update pylint job and pylint rules

* ci: update which python versions pylint test

* ci: update python pipeline to only run on pull_request

* refactor: fix pylint warning C0103

* chore: update linting and project configuration rules in .pylintrc and pyproject.toml

* Rename files to snake_case

* ci: add pipeline for grammar in comments

* refactor: fix spelling mistakes in comments

* ci: update job codespell_fix to continue on error

* refactor: fix spelling mistakes in comments

* ci: update pipeline codespell to only have one job

* refactor: fix variable naming issues and adjust imports as per review

* Committing codespell fixes

* refactor: spelling correction

* ci: update CI file and job names for consistency

* ci: update CI file for more clarity

* chore: add codespell configuration and ignore list

* chore: add type hints and return types

* ci: add mypy.ini for type checking configuration

* ci: add mypy type checking workflow

* refactor: format code with black

* refactor: format all yaml files using prettier

* ci: add pipeline for yaml formatting using prettier

* ci: update yaml formatting pipeline to run on pull request

* refactor: remove deprecated typing

* refactor: fix import sorting

* ci: remove faulty ci pipelines and combine python pipelines into single file

* fix(security): replace subprocess call with safer alternative to remove shell=true

* feat: add pre-commit hooks for black, isort, and codespell

* ci: update python pipeline to use latest versions

* refactor: apply code formatting fixes via pre-commit hooks

* ci: split python ci/cd pipelines into multiple files

* ci: add ci pipeline for checking style clang-format

* refactor: remove unused config file for clang-format

* refactor: remove duplicate file

* ci: add ci pipeline that tests that codebase can build

* refactor: update ci-build pipeline to only run on pull request

* refactor: update clang-format config

* refactor: apply clang-format

* feat: update pipeline to build and run tests

* refactor: disable linting checks when running colcon test

* refactor: update .pre-commit-config file (#480)

* ci: add semver.yaml pipeline and .releaserc configuration

* ci: add semver.yaml pipeline and .releaserc configuration

* fix: semver pipeline runs when push to main

---------

Co-authored-by: Yauhen <[email protected]>
Co-authored-by: Black Robot <[email protected]>
Co-authored-by: Codespell Robot <[email protected]>
Co-authored-by: Sondre Haugen <[email protected]>
Copy link
Contributor

@Andeshog Andeshog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a start. Made some general comments. Will go more in-depth after you make these adjustments.

guidance/guidance_los/package.xml Outdated Show resolved Hide resolved
guidance/guidance_los/package.xml Outdated Show resolved Hide resolved
guidance/guidance_los/package.xml Outdated Show resolved Hide resolved
guidance/guidance_los/setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kluge7 kluge7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix the changes flagged by the pre-commit hooks! To solve them you need to open a terminal and run:
pip install pre-commit
and then navigate to the vortex-auv directory:
cd vortex-auv
and then run pre-commit hooks on all the files:
pre-commit run --all-files

It will fix some things automatically, but others you have to configure manually! Send me a message if anything is unclear:)

@kluge7 kluge7 linked an issue Nov 11, 2024 that may be closed by this pull request
3 tasks
mission/gui_auv/package.xml Outdated Show resolved Hide resolved
guidance/guidance_los/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Andeshog Andeshog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General:

  • Make README for the package
  • Avoid unnecessary comments
  • Add a config directory and a yaml file that holds the necessary parameters

guidance/guidance_los/CMakeLists.txt Outdated Show resolved Hide resolved
guidance/guidance_los/CMakeLists.txt Show resolved Hide resolved
guidance/guidance_los/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Andeshog Andeshog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a general note, if the comments are just repeating what the code says, then remove them.

guidance/guidance_los/config/los_guidance_params.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Andeshog Andeshog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean up the redundant comments in the los_guidance_action_server.py file as well :)

Comment on lines 26 to 30
def init_filter_parameters(self):
"""Initialize filter state and parameters."""
self.x = np.zeros(9)
self.omega = np.diag(config_dict['filter']['omega_diag'])
self.zeta = np.diag(config_dict['filter']['zeta_diag'])
self.omega = np.diag(self.config_dict['filter']['omega_diag'])
self.zeta = np.diag(self.config_dict['filter']['zeta_diag'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this, consider using a dataclass instead of dictionary. FilterParams would be a fitting name for the class.

Comment on lines 273 to 278
def publish_guidance(self, commands):
"""Publish guidance commands for vehicle control."""
msg = LOSGuidance()
msg.surge = commands[0]
msg.pitch = commands[1]
msg.yaw = commands[2]
msg.surge = commands[0] # surge: forward velocity command
msg.pitch = commands[1] # pitch: pitch angle command
msg.yaw = commands[2] # yaw: yaw angle command
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont feel like the comments add much here. What I meant was to use typehint for the commands input, a more fitting structure (like the state dataclass), and perhaps a quick description in the docstring.

So something like this

def publish_guidance(self, commands: State):
    """Publish the commanded surge velocity, pitch angle, and yaw angle."""
    msg = LOSGuidance()
    msg.surge = commands.surge_vel
    msg.pitch = commands.pitch
    msg.yaw = commands.yaw

Note that in this example I added surge velocity to the State dataclass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will require me to modify how I created the commands in the process_guidance method as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this could work

def compute_raw_los_guidance(
        self, current_pos: State, target_pos: State
    ) -> State:
        
        dx = target_pos.x - current_pos.x
        dy = target_pos.y - current_pos.y

        horizontal_distance = np.sqrt(dx**2 + dy**2)
        desired_yaw = np.arctan2(dy, dx)
        yaw_error = self.ssa(desired_yaw - current_pos.yaw)

        depth_error = target_pos.z - current_pos.z
        desired_pitch = self.compute_pitch_command(depth_error, horizontal_distance)

        desired_surge = self.compute_desired_speed(yaw_error, horizontal_distance)

        commands = State(surge_vel=desired_surge, pitch=desired_pitch, yaw=desired_yaw)

        return commands
def apply_reference_filter(self, commands: State) -> State:
        x_dot = self.Ad @ self.x + self.Bd @ commands.as_los_array()
        self.x = self.x + x_dot * self.dt
        return State(surge_vel=self.x[0], pitch=self.x[1], yaw=self.x[2])

def compute_guidance(
        self, current_pos: State, target_pos: State
    ) -> State:
        raw_commands = self.compute_raw_los_guidance(current_pos, target_pos)

        filtered_commands = self.apply_reference_filter(raw_commands)
        filtered_commands.pitch = self.ssa(filtered_commands.pitch)
        filtered_commands.yaw = self.ssa(filtered_commands.yaw)
        return filtered_commands

where the State class is modified to

@dataclass
class State:
    x: float = 0.0
    y: float = 0.0
    z: float = 0.0
    pitch: float = 0.0
    yaw: float = 0.0
    surge_vel: float = 0.0

    def __add__(self, other: "State") -> "State":
        return State(
            x=self.x + other.x,
            y=self.y + other.y,
            z=self.z + other.z,
            pitch=self.pitch + other.pitch,
            yaw=self.yaw + other.yaw,
        )

    def __sub__(self, other: "State") -> "State":
        return State(
            x=self.x - other.x,
            y=self.y - other.y,
            z=self.z - other.z,
            pitch=self.pitch - other.pitch,
            yaw=self.yaw - other.yaw,
        )
    
    def as_los_array(self):
        return np.array([self.surge_vel, self.pitch, self.yaw])

Comment on lines 289 to 291
msg.pose.position.x = target[0] # x: x-coordinate of the target point
msg.pose.position.y = target[1] # y: y-coordinate of the target point
msg.pose.position.z = target[2] # z: z-coordinate of the target point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you utilize Point for target (thus, generally for waypoints), this could be simplified to

def publish_reference(self, target: Point):
    ...
    msg.pose.position = target

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I change this I might need to modify how waypoints are stored and processed, right now they are stored as a numpy array rather than points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and it is probably better to just do it straight away since waypoints will be sent and received through a topic as Points in the full system.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finish the README

Comment on lines 41 to 47
def get_omega_matrix(self) -> np.ndarray:
"""Get omega diagonal matrix."""
return np.diag(self.omega_diag)

def get_zeta_matrix(self) -> np.ndarray:
"""Get zeta diagonal matrix."""
return np.diag(self.zeta_diag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not needed. You can just access omega and zeta by

filter_params = FilterParameters()
omega = filter_params.omega_diag
zeta = filter_parameters.zeta_diag

Comment on lines 56 to 80
def __init__(self, config_dict: dict):
self.config_dict = config_dict
self.init_los_parameters()
self.init_filter_parameters()
self.setup_filter_matrices()

def init_los_parameters(self):
"""Initialize Line-of-Sight guidance parameters."""
self.los_params = LOSParameters(
lookahead_distance_min=self.config_dict['h_delta_min'],
lookahead_distance_max=self.config_dict['h_delta_max'],
lookahead_distance_factor=self.config_dict['h_delta_factor'],
nominal_speed=self.config_dict['nominal_speed'],
min_speed=self.config_dict['min_speed'],
max_pitch_angle=self.config_dict['max_pitch_angle'],
depth_gain=self.config_dict['depth_gain'],
)

def init_filter_parameters(self):
"""Initialize filter state and parameters."""
self.x = np.zeros(9) # Filter state
self.filter_params = FilterParameters(
omega_diag=np.array(self.config_dict['filter']['omega_diag']),
zeta_diag=np.array(self.config_dict['filter']['zeta_diag']),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you have the two dataclasses for the parameters, you can just use the objects directly

def __init__(self, los_params: LOSParameters, filter_params: FilterParameters):
    self.los_params = los_params
    self.filter_params = filter_params
    self.setup_filter_matrices()

Comment on lines 86 to 104
def setup_filter_matrices(self):
self.Ad = np.zeros((9, 9))
self.Bd = np.zeros((9, 3))

# Fill Ad matrix blocks
self.Ad[0:3, 3:6] = np.eye(3)
self.Ad[3:6, 6:9] = np.eye(3)

# Compute characteristic polynomial coefficients
omega_cubed = self.omega @ self.omega @ self.omega
omega_squared = self.omega @ self.omega

# Fill in the acceleration dynamics
self.Ad[6:9, 0:3] = -omega_cubed
self.Ad[6:9, 3:6] = -(2 * self.zeta + np.eye(3)) @ omega_squared
self.Ad[6:9, 6:9] = -(2 * self.zeta + np.eye(3)) @ self.omega

# Input matrix
self.Bd[6:9, :] = omega_cubed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a one time call, you could extract omega and zeta at the top

def setup_filter_matrices(self):
        omega = np.diag(self.filter_params.omega_diag)
        zeta = np.diag(self.filter_params.zeta_diag)
        omega_cubed = omega @ omega @ omega
        omega_squared = omega @ omega

        self.Ad = np.zeros((9, 9))
        self.Bd = np.zeros((9, 3))

        self.Ad[0:3, 3:6] = np.eye(3)
        self.Ad[3:6, 6:9] = np.eye(3)
        self.Ad[6:9, 0:3] = -omega_cubed
        self.Ad[6:9, 3:6] = -(2 * zeta + np.eye(3)) @ omega_squared
        self.Ad[6:9, 6:9] = -(2 * zeta + np.eye(3)) @ omega

        self.Bd[6:9, :] = omega_cubed

Comment on lines 20 to 26
lookahead_distance_min: float = 2.0
lookahead_distance_max: float = 8.0
lookahead_distance_factor: float = 1.0
nominal_speed: float = 1.0
min_speed: float = 0.1
max_pitch_angle: float = 0.5 # ~28.6 degrees
depth_gain: float = 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the update period dt here as well.

import numpy as np
import rclpy
from geometry_msgs.msg import PoseStamped, Vector3Stamped
from guidance_los.los_guidance_algorithm import ThirdOrderLOSGuidance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to also import the parameter classes.

Comment on lines 87 to 103
def _get_los_parameters(self) -> dict:
"""Get all LOS parameters and return them as a dictionary."""
return {
'h_delta_min': self.get_parameter('los_guidance.h_delta_min').value,
'h_delta_max': self.get_parameter('los_guidance.h_delta_max').value,
'h_delta_factor': self.get_parameter('los_guidance.h_delta_factor').value,
'nominal_speed': self.get_parameter('los_guidance.nominal_speed').value,
'min_speed': self.get_parameter('los_guidance.min_speed').value,
'max_pitch_angle': self.get_parameter('los_guidance.max_pitch_angle').value,
'depth_gain': self.get_parameter('los_guidance.depth_gain').value,
'filter': {
'omega_diag': self.get_parameter(
'los_guidance.filter.omega_diag'
).value,
'zeta_diag': self.get_parameter('los_guidance.filter.zeta_diag').value,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into two methods, where one creates the los_params object and one creates the filter_params object.

Comment on lines 209 to 223
# Initialize filter on first callback
if not self.filter_initialized:
initial_commands = np.array([0.0, 0.0, yaw])
self.guidance_calculator.reset_filter_state(initial_commands)
self.filter_initialized = True

# Log current position for debugging
self.publish_log(
f"Position: x={self.state.x:.3f}, "
f"y={self.state.y:.3f}, "
f"z={self.state.z:.3f}"
)
self.publish_log(
f"Orientation: roll={roll:.3f}, pitch={pitch:.3f}, yaw={yaw:.3f}"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a

if not self.debug_mode:
    return

self.publish_log...

before the log calls would remove the need for going into all the debug methods when it is not necessary. This means the same statement can be removed from inside the log methods.

Comment on lines 258 to 259
# Check if waypoint is reached (0.5m threshold)
if distance < 0.5:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The treshold should be a parameter. Add it to the config file.

Comment on lines 262 to 264
# Reset filter state for next waypoint
initial_commands = np.array([0.0, 0.0, self.state.yaw])
self.guidance_calculator.reset_filter_state(initial_commands)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the initial state not just the same as the actual state?

@Hallfred Hallfred mentioned this pull request Jan 6, 2025
3 tasks
@AbubakarAliyuBadawi AbubakarAliyuBadawi force-pushed the 436-task-guidance-node-for-autopilot branch from 6a131bc to afb0a89 Compare January 7, 2025 15:00
@AbubakarAliyuBadawi AbubakarAliyuBadawi force-pushed the 436-task-guidance-node-for-autopilot branch from 845e362 to 62b1fd8 Compare January 7, 2025 15:50
@AbubakarAliyuBadawi AbubakarAliyuBadawi force-pushed the 436-task-guidance-node-for-autopilot branch from 7ef298f to e6e3e9f Compare January 8, 2025 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] Guidance node for autopilot
5 participants