FTP request opcode #1357

vooon · 2014-09-09T13:47:05Z

This PR do:

Add req_opcode for returning request opcode in response messages.
Offsets response opcodes from request. This change allow us to add new opcodes without compatibility breaks.
Remove extra CRC32.

Extra crc not needed because mavlink already has crc16.

vooon · 2014-09-09T13:50:32Z

This PR implement my suggestions from #1345.

DonLakeFlyer · 2014-09-09T18:34:43Z

Don't understand why you need #1? Sequence numbers tell you if you are getting correct responses back and from what request.

On Sep 9, 2014, at 6:47 AM, "Vladimir Ermakov" notifications@github.com wrote:

This PR do:

Add req_opcode for returning request opcode in response messages.
Offsets response opcodes from request. This change allow us to add new opcodes without compatibility breaks.
Remove extra CRC32.
You can merge this Pull Request by running

git pull https://github.com/vooon/px4-firmware ftp_opcode
Or view, comment on, or merge it at:

#1357

Commit Summary

FTP: Add req_opcode field for return request opcode in response message.
FTP: Make responses start from opcode 128.
FTP: Remove CRC32 from protocol.
File Changes

M src/modules/mavlink/mavlink_ftp.cpp (30)
M src/modules/mavlink/mavlink_ftp.h (11)
Patch Links:

https://github.com/PX4/Firmware/pull/1357.patch
https://github.com/PX4/Firmware/pull/1357.diff
—
Reply to this email directly or view it on GitHub.

vooon · 2014-09-09T19:00:11Z

At least in open() i must know request opcode, now i guess it from size.

DonLakeFlyer · 2014-09-09T23:28:02Z

For req_opcode: You have to know what sort of a response you are waiting on. If a client sends an Open, you assume the Ack you are getting back is for an Open. The sequence numbers provide a decent level of validation that the Ack or Nak is associated with the correct command. You send an Open with sequence number "n" and get back "Ack/Nak" with sequence number "n+1". See the QGroundControl client code for example. I can see how adding req_opcode adds an additional level of validation, but other than that I don't see what it does. Are you trying to allow more than one command from the same client to be sent without waiting for a response to come back first? The protocol isn't set up to have a client send multiple commands without waiting for a response back. Even adding a req_opcode field won't allow for that any more reliably than just the current sequence number setup. At least not as far as I can see it. Maybe I need more explanation as to what you are trying to accomplish with this.

For crc32, and opcode 128: These are great changes. Thanks.

You need to update and pass the unit test as well though. See src/modules/mavlink/mavlink_tests. Build using "make px4fmu-v2_test". Flash with that build and run mavlink_tests from nsh.

DonLakeFlyer · 2014-09-09T23:35:51Z

src/modules/mavlink/mavlink_ftp.h

-		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


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.

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.

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.

vooon · 2014-09-10T06:39:05Z

I mean that open() handle both kCmdOpen and kCmdCreate. And potentially kCmd for open for write.
But i can save open type in client, just i think that more useful when protocol inform what they acked.

DonLakeFlyer · 2014-09-10T06:52:45Z

In order to write a client which handles all error cases, it will need to remember which command the ack is for. Otherwise an incorrectly written server implementation could return kCmdCreateFile in req_opcode in response to a kCmdOpenFile request. If your client just uses req_opcode on the Ack to determine what to do, strange things are going to happen in that case. I like the validation aspect of it, since it provides even better error checking on Acks. So I suggest leave it in. But I wouldn't depend on it to drive your client side state machine implementation.

_list_test failed.

vooon · 2014-09-10T08:25:55Z

@DonLakeFlyer unit-test failed on list operation, on response1, response2 works fine.

nsh> mavlink_tests
mavlink_tests: RUNNING TEST: _ack_test
mavlink_tests: TEST PASSED: _ack_test
mavlink_tests: RUNNING TEST: _bad_opcode_test
mavlink_tests: FTP: nak 7
mavlink_tests: TEST PASSED: _bad_opcode_test
mavlink_tests: RUNNING TEST: _bad_datasize_test
mavlink_tests: FTP: nak 3
mavlink_tests: TEST PASSED: _bad_datasize_test
mavlink_tests: RUNNING TEST: _list_test
mavlink_tests: FTP: can't open path '/bogus'
mavlink_tests: FTP: nak 2
mavlink_tests: Compare failed: Incorrect payload size - (reply->size:68) (expected_data_size:71) (/home/vovan/src/UAV/PX4/Firmware/src/modules)
mavlink_tests: TEST FAILED: _list_test
mavlink_tests: RUNNING TEST: _list_eof_test
mavlink_tests: FTP: nak 6
mavlink_tests: TEST PASSED: _list_eof_test
mavlink_tests: RUNNING TEST: _open_badfile_test
mavlink_tests: FTP: nak 2
mavlink_tests: TEST PASSED: _open_badfile_test
mavlink_tests: RUNNING TEST: _open_terminate_test
mavlink_tests: TEST PASSED: _open_terminate_test
mavlink_tests: RUNNING TEST: _terminate_badsession_test
mavlink_tests: FTP: nak 4
mavlink_tests: TEST PASSED: _terminate_badsession_test
mavlink_tests: RUNNING TEST: _read_test
mavlink_tests: TEST PASSED: _read_test
mavlink_tests: RUNNING TEST: _read_badsession_test
mavlink_tests: FTP: nak 4
mavlink_tests: TEST PASSED: _read_badsession_test
mavlink_tests: RUNNING TEST: _removedirectory_test
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: TEST PASSED: _removedirectory_test
mavlink_tests: RUNNING TEST: _createdirectory_test
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: TEST PASSED: _createdirectory_test
mavlink_tests: RUNNING TEST: _removefile_test
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: FTP: nak 2
mavlink_tests: TEST PASSED: _removefile_test
mavlink_tests: SOME TESTS FAILED
mavlink_tests:   Tests passed : 12
mavlink_tests:   Tests failed : 1
mavlink_tests:   Assertions : 282

vooon · 2014-09-10T08:43:40Z

Seems that this test not correct, list can return dirent in other order.

Dirty hack: inside if (test->success) {

            printf("RS: ");
            for (int i=0; i < reply->size; i++)
                printf("%c", (reply->data[i])? : '|');
            printf("\n");

Result:

RS: Ftest_240.data      240|Ftest_239.data      239|Dempty_dir|Ftest_238.data   238|

And i prefer to ignore "." and ".." dirs.

DonLakeFlyer · 2014-09-10T16:02:04Z

src/modules/mavlink/mavlink_ftp.cpp

-
-	payload->crc32 = _payload_crc32(payload);
+	payload->padding[0] = 0;
+	payload->padding[1] = 0;


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

DonLakeFlyer · 2014-09-10T16:05:34Z

Awesome. Thanks a ton for these changes both here and in QGroundControl. I saw the unit test failure you are seeing as well. I have a separate pull which fixes this already in the works. I'll also pull out "." and ".." returns. Other than removing the padding sets this is good to go. Once that is done I'll merge on both sides.

vooon · 2014-09-10T17:41:12Z

@DonLakeFlyer done.

FTP request opcode

vooon added 3 commits September 9, 2014 17:02

FTP: Add req_opcode field for return request opcode in response message.

564c9b7

FTP: Make responses start from opcode 128.

e7ae13a

FTP: Remove CRC32 from protocol.

0d2e250

Extra crc not needed because mavlink already has crc16.

vooon mentioned this pull request Sep 9, 2014

FTP request opcode mavlink/qgroundcontrol#865

Merged

DonLakeFlyer reviewed Sep 9, 2014
View reviewed changes

vooon added 2 commits September 10, 2014 11:22

FTP: remove reserved uint32 from header.

745707d

FTP: Update unit test for new header size.

ed66097

_list_test failed.

vooon mentioned this pull request Sep 10, 2014

FTP plugin mavlink/mavros#128

Closed

24 tasks

DonLakeFlyer reviewed Sep 10, 2014
View reviewed changes

FTP: remove padding zeroing.

0e3c606

DonLakeFlyer added a commit that referenced this pull request Sep 10, 2014

Merge pull request #1357 from vooon/ftp_opcode

e4f3fd8

FTP request opcode

DonLakeFlyer merged commit e4f3fd8 into PX4:master Sep 10, 2014

vooon deleted the ftp_opcode branch September 10, 2014 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTP request opcode #1357

FTP request opcode #1357

vooon commented Sep 9, 2014

vooon commented Sep 9, 2014

DonLakeFlyer commented Sep 9, 2014

vooon commented Sep 9, 2014

DonLakeFlyer commented Sep 9, 2014

DonLakeFlyer Sep 9, 2014

vooon Sep 10, 2014

DonLakeFlyer Sep 10, 2014

vooon commented Sep 10, 2014

DonLakeFlyer commented Sep 10, 2014

vooon commented Sep 10, 2014

vooon commented Sep 10, 2014

DonLakeFlyer Sep 10, 2014

DonLakeFlyer commented Sep 10, 2014

vooon commented Sep 10, 2014

FTP request opcode #1357

FTP request opcode #1357

Conversation

vooon commented Sep 9, 2014

vooon commented Sep 9, 2014

DonLakeFlyer commented Sep 9, 2014

vooon commented Sep 9, 2014

DonLakeFlyer commented Sep 9, 2014

DonLakeFlyer Sep 9, 2014

Choose a reason for hiding this comment

vooon Sep 10, 2014

Choose a reason for hiding this comment

DonLakeFlyer Sep 10, 2014

Choose a reason for hiding this comment

vooon commented Sep 10, 2014

DonLakeFlyer commented Sep 10, 2014

vooon commented Sep 10, 2014

vooon commented Sep 10, 2014

DonLakeFlyer Sep 10, 2014

Choose a reason for hiding this comment

DonLakeFlyer commented Sep 10, 2014

vooon commented Sep 10, 2014