-
Notifications
You must be signed in to change notification settings - Fork 222
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
Bootcamp - Davis Liu #109
Bootcamp - Davis Liu #109
Conversation
if report.status == drone_status.DroneStatus.HALTED: # if command done | ||
if self.command_index < len(self.commands): # next command | ||
print(f"frame({self.counter}) cmd({self.command_index}) pos({report.position})") | ||
command = self.commands[self.command_index] | ||
self.command_index += 1 | ||
elif not self.has_sent_landing_command: # start landing | ||
command = commands.Command.create_land_command() | ||
self.has_sent_landing_command = True | ||
|
||
self.counter += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that we have a acceptance_radius attribute
due to drone position being analog, this is often required when checking if we have reached a location. it is important to double check after the drone has halted that the drone is indeed at the location specified in the command
@@ -69,6 +79,34 @@ def run( | |||
# ============ | |||
|
|||
# Do something based on the report and the state of this class... | |||
pos_x = report.position.location_x | |||
pos_y = report.position.location_y | |||
if report.status == drone_status.DroneStatus.HALTED: # if command done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply the double check with location here as well
if not result: | ||
return [], image_annotated # data is invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job returning the whole dataset as wrong, this is common practice done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job adding the check, just some nit picks on PR quality and code flow issues that can be improved!
def square(x: float) -> float: | ||
return x * x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary, if needed the math library has functions for power.
# self.has_land = False | ||
# self.counter = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pr should be cleaned up without commented out code
self.trg_x = 0 | ||
self.trg_y = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use variable names that are very clear in what it means, it helps reviewers and other members understand the code faster even if it makes the code longer
def square(x: float) -> float: | ||
return x * x | ||
|
||
def wrong_rel_pos(i: int) -> bool: # returns true if the drone halts at wrong position | ||
if i < 0 or i >= len(self.commands): | ||
return False | ||
cmd = self.commands[i] | ||
if cmd.get_command_type() == commands.Command.CommandType.SET_RELATIVE_DESTINATION: | ||
pos_x = report.position.location_x | ||
pos_y = report.position.location_y | ||
return square(pos_x - self.trg_x) + square(pos_y - self.trg_y) > square( | ||
self.acceptance_radius | ||
) | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good use of type hinting!
cmd_x, cmd_y = command.get_relative_destination() | ||
self.trg_x += cmd_x | ||
self.trg_y += cmd_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is redundant, you can just use the waypoint's location_x and location_y to check whether you are at the location you want since you know that is the exact location you are trying to reach
if wrong_rel_pos(self.command_index - 1): # repeat previous command | ||
command = commands.Command.create_set_relative_destination_command( | ||
self.trg_x - pos_x, self.trg_y - pos_y | ||
) | ||
elif self.command_index < len(self.commands): # next command | ||
print(f"cmd({self.command_index}) pos({report.position})") | ||
command = self.commands[self.command_index] | ||
if ( | ||
command.get_command_type() | ||
== commands.Command.CommandType.SET_RELATIVE_DESTINATION | ||
): # update target position | ||
cmd_x, cmd_y = command.get_relative_destination() | ||
self.trg_x += cmd_x | ||
self.trg_y += cmd_y | ||
self.command_index += 1 | ||
elif self.land_status == 0: # start landing | ||
command = commands.Command.create_land_command() | ||
self.land_status = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick but generally you want the "double check" if statements to be at the bottom, which makes more sense when reading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, remember that you won't have to cache waypoint location for our specific scenario because waypoint is constant and is passed in as an argument.
No description provided.