Made SBC protocol more resilient

Fixed some bugs affecting SPI error recovery for SBC mode

3 files changed, 88 insertions, 42 deletions

diff --git a/src/SBC/DataTransfer.cpp b/src/SBC/DataTransfer.cpp
index ddf488a5..f6fc1779 100644
--- a/src/SBC/DataTransfer.cpp
+++ b/src/SBC/DataTransfer.cpp
@@ -721,25 +721,44 @@ void DataTransfer::ExchangeData() noexcept
 	setup_spi(rxBuffer, txBuffer, bytesToExchange);
 }
 
-void DataTransfer::ResetTransfer(bool ownRequest) noexcept
+void DataTransfer::RestartTransfer(bool ownRequest) noexcept
 {
 	if (reprap.Debug(moduleSbcInterface))
 	{
 		debugPrintf(ownRequest ? "Resetting transfer\n" : "Resetting transfer due to Sbc request\n");
 	}
 
+	failedTransfers++;
 	if (ownRequest)
 	{
-		// Invalidate the data to send
-		txResponse = TransferResponse::BadResponse;
-		Cache::FlushBeforeDMASend(&txResponse, sizeof(txResponse));
-		state = InternalTransferState::Resetting;
-		setup_spi(&rxResponse, &txResponse, sizeof(uint32_t));
+		if (rxHeader.dataLength > 0 || txPointer > 0)
+		{
+			// Transfer bad data response and restart the transfer
+			txResponse = TransferResponse::BadResponse;
+			Cache::FlushBeforeDMASend(&txResponse, sizeof(txResponse));
+			state = InternalTransferState::Resetting;
+			setup_spi(&rxResponse, &txResponse, sizeof(uint32_t));
+		}
+		else
+		{
+			// No data was transferred so we are still in sync. Continue with the next transfer
+			lastTransferNumber = rxHeader.sequenceNumber - 1;
+			ExchangeHeader();
+		}
 	}
 	else
 	{
-		// Sbc wants to reset the state
-		ExchangeHeader();
+		if (state != InternalTransferState::ExchangingHeader)
+		{
+			// SBC wants to restart the transfer
+			ExchangeHeader();
+		}
+		else
+		{
+			// Last data response exchange failed, try to perform it again
+			ExchangeResponse(TransferResponse::Success);
+			state = InternalTransferState::ExchangingDataResponseRetry;
+		}
 	}
 }
 
@@ -780,13 +799,17 @@ TransferState DataTransfer::DoTransfer() noexcept
 		{
 		case InternalTransferState::ExchangingHeader:
 		{
-			Cache::InvalidateAfterDMAReceive(&rxHeader, sizeof(rxHeader));
 			// (1) Exchanged transfer headers
+			Cache::InvalidateAfterDMAReceive(&rxHeader, sizeof(rxHeader));
 			const uint32_t headerResponse = *reinterpret_cast<const uint32_t*>(&rxHeader);
 			if (headerResponse == TransferResponse::BadResponse)
 			{
-				// Sbc wants to restart the transfer
-				ResetTransfer(false);
+				// SBC received a bad response code. We must have been happy if we got here, else RRF would have complained
+				if (reprap.Debug(moduleSbcInterface))
+				{
+					debugPrintf("Retrying data response exchange\n");
+				}
+				RestartTransfer(false);
 				break;
 			}
 
@@ -840,11 +863,6 @@ TransferState DataTransfer::DoTransfer() noexcept
 					return IsConnectionReset() ? TransferState::connectionReset : TransferState::finished;
 				}
 			}
-			else if (rxResponse == TransferResponse::BadResponse)
-			{
-				// Sbc wants to restart the transfer
-				ResetTransfer(false);
-			}
 			else if (rxResponse == TransferResponse::BadHeaderChecksum || txResponse == TransferResponse::BadHeaderChecksum)
 			{
 				// Failed to exchange header, restart the full transfer
@@ -853,23 +871,18 @@ TransferState DataTransfer::DoTransfer() noexcept
 			}
 			else
 			{
-				// Received invalid response code
-				ResetTransfer(true);
+				// Restart the full transfer
+				RestartTransfer(rxResponse != TransferResponse::BadResponse);
 			}
 			break;
 
 		case InternalTransferState::ExchangingData:
 		{
-			Cache::InvalidateAfterDMAReceive(rxBuffer, rxHeader.dataLength);
 			// (3) Exchanged data
+			Cache::InvalidateAfterDMAReceive(rxBuffer, rxHeader.dataLength);
 			if (*reinterpret_cast<uint32_t*>(rxBuffer) == TransferResponse::BadResponse)
 			{
-				if (reprap.Debug(moduleSbcInterface))
-				{
-					debugPrintf("Resetting state due to Sbc request\n");
-				}
-				failedTransfers++;
-				ExchangeHeader();
+				RestartTransfer(false);
 				break;
 			}
 
@@ -889,7 +902,7 @@ TransferState DataTransfer::DoTransfer() noexcept
 		}
 
 		case InternalTransferState::ExchangingDataResponse:
-			// (4) Exchanged response to data transfer
+			// (4a) Exchanged response to data transfer
 			Cache::InvalidateAfterDMAReceive(&rxResponse, sizeof(rxResponse));
 			if (rxResponse == TransferResponse::Success && txResponse == TransferResponse::Success)
 			{
@@ -900,29 +913,54 @@ TransferState DataTransfer::DoTransfer() noexcept
 				return IsConnectionReset() ? TransferState::connectionReset : TransferState::finished;
 			}
 
-			if (rxResponse == TransferResponse::BadResponse)
-			{
-				// Sbc wants to restart the transfer
-				ResetTransfer(false);
-			}
-			else if (rxResponse == TransferResponse::BadDataChecksum || txResponse == TransferResponse::BadDataChecksum)
+			if (rxResponse == TransferResponse::BadDataChecksum || txResponse == TransferResponse::BadDataChecksum)
 			{
 				// Resend the data if a checksum error occurred
 				checksumErrors++;
 				ExchangeData();
 			}
+			else if (rxResponse == TransferResponse::BadResponse)
+			{
+				// Restart the full transfer
+				RestartTransfer(false);
+			}
 			else
 			{
-				// Received invalid response, reset the SPI transfer
-				ResetTransfer(true);
+				// Restart the data response transfer
+				RestartTransfer(true);
+				state = InternalTransferState::ResettingDataResponse;
+			}
+			break;
+
+		case InternalTransferState::ExchangingDataResponseRetry:
+			// (4b) Exchanged response to data transfer when new transfer is being started (fallback on bad response)
+			Cache::InvalidateAfterDMAReceive(&rxResponse, sizeof(rxResponse));
+			if (rxResponse == TransferResponse::Success && txResponse == TransferResponse::Success)
+			{
+				// Retry succeeded
+				ExchangeHeader();
+			}
+			else
+			{
+				// Retry failed, reset the connection
+				if (reprap.Debug(moduleSbcInterface))
+				{
+					debugPrintf("Data response retry failed (sent %08" PRIx32 ", got %08" PRIx32 ")\n", txResponse, rxResponse);
+				}
+				return TransferState::connectionReset;
 			}
 			break;
 
 		case InternalTransferState::Resetting:
-			// Transmitted bad response, attempt to start a new transfer
+			// Transmitted bad response, attempt to restart the connection
 			ExchangeHeader();
 			break;
 
+		case InternalTransferState::ResettingDataResponse:
+			// Transmitted bad response after data response exchange, attempt to restart the data response exchange
+			ExchangeResponse(TransferResponse::Success);
+			break;
+
 		default:
 			// Should never get here. If we do, this probably means that StartNextTransfer has not been called
 			ExchangeHeader();
@@ -952,7 +990,7 @@ void DataTransfer::StartNextTransfer() noexcept
 	txHeader.crcData = CalcCRC32(txBuffer, txPointer);
 	txHeader.crcHeader = CalcCRC32(reinterpret_cast<const char *>(&txHeader), sizeof(TransferHeader) - sizeof(uint32_t));
 
-	// Begin SPI transfe
+	// Begin SPI transfer
 	ExchangeHeader();
 }
 
diff --git a/src/SBC/DataTransfer.h b/src/SBC/DataTransfer.h
index bdc87bc9..3520ed41 100644
--- a/src/SBC/DataTransfer.h
+++ b/src/SBC/DataTransfer.h
@@ -97,8 +97,10 @@ private:
 		ExchangingHeaderResponse,
 		ExchangingData,
 		ExchangingDataResponse,
+		ExchangingDataResponseRetry,
 		ProcessingData,
-		Resetting
+		Resetting,
+		ResettingDataResponse
 	} state;
 
 	// Transfer properties
@@ -137,7 +139,7 @@ private:
 	void ExchangeHeader() noexcept;
 	void ExchangeResponse(uint32_t response) noexcept;
 	void ExchangeData() noexcept;
-	void ResetTransfer(bool ownRequest) noexcept;
+	void RestartTransfer(bool ownRequest) noexcept;
 	uint32_t CalcCRC32(const char *buffer, size_t length) const noexcept;
 
 	template<typename T> const T *ReadDataHeader() noexcept;
@@ -155,8 +157,7 @@ private:
 
 inline bool DataTransfer::IsConnectionReset() const noexcept
 {
-	uint16_t nextTransferNumber = lastTransferNumber + 1;
-	return lastTransferNumber != 0 && (nextTransferNumber != rxHeader.sequenceNumber);
+	return lastTransferNumber != 0 && (lastTransferNumber + 1 != rxHeader.sequenceNumber);
 }
 
 inline size_t DataTransfer::PacketsToRead() const noexcept
diff --git a/src/SBC/SbcInterface.cpp b/src/SBC/SbcInterface.cpp
index bf6696de..7c039e95 100644
--- a/src/SBC/SbcInterface.cpp
+++ b/src/SBC/SbcInterface.cpp
@@ -154,8 +154,12 @@ void SbcInterface::Spin() noexcept
 				{
 					numSbcTimeouts++;
 				}
+				reprap.GetPlatform().MessageF(NetworkInfoMessage, "Lost connection to SBC due to %s timeout\n", hadSbcTimeout ? "remote" : "local");
+			}
+			else
+			{
+				reprap.GetPlatform().Message(NetworkInfoMessage, "Lost connection to SBC due to connection reset\n");
 			}
-			reprap.GetPlatform().MessageF(NetworkInfoMessage, "Lost connection to SBC due to %s timeout\n", hadSbcTimeout ? "remote" : "local");
 
 			// Invalidate local resources
 			InvalidateResources();
@@ -176,7 +180,10 @@ void SbcInterface::Spin() noexcept
 			}
 
 			// Handle exchanged data and kick off the next transfer
-			ExchangeData();
+			if (!hadReset)
+			{
+				ExchangeData();
+			}
 			transfer.StartNextTransfer();
 		}
 		else if (hadTimeout || hadReset)