Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

FTP request opcode #1357

Merged
merged 6 commits into from
Sep 10, 2014
Merged

FTP request opcode #1357

Show file tree
Hide file tree
Changes from 5 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
Binary file renamed ...nit_test_data/mavlink_tests/test_234.data → ...nit_test_data/mavlink_tests/test_238.data
View file
Open in desktop
Binary file not shown.
Binary file renamed ...nit_test_data/mavlink_tests/test_235.data → ...nit_test_data/mavlink_tests/test_239.data
View file
Open in desktop
Binary file not shown.
Binary file renamed ...nit_test_data/mavlink_tests/test_236.data → ...nit_test_data/mavlink_tests/test_240.data
View file
Open in desktop
Binary file not shown.
29 changes: 4 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,8 @@ MavlinkFTP::_reply(Request *req)
PayloadHeader *payload = reinterpret_cast<PayloadHeader *>(&req->message.payload[0]);

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

payload->crc32 = _payload_crc32(payload);
payload->padding[0] = 0;
payload->padding[1] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment on not needing to do this from QGC side of things.


mavlink_message_t msg;
msg.checksum = 0;
Expand Down Expand Up @@ -645,18 +639,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
uint8_t padding[2]; ///< 32 bit aligment padding
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
57 changes: 6 additions & 51 deletions src/modules/mavlink/mavlink_tests/mavlink_ftp_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@

/// @brief Test case file name for Read command. File are generated using mavlink_ftp_test_data.py
const MavlinkFtpTest::ReadTestCase MavlinkFtpTest::_rgReadTestCases[] = {
{ "/etc/unit_test_data/mavlink_tests/test_234.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) - 1}, // Read takes less than single packet
{ "/etc/unit_test_data/mavlink_tests/test_235.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) }, // Read completely fills single packet
{ "/etc/unit_test_data/mavlink_tests/test_236.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) + 1 }, // Read take two packets
{ "/etc/unit_test_data/mavlink_tests/test_238.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) - 1}, // Read takes less than single packet
{ "/etc/unit_test_data/mavlink_tests/test_239.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) }, // Read completely fills single packet
{ "/etc/unit_test_data/mavlink_tests/test_240.data", MAVLINK_MSG_FILE_TRANSFER_PROTOCOL_FIELD_PAYLOAD_LEN - sizeof(MavlinkFTP::PayloadHeader) + 1 }, // Read take two packets
};

const char MavlinkFtpTest::_unittest_microsd_dir[] = "/fs/microsd/ftp_unit_test_dir";
Expand Down Expand Up @@ -105,33 +105,6 @@ bool MavlinkFtpTest::_ack_test(void)
return true;
}

/// @brief Tests for correct response to a message sent with an invalid CRC.
bool MavlinkFtpTest::_bad_crc_test(void)
{
mavlink_message_t msg;
MavlinkFTP::PayloadHeader payload;
mavlink_file_transfer_protocol_t ftp_msg;
MavlinkFTP::PayloadHeader *reply;

payload.opcode = MavlinkFTP::kCmdNone;

_setup_ftp_msg(&payload, 0, nullptr, &msg);

((MavlinkFTP::PayloadHeader*)((mavlink_file_transfer_protocol_t*)msg.payload64)->payload)->crc32++;

_ftp_server->handle_message(nullptr /* mavlink */, &msg);

if (!_decode_message(&_reply_msg, &ftp_msg, &reply)) {
return false;
}

ut_compare("Didn't get Nak back", reply->opcode, MavlinkFTP::kRspNak);
ut_compare("Incorrect payload size", reply->size, 1);
ut_compare("Incorrect error code", reply->data[0], MavlinkFTP::kErrCrc);

return true;
}

/// @brief Tests for correct response to an invalid opcpde.
bool MavlinkFtpTest::_bad_opcode_test(void)
{
Expand Down Expand Up @@ -191,7 +164,7 @@ bool MavlinkFtpTest::_list_test(void)
mavlink_file_transfer_protocol_t ftp_msg;
MavlinkFTP::PayloadHeader *reply;

char response1[] = "D.|Dempty_dir|Ftest_234.data\t234|Ftest_235.data\t235|Ftest_236.data\t236";
char response1[] = "D.|Dempty_dir|Ftest_238.data\t238|Ftest_239.data\t239|Ftest_240.data\t240";
char response2[] = "Ddev|Detc|Dfs|Dobj";

struct _testCase {
Expand Down Expand Up @@ -690,9 +663,6 @@ bool MavlinkFtpTest::_decode_message(const mavlink_message_t *msg, ///< Mavlin

*payload = reinterpret_cast<MavlinkFTP::PayloadHeader *>(ftp_msg->payload);

// Make sure we have a good CRC
ut_compare("Incoming CRC mismatch", (*payload)->crc32, _payload_crc32((*payload)));

// Make sure we have a good sequence number
ut_compare("Sequence number mismatch", (*payload)->seqNumber, _lastOutgoingSeqNumber + 1);

Expand All @@ -702,21 +672,6 @@ bool MavlinkFtpTest::_decode_message(const mavlink_message_t *msg, ///< Mavlin
return true;
}

/// @brief Returns the 32 bit CRC for the payload, crc32 and padding are set to 0 for calculation.
uint32_t MavlinkFtpTest::_payload_crc32(struct MavlinkFTP::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(MavlinkFTP::PayloadHeader));
payload->crc32 = saveCRC;

return retCRC;
}

/// @brief Initializes an FTP message into a mavlink message
void MavlinkFtpTest::_setup_ftp_msg(MavlinkFTP::PayloadHeader *payload_header, ///< FTP payload header
uint8_t size, ///< size in bytes of data
Expand All @@ -734,7 +689,8 @@ void MavlinkFtpTest::_setup_ftp_msg(MavlinkFTP::PayloadHeader *payload_header, /
memcpy(payload->data, data, size);
}

payload->crc32 = _payload_crc32(payload);
payload->padding[0] = 0;
payload->padding[1] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need


msg->checksum = 0;
mavlink_msg_file_transfer_protocol_pack(clientSystemId, // Sender system id
Expand Down Expand Up @@ -771,7 +727,6 @@ void MavlinkFtpTest::_cleanup_microsd(void)
void MavlinkFtpTest::runTests(void)
{
ut_run_test(_ack_test);
ut_run_test(_bad_crc_test);
ut_run_test(_bad_opcode_test);
ut_run_test(_bad_datasize_test);
ut_run_test(_list_test);
Expand Down
2 changes: 0 additions & 2 deletions src/modules/mavlink/mavlink_tests/mavlink_ftp_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class MavlinkFtpTest : public UnitTest

private:
bool _ack_test(void);
bool _bad_crc_test(void);
bool _bad_opcode_test(void);
bool _bad_datasize_test(void);
bool _list_test(void);
Expand All @@ -81,7 +80,6 @@ class MavlinkFtpTest : public UnitTest
bool _removefile_test(void);

void _receive_message(const mavlink_message_t *msg);
uint32_t _payload_crc32(struct MavlinkFTP::PayloadHeader *payload);
void _setup_ftp_msg(MavlinkFTP::PayloadHeader *payload_header, uint8_t size, const uint8_t *data, mavlink_message_t *msg);
bool _decode_message(const mavlink_message_t *msg, mavlink_file_transfer_protocol_t *ftp_msg, MavlinkFTP::PayloadHeader **payload);
bool _send_receive_msg(MavlinkFTP::PayloadHeader *payload_header,
Expand Down