Conversation
There was a problem hiding this comment.
We should aim to avoid duplicating the code for each camera - also, what is the purpose of nesting all of the functions into main()?
Some of the functions should really be outside this file - when looking over the main script any maintainer should be able to be able to take a quick glance at the main script and quickly determine the flow of the program. In the worst case and this ends up being the main script it should be much easier to modify than it is in its current state
There was a problem hiding this comment.
nested functions was because I was lazy lol, I think refactoring those out into standalone functions is a decent idea. I admit on second glance this is pretty sloppy at parts (I was really sleepy when writing the hud 😪) and a decent bit of this can be removed/cleaned up. I don't really think there's much to move to other files though, I want to keep the business logic in main
| ) | ||
|
|
||
| def get_face_info(self, target: Coordinate) -> dict | None: | ||
| """ |
There was a problem hiding this comment.
Just wondering, for MVP purposes should we include autonomous face determination at all? It should be fairly obvious to the pilot so they could manually input the detection face (NESW) at groundside (unless this is against the rules)
There was a problem hiding this comment.
It's easy enough to remove by commenting out the call in main, doesn't hurt to have and we an rethink this later
zjteachen
left a comment
zjteachen
left a comment
There was a problem hiding this comment.
Can test this this week. I think my main concern stems from that it's a lot of code (tech debt) for an MVP and in the worst case if this is what we put on the drone being slimmer would leave it more open to emergency modifications.
Wonder if common implements a lot of the modules/utilities used already (maybe they are copied from there, I wouldn't know)
README.md
Outdated
| - Set up and activate Python3.11 venv on drone and ground station | ||
| - Run `airside` on drone | ||
| - TODO: setup autostart on drone | ||
| - Run `groundside` on ground control station |
There was a problem hiding this comment.
Could be good to add instructions on the command to run the script.
|
|
||
| max_x, min_x = max(xs), min(xs) | ||
| max_y, min_y = max(ys), min(ys) | ||
| return min_x <= point.lat <= max_x and min_y <= point.lon <= max_y |
There was a problem hiding this comment.
nit: Directly comparing lat/lon assumes flat Earth; this could cause enough accuracy distortion to cause us to lose points. Might be worth looking into Haversine-based bound checking.
There was a problem hiding this comment.
Haversine seems to involve trig that, I assume, is equivalent since the distances we have are small with comparison to the earth's radius (so I think small angle approximation applies?). However, if you think that it is worth implementing, then I can definitely try my hand at it!
There was a problem hiding this comment.
We wouldn't have to implement there is a python library for it. Also yeah realistically error wouldn't be too too much but it's just extra noise we can get rid of fairly easily since library means no implementation burden.
There was a problem hiding this comment.
Oh I'll just use the library then, thank you!
| """Request position and RC channel data streams from drone.""" | ||
| try: | ||
| # Request position data at 1 Hz | ||
| self.mav.mav.request_data_stream_send( |
There was a problem hiding this comment.
I believe in newer versions of Ardupilot this is depracated, probably good to move to set_message_interval_send.
There was a problem hiding this comment.
What message id would this be streaming?
| ) | ||
|
|
||
| # Request RC channel data at 5 Hz | ||
| self.mav.mav.request_data_stream_send( |
| data = msg['data'] | ||
|
|
||
| if msg_type == 'building_corner': | ||
| building.corners.append(data) |
There was a problem hiding this comment.
If data is malformed or not a Coordinate, later calls to building.is_complete() or generate_target_description() will raise AttributeError or TypeError.
|
|
||
| frame_hsv = cv2.cvtColor(frame, cv2.COLOR_BGR2HSV) | ||
| for colour in [c.value for c in Colours]: | ||
| mask = cv2.inRange(frame_hsv, colour.lower_hsv, colour.upper_hsv) |
There was a problem hiding this comment.
i'm assuming the colour bounds you've put for each of them are very wide as we don't know what hues or saturations the targets will be, but as this is MVP it should do the job
There was a problem hiding this comment.
Just for the future devs, I'll at a TODO: in!
5 participants
No description provided.