pw_transfer: Add handshake chunk types

This adds new chunk type values for the opening transfer handshake:
START_ACK and START_ACK_CONFIRMATION. Additionally, the redundant
TRANSFER_ prefix is removed from chunk types which previously had it.

Change-Id: I3da384b8d09cca99c05a6b6e916b9e55e6737e6b
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/96001
Commit-Queue: Alexei Frolov <[email protected]>
Reviewed-by: Wyatt Hepler <[email protected]>

pw_transfer/java/main/dev/pigweed/pw_transfer/Transfer.java

@@ -309,7 +309,7 @@ final synchronized void sendFinalChunk(Status status) {
 
     // Only call finish() if the sendChunk was successful. If it wasn't, the exception would have
     // already terminated the transfer.
-    if (sendChunk(newChunk(Chunk.Type.TRANSFER_COMPLETION).setStatus(status.code()))) {
+    if (sendChunk(newChunk(Chunk.Type.COMPLETION).setStatus(status.code()))) {
       cleanUp(status);
     }
   }

pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java

@@ -68,10 +68,7 @@ protected WriteTransfer(int id,
 
   @Override
   synchronized Chunk getInitialChunk() {
-    return newChunk(Chunk.Type.TRANSFER_START)
-        .setResourceId(getId())
-        .setRemainingBytes(data.length)
-        .build();
+    return newChunk(Chunk.Type.START).setResourceId(getId()).setRemainingBytes(data.length).build();
   }
 
   @Override
@@ -221,7 +218,7 @@ private static boolean isRetransmit(Chunk chunk) {
     // have a type field.
     return !chunk.hasType()
         || (chunk.getType().equals(Chunk.Type.PARAMETERS_RETRANSMIT)
-            || chunk.getType().equals(Chunk.Type.TRANSFER_START));
+            || chunk.getType().equals(Chunk.Type.START));
   }
 
   private static int getWindowEndOffset(Chunk chunk, int dataLength) {
@@ -234,8 +231,7 @@ private static int getWindowEndOffset(Chunk chunk, int dataLength) {
   }
 
   private Chunk buildDataChunk(ByteString chunkData) {
-    Chunk.Builder chunk =
-        newChunk(Chunk.Type.TRANSFER_DATA).setOffset(sentOffset).setData(chunkData);
+    Chunk.Builder chunk = newChunk(Chunk.Type.DATA).setOffset(sentOffset).setData(chunkData);
 
     // If this is the last data chunk, setRemainingBytes to 0.
     if (sentOffset + chunkData.size() == data.length) {

pw_transfer/py/pw_transfer/transfer.py

@@ -201,10 +201,9 @@ def _update_progress(self, bytes_sent: int, bytes_confirmed_received: int,
     def _send_error(self, error: Status) -> None:
         """Sends an error chunk to the server and finishes the transfer."""
         self._send_chunk(
-            transfer_pb2.Chunk(
-                transfer_id=self.id,
-                status=error.value,
-                type=transfer_pb2.Chunk.Type.TRANSFER_COMPLETION))
+            transfer_pb2.Chunk(transfer_id=self.id,
+                               status=error.value,
+                               type=transfer_pb2.Chunk.Type.COMPLETION))
         self.finish(error)
 
 
@@ -248,7 +247,7 @@ def _initial_chunk(self) -> transfer_pb2.Chunk:
         # server during an initial handshake.
         return transfer_pb2.Chunk(transfer_id=self.id,
                                   resource_id=self.id,
-                                  type=transfer_pb2.Chunk.Type.TRANSFER_START)
+                                  type=transfer_pb2.Chunk.Type.START)
 
     async def _handle_data_chunk(self, chunk: transfer_pb2.Chunk) -> None:
         """Processes an incoming chunk from the server.
@@ -298,9 +297,9 @@ def _handle_parameters_update(self, chunk: transfer_pb2.Chunk) -> bool:
 
         retransmit = True
         if chunk.HasField('type'):
-            retransmit = (
-                chunk.type == transfer_pb2.Chunk.Type.PARAMETERS_RETRANSMIT
-                or chunk.type == transfer_pb2.Chunk.Type.TRANSFER_START)
+            retransmit = (chunk.type
+                          == transfer_pb2.Chunk.Type.PARAMETERS_RETRANSMIT
+                          or chunk.type == transfer_pb2.Chunk.Type.START)
 
         if chunk.offset > len(self.data):
             # Bad offset; terminate the transfer.
@@ -355,7 +354,7 @@ def _next_chunk(self) -> transfer_pb2.Chunk:
         """Returns the next Chunk message to send in the data transfer."""
         chunk = transfer_pb2.Chunk(transfer_id=self.id,
                                    offset=self._offset,
-                                   type=transfer_pb2.Chunk.Type.TRANSFER_DATA)
+                                   type=transfer_pb2.Chunk.Type.DATA)
         max_bytes_in_chunk = min(self._max_chunk_size,
                                  self._window_end_offset - self._offset)
 
@@ -416,8 +415,7 @@ def data(self) -> bytes:
         return bytes(self._data)
 
     def _initial_chunk(self) -> transfer_pb2.Chunk:
-        return self._transfer_parameters(
-            transfer_pb2.Chunk.Type.TRANSFER_START)
+        return self._transfer_parameters(transfer_pb2.Chunk.Type.START)
 
     async def _handle_data_chunk(self, chunk: transfer_pb2.Chunk) -> None:
         """Processes an incoming chunk from the server.
@@ -446,7 +444,7 @@ async def _handle_data_chunk(self, chunk: transfer_pb2.Chunk) -> None:
                     transfer_pb2.Chunk(
                         transfer_id=self.id,
                         status=Status.OK.value,
-                        type=transfer_pb2.Chunk.Type.TRANSFER_COMPLETION))
+                        type=transfer_pb2.Chunk.Type.COMPLETION))
                 self.finish(Status.OK)
                 return
 

pw_transfer/py/tests/transfer_test.py

@@ -473,18 +473,18 @@ def test_write_transfer_timeout_after_initial_chunk(self) -> None:
         self.assertEqual(
             self._sent_chunks,
             [
-                transfer_pb2.Chunk(transfer_id=22,
-                                   resource_id=22,
-                                   type=transfer_pb2.Chunk.Type.TRANSFER_START
-                                   ),  # initial chunk
                 transfer_pb2.Chunk(
                     transfer_id=22,
                     resource_id=22,
-                    type=transfer_pb2.Chunk.Type.TRANSFER_START),  # retry 1
+                    type=transfer_pb2.Chunk.Type.START),  # initial chunk
+                transfer_pb2.Chunk(
+                    transfer_id=22,
+                    resource_id=22,
+                    type=transfer_pb2.Chunk.Type.START),  # retry 1
                 transfer_pb2.Chunk(
                     transfer_id=22,
                     resource_id=22,
-                    type=transfer_pb2.Chunk.Type.TRANSFER_START),  # retry 2
+                    type=transfer_pb2.Chunk.Type.START),  # retry 2
             ])
 
         exception = context.exception
@@ -506,23 +506,21 @@ def test_write_transfer_timeout_after_intermediate_chunk(self) -> None:
         with self.assertRaises(pw_transfer.Error) as context:
             manager.write(22, b'0123456789')
 
-        last_data_chunk = transfer_pb2.Chunk(
-            transfer_id=22,
-            data=b'56789',
-            offset=5,
-            remaining_bytes=0,
-            type=transfer_pb2.Chunk.Type.TRANSFER_DATA)
+        last_data_chunk = transfer_pb2.Chunk(transfer_id=22,
+                                             data=b'56789',
+                                             offset=5,
+                                             remaining_bytes=0,
+                                             type=transfer_pb2.Chunk.Type.DATA)
 
         self.assertEqual(
             self._sent_chunks,
             [
-                transfer_pb2.Chunk(
-                    transfer_id=22,
-                    resource_id=22,
-                    type=transfer_pb2.Chunk.Type.TRANSFER_START),
+                transfer_pb2.Chunk(transfer_id=22,
+                                   resource_id=22,
+                                   type=transfer_pb2.Chunk.Type.START),
                 transfer_pb2.Chunk(transfer_id=22,
                                    data=b'01234',
-                                   type=transfer_pb2.Chunk.Type.TRANSFER_DATA),
+                                   type=transfer_pb2.Chunk.Type.DATA),
                 last_data_chunk,  # last chunk
                 last_data_chunk,  # retry 1
                 last_data_chunk,  # retry 2

pw_transfer/transfer.proto

@@ -143,10 +143,10 @@ message Chunk {
 
   enum Type {
     // Chunk containing transfer data.
-    TRANSFER_DATA = 0;
+    DATA = 0;
 
     // First chunk of a transfer (only sent by the client).
-    TRANSFER_START = 1;
+    START = 1;
 
     // Transfer parameters indicating that the transmitter should retransmit
     // from the specified offset.
@@ -158,11 +158,21 @@ message Chunk {
     PARAMETERS_CONTINUE = 3;
 
     // Sender of the chunk is terminating the transfer.
-    TRANSFER_COMPLETION = 4;
+    COMPLETION = 4;
 
     // Acknowledge the completion of a transfer. Currently unused.
     // TODO(konkers): Implement this behavior.
-    TRANSFER_COMPLETION_ACK = 5;
+    COMPLETION_ACK = 5;
+
+    // Acknowledges a transfer start request, assigning a session ID for the
+    // transfer and optionally negotiating the protocol version. Sent from
+    // server to client.
+    START_ACK = 6;
+
+    // Confirmation of a START_ACK's assigned session ID and negotiated
+    // parameters, sent by the client to the server. Initiates the data transfer
+    // proper.
+    START_ACK_CONFIRMATION = 7;
   };
 
   // The type of this chunk. This field should only be processed when present.

pw_transfer/ts/transfer.ts

@@ -138,7 +138,7 @@ export abstract class Transfer {
     const chunk = new Chunk();
     chunk.setStatus(error);
     chunk.setTransferId(this.id);
-    chunk.setType(Chunk.Type.TRANSFER_COMPLETION);
+    chunk.setType(Chunk.Type.COMPLETION);
     this.sendChunk(chunk);
     this.finish(error);
   }
@@ -253,7 +253,7 @@ export class ReadTransfer extends Transfer {
   }
 
   protected get initialChunk(): Chunk {
-    return this.transferParameters(Chunk.Type.TRANSFER_START);
+    return this.transferParameters(Chunk.Type.START);
   }
 
   /** Builds an updated transfer parameters chunk to send the server. */
@@ -305,7 +305,7 @@ export class ReadTransfer extends Transfer {
         const endChunk = new Chunk();
         endChunk.setTransferId(this.id);
         endChunk.setStatus(Status.OK);
-        endChunk.setType(Chunk.Type.TRANSFER_COMPLETION);
+        endChunk.setType(Chunk.Type.COMPLETION);
         this.sendChunk(endChunk);
         this.finish(Status.OK);
         return;
@@ -407,7 +407,7 @@ export class WriteTransfer extends Transfer {
     const chunk = new Chunk();
     chunk.setTransferId(this.id);
     chunk.setResourceId(this.id);
-    chunk.setType(Chunk.Type.TRANSFER_START);
+    chunk.setType(Chunk.Type.START);
     return chunk;
   }
 
@@ -518,7 +518,7 @@ export class WriteTransfer extends Transfer {
     const chunk = new Chunk();
     chunk.setTransferId(this.id);
     chunk.setOffset(this.offset);
-    chunk.setType(Chunk.Type.TRANSFER_DATA);
+    chunk.setType(Chunk.Type.DATA);
 
     const maxBytesInChunk = Math.min(
       this.maxChunkSize,

pw_transfer/ts/transfer_test.ts

@@ -614,17 +614,17 @@ describe('Transfer client', () => {
         const expectedChunk1 = new Chunk();
         expectedChunk1.setTransferId(22);
         expectedChunk1.setResourceId(22);
-        expectedChunk1.setType(Chunk.Type.TRANSFER_START);
+        expectedChunk1.setType(Chunk.Type.START);
         const expectedChunk2 = new Chunk();
         expectedChunk2.setTransferId(22);
         expectedChunk2.setData(textEncoder.encode('01234'));
-        expectedChunk2.setType(Chunk.Type.TRANSFER_DATA);
+        expectedChunk2.setType(Chunk.Type.DATA);
         const lastChunk = new Chunk();
         lastChunk.setTransferId(22);
         lastChunk.setData(textEncoder.encode('56789'));
         lastChunk.setOffset(5);
         lastChunk.setRemainingBytes(0);
-        lastChunk.setType(Chunk.Type.TRANSFER_DATA);
+        lastChunk.setType(Chunk.Type.DATA);
 
         const expectedChunks = [
           expectedChunk1,