Merge pull request #14119 from jordan-woyak/si-status-response-length

HW/SI: Fix CMD_STATUS response lengths.

Source/Core/Core/HW/SI/SI.cpp

@@ -3,13 +3,15 @@
 
 #include "Core/HW/SI/SI.h"
 
-#include <algorithm>
 #include <array>
-#include <atomic>
 #include <cstring>
-#include <iomanip>
 #include <memory>
-#include <sstream>
+
+#if defined(_DEBUG)
+#include <vector>
+
+#include "Common/StringUtil.h"
+#endif
 
 #include "Common/BitField.h"
 #include "Common/ChunkFile.h"
@@ -147,9 +149,12 @@ void SerialInterfaceManager::RunSIBuffer(u64 user_data, s64 cycles_late)
   {
     const s32 request_length = ConvertSILengthField(m_com_csr.OUTLNGTH);
     const s32 expected_response_length = ConvertSILengthField(m_com_csr.INLNGTH);
-    const std::vector<u8> request_copy(m_si_buffer.data(), m_si_buffer.data() + request_length);
 
-    const std::unique_ptr<ISIDevice>& device = m_channel[m_com_csr.CHANNEL].device;
+#if defined(_DEBUG)
+    const std::vector<u8> request_copy(m_si_buffer.data(), m_si_buffer.data() + request_length);
+#endif
+
+    auto* const device = m_channel[m_com_csr.CHANNEL].device.get();
     const s32 actual_response_length = device->RunBuffer(m_si_buffer.data(), request_length);
 
     DEBUG_LOG_FMT(SERIALINTERFACE,
@@ -159,15 +164,16 @@ void SerialInterfaceManager::RunSIBuffer(u64 user_data, s64 cycles_late)
                   actual_response_length);
     if (actual_response_length > 0 && expected_response_length != actual_response_length)
     {
-      std::ostringstream ss;
+#if defined(_DEBUG)
-      for (const u8 b : request_copy)
+      WARN_LOG_FMT(
-      {
-        ss << std::hex << std::setw(2) << std::setfill('0') << (int)b << ' ';
-      }
-      DEBUG_LOG_FMT(
           SERIALINTERFACE,
           "RunSIBuffer: expected_response_length({}) != actual_response_length({}): request: {}",
-          expected_response_length, actual_response_length, ss.str());
+          expected_response_length, actual_response_length, Common::BytesToHexString(request_copy));
+#else
+      WARN_LOG_FMT(SERIALINTERFACE,
+                   "RunSIBuffer: expected_response_length({}) != actual_response_length({})",
+                   expected_response_length, actual_response_length);
+#endif
     }
 
     // TODO:

Source/Core/Core/HW/SI/SI_Device.cpp

@@ -11,6 +11,7 @@
 #include "Common/CommonTypes.h"
 #include "Common/Logging/Log.h"
 #include "Common/MsgHandler.h"
+#include "Common/Swap.h"
 #include "Core/HW/SI/SI_DeviceDanceMat.h"
 #include "Core/HW/SI/SI_DeviceGBA.h"
 #ifdef HAS_LIBMGBA
@@ -87,6 +88,15 @@ void ISIDevice::OnEvent(u64 userdata, s64 cycles_late)
 {
 }
 
+int ISIDevice::CreateStatusResponse(u32 si_device_id, u8* buffer)
+{
+  constexpr int RESPONSE_LENGTH = 3;
+
+  Common::BigEndianValue<u32> id(si_device_id);
+  std::memcpy(buffer, &id, RESPONSE_LENGTH);
+  return RESPONSE_LENGTH;
+}
+
 int SIDevice_GetGBATransferTime(const SystemTimers::SystemTimersManager& timers,
                                 EBufferCommands cmd)
 {

Source/Core/Core/HW/SI/SI_Device.h

@@ -123,7 +123,12 @@ public:
   SIDevices GetDeviceType() const;
 
   // Run the SI Buffer
+  // Return value:
+  //  positive: The response length.
+  //  0: Response not ready, we will try again `TransferInterval()` cycles later.
+  //  -1: No response.
   virtual int RunBuffer(u8* buffer, int request_length);
+
   virtual int TransferInterval();
 
   virtual DataResponse GetData(u32& hi, u32& low) = 0;
@@ -138,6 +143,9 @@ public:
   virtual void OnEvent(u64 userdata, s64 cycles_late);
 
 protected:
+  // Only the three high bytes of `si_device_id` are used.
+  static int CreateStatusResponse(u32 si_device_id, u8* buffer);
+
   Core::System& m_system;
 
   int m_device_number;

Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp

@@ -3,10 +3,7 @@
 
 #include "Core/HW/SI/SI_DeviceDanceMat.h"
 
-#include <cstring>
-
 #include "Common/CommonTypes.h"
-#include "Common/Swap.h"
 #include "InputCommon/GCPadStatus.h"
 
 namespace SerialInterface
@@ -22,11 +19,10 @@ int CSIDevice_DanceMat::RunBuffer(u8* buffer, int request_length)
   const auto command = static_cast<EBufferCommands>(buffer[0]);
   if (command == EBufferCommands::CMD_STATUS)
   {
+    // Only used for logging.
     ISIDevice::RunBuffer(buffer, request_length);
 
-    const u32 id = Common::swap32(SI_DANCEMAT);
+    return CreateStatusResponse(SI_DANCEMAT, buffer);
-    std::memcpy(buffer, &id, sizeof(id));
-    return sizeof(id);
   }
   return CSIDevice_GCController::RunBuffer(buffer, request_length);
 }

Source/Core/Core/HW/SI/SI_DeviceGBAEmu.cpp

@@ -10,7 +10,6 @@
 #include "Common/ChunkFile.h"
 #include "Common/CommonTypes.h"
 #include "Common/Logging/Log.h"
-#include "Common/Swap.h"
 #include "Core/Core.h"
 #include "Core/CoreTiming.h"
 #include "Core/HW/GBACore.h"

Source/Core/Core/HW/SI/SI_DeviceGCController.cpp

@@ -57,9 +57,7 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int request_length)
   case EBufferCommands::CMD_STATUS:
   case EBufferCommands::CMD_RESET:
   {
-    const u32 id = Common::swap32(SI_GC_CONTROLLER);
+    return CreateStatusResponse(SI_GC_CONTROLLER, buffer);
-    std::memcpy(buffer, &id, sizeof(id));
-    return sizeof(id);
   }
 
   case EBufferCommands::CMD_DIRECT:

Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp

@@ -34,9 +34,7 @@ int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int request_length)
   case EBufferCommands::CMD_STATUS:
   case EBufferCommands::CMD_RESET:
   {
-    const u32 id = Common::swap32(SI_GC_STEERING);
+    return CreateStatusResponse(SI_GC_STEERING, buffer);
-    std::memcpy(buffer, &id, sizeof(id));
-    return sizeof(id);
   }
   default:
     return CSIDevice_GCController::RunBuffer(buffer, request_length);

Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp

@@ -9,7 +9,6 @@
 #include "Common/ChunkFile.h"
 #include "Common/CommonTypes.h"
 #include "Common/Logging/Log.h"
-#include "Common/Swap.h"
 #include "Core/HW/GCKeyboard.h"
 #include "InputCommon/KeyboardStatus.h"
 
@@ -35,9 +34,7 @@ int CSIDevice_Keyboard::RunBuffer(u8* buffer, int request_length)
   case EBufferCommands::CMD_STATUS:
   case EBufferCommands::CMD_RESET:
   {
-    const u32 id = Common::swap32(SI_GC_KEYBOARD);
+    return CreateStatusResponse(SI_GC_KEYBOARD, buffer);
-    std::memcpy(buffer, &id, sizeof(id));
-    return sizeof(id);
   }
 
   case EBufferCommands::CMD_DIRECT_KB: