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

Operator #9

Merged
merged 125 commits into from
Feb 1, 2021
Merged

Operator #9

merged 125 commits into from
Feb 1, 2021

Conversation

erwang01
Copy link
Contributor

@erwang01 erwang01 commented Oct 3, 2020

Layer 1 of the server.

Contains Drone Base Class as well.

@varunsaran varunsaran marked this pull request as ready for review October 6, 2020 02:59
@varunsaran varunsaran closed this Oct 6, 2020
@varunsaran varunsaran reopened this Oct 6, 2020
Copy link
Contributor

@shreyask23 shreyask23 left a comment

Choose a reason for hiding this comment

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

Reviewed mavros_drone.py. Looks good!

Copy link
Contributor

@DragonKyro DragonKyro left a comment

Choose a reason for hiding this comment

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

Reviewed roslibpy_test folder and simulate_drone_services folder

Copy link
Contributor

@KDharmarajanDev KDharmarajanDev left a comment

Choose a reason for hiding this comment

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

missionwaypointmsg.py, navsatfixex.py, waypoint.py can be deleted as they don't serve a purpose in the codebase. Other options that would serve similar functions are present inside the various drone implementations.

for navsatfix in waypoints:
converted_waypoint_objects.append(self.convert_navsatfix_mavroswaypoint(navsatfix))

converted_waypoint_objects = 2*[{'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.TAKEOFF.value, 'is_current': False, 'autocontinue': True, 'param1': 0, 'param2': 0, 'param3': 0, 'x_lat': self.location['latitude'], 'y_long': self.location['longitude'], 'z_alt': 10}] + converted_waypoint_objects
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
converted_waypoint_objects = 2*[{'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.TAKEOFF.value, 'is_current': False, 'autocontinue': True, 'param1': 0, 'param2': 0, 'param3': 0, 'x_lat': self.location['latitude'], 'y_long': self.location['longitude'], 'z_alt': 10}] + converted_waypoint_objects
# For MavRos, the waypoints need to include a "takeoff waypoint" that signals the drone to takeoff before flying
takeoff_cmd = {'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.TAKEOFF.value}
takeoff_cmd['is_current'] = False
takeoff_cmd['autocontinue'] = True
takeoff_cmd['param1'] = 0
takeoff_cmd['param2'] = 0
takeoff_cmd['param3'] = 0
takeoff_cmd['x_lat'] = self.location['latitude']
takeoff_cmd['y_long'] = self.location['longitude']
takeoff_cmd['z_alt'] = 10
converted_waypoint_objects = 2*[takeoff_cmd] + converted_waypoint_objects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KDharmarajanDev are these suggestions still valid?

Takes in a NavSatFix message and returns a mavros_msgs/Waypoint message.
This is in the form of a dictionary.
'''
waypoint = {'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.NAVIGATE_TO_WAYPOINT.value, 'is_current': False, 'autocontinue': True, 'param1': 0, 'param2': 0, 'param3': 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
waypoint = {'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.NAVIGATE_TO_WAYPOINT.value, 'is_current': False, 'autocontinue': True, 'param1': 0, 'param2': 0, 'param3': 0}
waypoint = {'frame': MavrosDrone.FRAME_REFERENCE.RELATIVE_ALT.value, 'command': MavrosDrone.MAV_CMD.NAVIGATE_TO_WAYPOINT.value}
waypoint['is_current'] = False
waypoint['autocontinue'] = True
waypoint['param1'] = 0
waypoint['param2'] = 0
waypoint['param3'] = 0

Copy link
Contributor

@akhilvemuri akhilvemuri left a comment

Choose a reason for hiding this comment

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

Finished review on djimatrice_drone.py

for i in range(len(self.waypoints)):
waypoint_msg = waypoints[i]
self.mission_msg_list.append(waypoint_msg)
self.upload_waypoint_task(waypoint_msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we supposed to upload_waypoint_task all at once or one by one. I vaguely recall it is all at once. @DragonKyro @KDharmarajanDev ?

waypoint_msg = waypoints[i]
self.mission_msg_list.append(waypoint_msg)
self.upload_waypoint_task(waypoint_msg)
return True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return type should be : dictionary {
success: boolean
message: descriptive string
}

per drone.py

return True
else:
print("Must be in ON_GROUND_STANDBY to upload mission")
return False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto dict return type

result = service.call(request)
print('Service response: {}'.format(result))
except:
result = {"success":False, "message":"Drone landing failed"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drone landing?

request = roslibpy.ServiceRequest({"speed": speed})

print('Calling mission_waypoint_setSpeed service...')
result = service.call(request)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is result from the dji service call also dictionary {
success: boolean
message: descriptive string
}


print('Calling fly_home service...')
#TODO parse service.call(request)
result = service.call(request)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto.

src/djimatrice_drone.py Outdated Show resolved Hide resolved
src/drone.py Show resolved Hide resolved
src/drone.py Outdated
from mavros_drone import MavrosDrone
drones = {
"DjiMatrice": DjiMatriceDrone,
"MavrosDrone": MavrosDrone
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"MavrosDrone": MavrosDrone
"Mavros": MavrosDrone

{"command": MavrosDrone.MAV_CMD.SET_SPEED.value,
"param1": 0, "param2": speed, "param3": -1, "param4": 0})
print('Calling mission_waypoint_setSpeed service...')
result = service.call(request)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the result of this service request have the form {success, message}?
Ditto at all the other services...

Copy link
Contributor

@leonardodtang leonardodtang left a comment

Choose a reason for hiding this comment

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

srv/ needs a little bit of attention on a couple of missing response id's by whoever wrote it, and drone.py just needs someone else to commit suggestions

src/drone.py Show resolved Hide resolved
src/mavros_drone.py Outdated Show resolved Hide resolved
src/mavros_drone.py Outdated Show resolved Hide resolved
src/mavros_drone.py Outdated Show resolved Hide resolved
src/mavros_drone.py Outdated Show resolved Hide resolved
src/djimatrice_drone.py Show resolved Hide resolved
src/drone.py Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
uint32 task

This comment was marked as resolved.

srv/control_drone.srv Show resolved Hide resolved
srv/upload_mission.srv Show resolved Hide resolved
Copy link
Contributor Author

@erwang01 erwang01 left a comment

Choose a reason for hiding this comment

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

Generally LGTM. More bug fixes / concrete implementation can be done later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants