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.

Sign up for GitHub

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

FTP request opcode #1357

Merged
merged 6 commits into from
Sep 10, 2014
+12 −85
Merged

FTP request opcode #1357

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
564c9b7
FTP: Add req_opcode field for return request opcode in response message.
vooon Sep 9, 2014
e7ae13a
FTP: Make responses start from opcode 128.
vooon Sep 9, 2014
0d2e250
FTP: Remove CRC32 from protocol.
vooon Sep 9, 2014
745707d
FTP: remove reserved uint32 from header.
vooon Sep 10, 2014
ed66097
FTP: Update unit test for new header size.
vooon Sep 10, 2014
0e3c606
FTP: remove padding zeroing.
vooon Sep 10, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 5 additions & 25 deletions src/modules/mavlink/mavlink_ftp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
/// @file mavlink_ftp.cpp
/// @author px4dev, Don Gagne <don@thegagnes.com>

#include <crc32.h>
#include <unistd.h>
#include <stdio.h>
#include <fcntl.h>
Expand Down Expand Up @@ -161,13 +160,6 @@ MavlinkFTP::_process_request(Request *req)
goto out;
}

// check request CRC to make sure this is one of ours
if (_payload_crc32(payload) != payload->crc32) {
errorCode = kErrCrc;
goto out;
warnx("ftp: bad crc");
}

#ifdef MAVLINK_FTP_DEBUG
printf("ftp: channel %u opc %u size %u offset %u\n", req->serverChannel, payload->opcode, payload->size, payload->offset);
#endif
Expand Down Expand Up @@ -224,12 +216,14 @@ MavlinkFTP::_process_request(Request *req)
out:
// handle success vs. error
if (errorCode == kErrNone) {
payload->req_opcode = payload->opcode;
payload->opcode = kRspAck;
#ifdef MAVLINK_FTP_DEBUG
warnx("FTP: ack\n");
#endif
} else {
warnx("FTP: nak %u", errorCode);
payload->req_opcode = payload->opcode;
payload->opcode = kRspNak;
payload->size = 1;
payload->data[0] = errorCode;
Expand All @@ -253,8 +247,9 @@ MavlinkFTP::_reply(Request *req)
PayloadHeader *payload = reinterpret_cast<PayloadHeader *>(&req->message.payload[0]);

payload->seqNumber = payload->seqNumber + 1;

payload->crc32 = _payload_crc32(payload);
payload->reserved[0] = 0;
payload->reserved[1] = 0;
payload->reserved[2] = 0;

mavlink_message_t msg;
msg.checksum = 0;
Expand Down Expand Up @@ -645,18 +640,3 @@ MavlinkFTP::_return_request(Request *req)
_unlock_request_queue();
}

/// @brief Returns the 32 bit CRC for the payload, crc32 and padding members are set to 0 for calculation.
uint32_t
MavlinkFTP::_payload_crc32(PayloadHeader *payload)
{
// We calculate CRC with crc and padding set to 0.
uint32_t saveCRC = payload->crc32;
payload->crc32 = 0;
payload->padding[0] = 0;
payload->padding[1] = 0;
payload->padding[2] = 0;
uint32_t retCRC = crc32((const uint8_t*)payload, payload->size + sizeof(PayloadHeader));
payload->crc32 = saveCRC;

return retCRC;
}
11 changes: 4 additions & 7 deletions src/modules/mavlink/mavlink_ftp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ class MavlinkFTP
uint8_t session; ///< Session id for read and write commands
uint8_t opcode; ///< Command opcode
uint8_t size; ///< Size of data
uint8_t padding[3]; ///< 32 bit aligment padding
uint32_t crc32; ///< CRC for entire Request structure, with crc32 and padding set to 0
uint8_t req_opcode; ///< Request opcode returned in kRspAck, kRspNak message
uint16_t reserved[3]; ///< reserved area
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the name from "padding" to "reserved" as well as change the comment? This makes it less clear as to why this is here. I can see "reserved" to keep people from touching this, but you should leave the comment about 32-bit alignment. Also the padding should now be 2 uint8_t's since you added one more byte to the structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the padding in the reserved as to leave room for crc32, for further development. So reserved is padding[2] + uint32_t.

Maybe better is to remove this space and leave padding only.

Copy link
Contributor

Choose a reason for hiding this comment

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

The header needs to be as small as possible in order to get as many bytes for data as we can. Downloading a file in as few packets as possible is critical. Given that I don't think reserving space for future changes is good in this case.

uint32_t offset; ///< Offsets for List and Read commands
uint8_t data[]; ///< command data, varies by Opcode
};
Expand All @@ -97,7 +97,7 @@ class MavlinkFTP
kCmdCreateDirectory, ///< Creates directory at <path>
kCmdRemoveDirectory, ///< Removes Directory at <path>, must be empty

kRspAck, ///< Ack response
kRspAck = 128, ///< Ack response
kRspNak ///< Nak response
};

Expand All @@ -111,8 +111,7 @@ class MavlinkFTP
kErrInvalidSession, ///< Session is not currently open
kErrNoSessionsAvailable, ///< All available Sessions in use
kErrEOF, ///< Offset past end of file for List and Read commands
kErrUnknownCommand, ///< Unknown command opcode
kErrCrc ///< CRC on Payload is incorrect
kErrUnknownCommand ///< Unknown command opcode
};

private:
Expand All @@ -134,8 +133,6 @@ class MavlinkFTP
void _lock_request_queue(void);
void _unlock_request_queue(void);

uint32_t _payload_crc32(PayloadHeader *hdr);

char *_data_as_cstring(PayloadHeader* payload);

static void _worker_trampoline(void *arg);
Expand Down