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

groundside script to open and read data from files in pixhawk file system #69

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonz9
Copy link

@jonz9 jonz9 commented Dec 1, 2024

No description provided.

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

ftp_payload[6] = 0 # burst_complete
ftp_payload[7] = 0 # padding
ftp_payload[8:12] = struct.pack("<I", offset)
ftp_payload[12 : 12 + len(payload)] = payload
Copy link
Member

Choose a reason for hiding this comment

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

Please check to make sure this doesn't overflow

Comment on lines +37 to +42
opcode (int): FTP command opcode.
req_opcode (int): Requested opcode.
session (int): Session ID.
offset (int): Offset in the file.
size (int): Size of the payload.
payload (bytes): Payload data.
Copy link
Member

Choose a reason for hiding this comment

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

Add comment giving the range (and/or size) of all of these numbers

Copy link
Member

Choose a reason for hiding this comment

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

Can you move this file to modules/mavlink folder. Also rename the file to ftp_example.py. We probably won't be using this as the communication method for comp, but it may be useful for logging in the future.

send_ftp_command(
vehicle,
seq_num=SEQ_NUM,
opcode=4,
Copy link
Member

Choose a reason for hiding this comment

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

Make an enum for opcode so that people don't have to constantly look up the FTP docs

# open file for reading session
send_ftp_command(
vehicle,
seq_num=SEQ_NUM,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a constant, it should increment by 1 (mod 65535+1) every time you send a message (ie if you receive a message that has seq_num=123, you should send the next message with seq_num=124).

size=len(FILE_PATH),
payload=FILE_PATH,
)
SEQ_NUM += 1
Copy link
Member

Choose a reason for hiding this comment

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

You increment this from the value you received in the response FTP message. Also, capital variables are meant to be constants and shouldn't change. You can rename this to lower case, and write out the full name.

Comment on lines +99 to +105
# retrieve session id, file size, and sequence number from ACK response
SESSION_ID = response_payload[2]
DATA_OFFSET = struct.unpack("<I", response_payload[8:12])[0]
FILE_SIZE = struct.unpack("<I", response_payload[12 : 12 + response_payload[4]])[0]
SEQ_NUM = struct.unpack("<H", response_payload[0:2])[0]
CHUNK_SIZE = 239 # Max data size per chunk
FILE_DATA = b""
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this, make a function that returns a "FTP_Response" object with these as fields. You should also do sequence_num += 1 mod 2^16, so people don't have to remember to do that

if response_payload[3] != 128: # Check for error - NAK Response
print("ERROR RECEIVED")
print("ERROR CODE: ", response_payload[12])
sys.exit()
Copy link
Member

Choose a reason for hiding this comment

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

This is ok for now, but in practice we never just quit the program, that may cause drone to fall out of the sky.

response_payload = bytes(response.payload)
if response_payload[3] != 128: # Check for error - NAK Response
print("ERROR RECEIVED")
print("ERROR CODE: ", response_payload[12])
Copy link
Member

Choose a reason for hiding this comment

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

This thing could be 2 bytes, you should check the size byte. Also, make an enum for error codes so that you don't print a number but the actual error message.

FILE_DATA += chunk_data
DATA_OFFSET += len(chunk_data)

print(chunk_data.decode("utf-8", errors="ignore"), end="")
Copy link
Member

Choose a reason for hiding this comment

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

Rather than printing things one chunk at a time, you should print the entire file at once at the very end. Python print statements add newline characters, which will make your file look like it has random newlines everywhere. Also, if the bytes broke in a bad spot (ie if there was an integer in the file which is 4 bytes but you only received 1, you're going to print garbage)

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.

2 participants