-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial commit for statustext branch #2
base: main
Are you sure you want to change the base?
Conversation
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.
Reviewed
modules/recieve_statustxt.py
Outdated
@@ -0,0 +1,63 @@ | |||
import time |
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.
Please add a docstring at the top of this file briefly explaining what this code/module does
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.
Done :)
modules/recieve_statustxt.py
Outdated
import time | ||
from pymavlink import mavutil | ||
from modules.common.data_encoding.message_encoding_decoding import decode_bytes_to_position_global | ||
from modules.common.data_encoding.metadata_encoding_decoding import decode_metadata | ||
from modules.common.data_encoding.worker_enum import WorkerEnum | ||
from modules.common.kml.kml_conversion import positions_to_kml | ||
from pathlib import Path |
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.
Our import convention is to have Python imports first, then outside library imports, then our own module imports. There is a space between each section. We only import modules (files), so no importing classes or functions directly (ie no import WorkerEnum
, instead import worker_enum
). Finally, everything needs to be in alphabetical order, and two spaces after all of the imports
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.
Done :)
modules/recieve_statustxt.py
Outdated
CONNECTION_ADDRESS = "tcp:localhost:14550" | ||
DELAY = 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.
Our convention is to place 2 spaces after global constants. Also, for your script you would not need a DELAY
variable, since recv_match
is blocking
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.
Done :)
modules/recieve_statustxt.py
Outdated
f"Unsuccessful or Non-communications worker message (worker_id: {worker_id})" | ||
) | ||
# After collecting all positions, generate KML | ||
save_directory = Path("path/to/save/kml/files") |
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.
Make this a global constant
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.
Done :)
modules/recieve_statustxt.py
Outdated
) | ||
# After collecting all positions, generate KML | ||
save_directory = Path("path/to/save/kml/files") | ||
document_name_prefix = "drone_gps_data" |
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.
Save this in a global constant
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.
Done :)
modules/recieve_statustxt.py
Outdated
print("Failed to save KML file.") | ||
else: | ||
print(f"Unsuccessful or Non-communications worker message (worker_id: {worker_id})") | ||
time.sleep(DELAY) |
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.
No need for this
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.
Done :)
modules/recieve_statustxt.py
Outdated
vehicle.wait_heartbeat() | ||
print("connected") | ||
|
||
positions = [] |
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.
Better to declare this inside the infinite loop, so you don't have to do it again later below.
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.
Done :)
modules/recieve_statustxt.py
Outdated
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.
Can you put the entire thing in a main()
function, and then use a main guard (if __name__ == "__main__"
), and use argparse
to optionally take in save directory and document name prefix. Defaults are save directory = logs, document name prefix = IR hotspot locations
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.
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.
Reviewed
import argparse | ||
import time | ||
from pathlib import Path | ||
from pymavlink import mavutil |
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.
Please put 1 space above this line (this is a external library)
from modules.common.data_encoding import worker_enum | ||
from modules.common.kml.kml_conversion import positions_to_kml | ||
|
||
CONNECTION_ADDRESS = "tcp:localhost:14550" |
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.
Please put one more space above this line (2 spaces between imports and constants)
CONNECTION_ADDRESS = "tcp:localhost:14550" | ||
|
||
|
||
def main(save_directory, document_name_prefix): |
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.
Can you make main return a int, 0 for success and -1 (negative numbers) for errors and accordingly update the running part? (Just copy C++ standards)
from modules.common.data_encoding.message_encoding_decoding import decode_bytes_to_position_global | ||
from modules.common.data_encoding.metadata_encoding_decoding import decode_metadata | ||
from modules.common.data_encoding import worker_enum | ||
from modules.common.kml.kml_conversion import positions_to_kml |
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.
For these imports, can you import modules only, and not specific functions/classes? Just a convention thing. ie import message_encoding_decoding
, then use message_encoding_decoding.decode_bytes_to_position_global
.
if __name__ == "__main__": | ||
parser = argparse.ArgumentParser(description="Collect drone GPS positions and save as KML.") | ||
parser.add_argument( | ||
"--save-directory", type=str, default="logs", help="Directory to save KML files." |
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.
Preferably make the default value a constant
parser.add_argument( | ||
"--document-name-prefix", | ||
type=str, | ||
default="IR hotspot locations", |
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.
Preferably make the default value a constant
No description provided.