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

Read Mavlink from CSV file. Modules completed with tests #65

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

janez45
Copy link
Contributor

@janez45 janez45 commented Jul 10, 2024

No description provided.

@janez45
Copy link
Contributor Author

janez45 commented Jul 10, 2024

Completed my task with tests written. My main concern is if the pilots make a mistake with the params. Like if a value that should be zero is not set to zero (e.g. param2 for waypoint). I wonder if there should be more safeguarding in that sense?

@janez45
Copy link
Contributor Author

janez45 commented Jul 11, 2024

Another thing I'm concerned about is whether the code should crash if they forget a command parameter. Say, a 0 is missed. I don't think they'd want a drone to accidentally skip a command without warning. Gonna actually make that a crash

Copy link
Contributor

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

def generate_command_advanced(
frame: str,
command_type: str,
param1: float, # I'm anxious about this because of do_jump which requires ints
Copy link
Contributor

Choose a reason for hiding this comment

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

This function can dispatch to more specialized functions to construct the various commands.

param5: float,
param6: float,
param7: float,
) -> "tuple[bool,dronekit.Command]":
Copy link
Contributor

Choose a reason for hiding this comment

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

"tuple[bool, dronekit.Command | None]"

Comment on lines 41 to 53
frame: The command's frame of reference. Frame should be one of the following two values.
- global (usually used for landing or return to launch)
- global_relative_alt (all other commands, such as waypoint, spline waypoint, etc.)

command_type: The command type. Depending on this value, params 1 through 7 will take on different meanings. This should be one of the following enums:
- land:
- return_to_launch: Return to the home location where the vehicle was last armed.
- takeoff
- waypoint: Navigate to the specified waypoint
- waypoint_spline
- loiter_timed: Loiter at the specified location for an specified amount of time
- loiter_unlimited: Loiter at the specified location for an unlimited amount of time
- do_jump: Jump to the specified command in the mission list.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think linking the Confluence document is sufficient, no need to repeat the information from the dictionary above or the information in the document.

- A list of dronekit commands that represent the mission

"""
# Does the file path exist?
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove.

with open(mission_file_path, encoding="utf-8") as file:
for line in file:
# Skip header and empty lines
parts = line.split(",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Move below the conditional below.

return False, None

frame, command_type, param1, param2, param3, param4, param5, param6, param7 = parts
success, command = generate_command_advanced(
Copy link
Contributor

Choose a reason for hiding this comment

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

result, command =

float(param6),
float(param7),
)
# Was the command successfully generated?
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove.

Comment on lines 136 to 139
if len(mission) > 0:
return True, mission

return False, None
Copy link
Contributor

Choose a reason for hiding this comment

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

Switch:

if len(mission) == 0:
    return False, None

return True, mission

Copy link
Contributor

@AaronWang04 AaronWang04 left a comment

Choose a reason for hiding this comment

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

lgtm, nice job!

1: Parameter should be a float
2: Parameter should be an integer
"""
COMMAND_TO_PARAMETER_MATRIX = {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could also be ENUMs

@janez45 janez45 merged commit 2e68a4d into main Jul 18, 2024
1 check passed
@janez45 janez45 deleted the mavlink_from_csv branch July 18, 2024 03:44
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.

3 participants