From 62a2611925b3ffa8e3e36201a7ed46fd9ac6d843 Mon Sep 17 00:00:00 2001 From: booto Date: Thu, 11 Jul 2019 01:22:22 -0400 Subject: [PATCH] SI: Clarify length fields for manual commands Fix up the calculation of the length fields and check that the returned response is the expected length. This touches many files because it converts a parameter name from the SI_Device interface from 'length' to 'request_length'. Prior, this field seemed to be used as request length sometimes, as response length sometimes, and usually just totally ignored. --- Source/Core/Core/HW/SI/SI.cpp | 56 +++++++++++++------ Source/Core/Core/HW/SI/SI_Device.cpp | 7 ++- Source/Core/Core/HW/SI/SI_Device.h | 2 +- Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp | 13 ++--- Source/Core/Core/HW/SI/SI_DeviceDanceMat.h | 2 +- Source/Core/Core/HW/SI/SI_DeviceGBA.cpp | 2 +- Source/Core/Core/HW/SI/SI_DeviceGBA.h | 2 +- Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp | 4 +- Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h | 2 +- .../Core/Core/HW/SI/SI_DeviceGCController.cpp | 20 +++---- .../Core/Core/HW/SI/SI_DeviceGCController.h | 2 +- .../Core/HW/SI/SI_DeviceGCSteeringWheel.cpp | 13 ++--- .../Core/HW/SI/SI_DeviceGCSteeringWheel.h | 2 +- Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp | 14 ++--- Source/Core/Core/HW/SI/SI_DeviceKeyboard.h | 2 +- Source/Core/Core/HW/SI/SI_DeviceNull.cpp | 4 +- Source/Core/Core/HW/SI/SI_DeviceNull.h | 2 +- 17 files changed, 82 insertions(+), 67 deletions(-) diff --git a/Source/Core/Core/HW/SI/SI.cpp b/Source/Core/Core/HW/SI/SI.cpp index 3438a4c950..b82af48ce0 100644 --- a/Source/Core/Core/HW/SI/SI.cpp +++ b/Source/Core/Core/HW/SI/SI.cpp @@ -8,7 +8,9 @@ #include #include #include +#include #include +#include #include "Common/ChunkFile.h" #include "Common/CommonTypes.h" @@ -280,31 +282,51 @@ static void GenerateSIInterrupt(SIInterruptType type) UpdateInterrupts(); } +constexpr u32 SI_XFER_LENGTH_MASK = 0x7f; + +// Translate [0,1,2,...,126,127] to [128,1,2,...,126,127] +constexpr u32 ConvertSILengthField(u32 field) +{ + return ((field - 1) & SI_XFER_LENGTH_MASK) + 1; +} + static void RunSIBuffer(u64 user_data, s64 cycles_late) { if (s_com_csr.TSTART) { - // Math in_length - int in_length = s_com_csr.INLNGTH; - if (in_length == 0) - in_length = 128; - else - in_length++; - - // Math out_length - int out_length = s_com_csr.OUTLNGTH; - if (out_length == 0) - out_length = 128; - else - out_length++; + u32 request_length = ConvertSILengthField(s_com_csr.OUTLNGTH); + u32 expected_response_length = ConvertSILengthField(s_com_csr.INLNGTH); + std::vector request_copy(s_si_buffer.data(), s_si_buffer.data() + request_length); std::unique_ptr& device = s_channel[s_com_csr.CHANNEL].device; - int numOutput = device->RunBuffer(s_si_buffer.data(), in_length); + u32 actual_response_length = device->RunBuffer(s_si_buffer.data(), request_length); - DEBUG_LOG(SERIALINTERFACE, "RunSIBuffer chan: %d inLen: %i outLen: %i processed: %i", - s_com_csr.CHANNEL, in_length, out_length, numOutput); + DEBUG_LOG(SERIALINTERFACE, + "RunSIBuffer chan: %d request_length: %u expected_response_length: %u " + "actual_response_length: %u", + s_com_csr.CHANNEL, request_length, expected_response_length, actual_response_length); + if (expected_response_length != actual_response_length) + { + std::stringstream ss; + for (u8 b : request_copy) + { + ss << std::hex << std::setw(2) << std::setfill('0') << (int)b << ' '; + } + ERROR_LOG( + SERIALINTERFACE, + "RunSIBuffer: expected_response_length(%u) != actual_response_length(%u): request: %s", + expected_response_length, actual_response_length, ss.str().c_str()); + } - if (numOutput != 0) + // TODO: + // 1) Wait a reasonable amount of time for the result to be available: + // request is N bytes, ends with a stop bit + // response in M bytes, ends with a stop bit + // processing controller-side takes K us (investigate?) + // each bit takes 4us ([3us low/1us high] for a 0, [1us low/3us high] for a 1) + // time until response is available is at least: K + ((N*8 + 1) + (M*8 + 1)) * 4 us + // 2) Investigate the timeout period for NOREP0 + if (actual_response_length != 0) { s_com_csr.TSTART = 0; GenerateSIInterrupt(INT_TCINT); diff --git a/Source/Core/Core/HW/SI/SI_Device.cpp b/Source/Core/Core/HW/SI/SI_Device.cpp index bd63f0ad76..0d5eb89289 100644 --- a/Source/Core/Core/HW/SI/SI_Device.cpp +++ b/Source/Core/Core/HW/SI/SI_Device.cpp @@ -37,15 +37,16 @@ SIDevices ISIDevice::GetDeviceType() const return m_device_type; } -int ISIDevice::RunBuffer(u8* buffer, int length) +int ISIDevice::RunBuffer(u8* buffer, int request_length) { #ifdef _DEBUG - DEBUG_LOG(SERIALINTERFACE, "Send Data Device(%i) - Length(%i) ", m_device_number, length); + DEBUG_LOG(SERIALINTERFACE, "Send Data Device(%i) - Length(%i) ", m_device_number, + request_length); std::string temp; int num = 0; - while (num < length) + while (num < request_length) { temp += StringFromFormat("0x%02x ", buffer[num]); num++; diff --git a/Source/Core/Core/HW/SI/SI_Device.h b/Source/Core/Core/HW/SI/SI_Device.h index eaf4f0486b..c666f98805 100644 --- a/Source/Core/Core/HW/SI/SI_Device.h +++ b/Source/Core/Core/HW/SI/SI_Device.h @@ -73,7 +73,7 @@ public: SIDevices GetDeviceType() const; // Run the SI Buffer - virtual int RunBuffer(u8* buffer, int length); + virtual int RunBuffer(u8* buffer, int request_length); virtual int TransferInterval(); // Return true on new data diff --git a/Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp b/Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp index 2c76200559..08686a140e 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp @@ -17,24 +17,19 @@ CSIDevice_DanceMat::CSIDevice_DanceMat(SIDevices device, int device_number) { } -int CSIDevice_DanceMat::RunBuffer(u8* buffer, int length) +int CSIDevice_DanceMat::RunBuffer(u8* buffer, int request_length) { // Read the command EBufferCommands command = static_cast(buffer[0]); - if (command == CMD_RESET) { - ISIDevice::RunBuffer(buffer, length); + ISIDevice::RunBuffer(buffer, request_length); u32 id = Common::swap32(SI_DANCEMAT); std::memcpy(buffer, &id, sizeof(id)); + return sizeof(id); } - else - { - return CSIDevice_GCController::RunBuffer(buffer, length); - } - - return length; + return CSIDevice_GCController::RunBuffer(buffer, request_length); } u32 CSIDevice_DanceMat::MapPadStatus(const GCPadStatus& pad_status) diff --git a/Source/Core/Core/HW/SI/SI_DeviceDanceMat.h b/Source/Core/Core/HW/SI/SI_DeviceDanceMat.h index 5e6fdda9b8..17f35cd59b 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceDanceMat.h +++ b/Source/Core/Core/HW/SI/SI_DeviceDanceMat.h @@ -16,7 +16,7 @@ class CSIDevice_DanceMat : public CSIDevice_GCController public: CSIDevice_DanceMat(SIDevices device, int device_number); - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; u32 MapPadStatus(const GCPadStatus& pad_status) override; bool GetData(u32& hi, u32& low) override; }; diff --git a/Source/Core/Core/HW/SI/SI_DeviceGBA.cpp b/Source/Core/Core/HW/SI/SI_DeviceGBA.cpp index 8d28ed322f..a776dad587 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGBA.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceGBA.cpp @@ -290,7 +290,7 @@ CSIDevice_GBA::CSIDevice_GBA(SIDevices device, int device_number) : ISIDevice(de { } -int CSIDevice_GBA::RunBuffer(u8* buffer, int length) +int CSIDevice_GBA::RunBuffer(u8* buffer, int request_length) { switch (m_next_action) { diff --git a/Source/Core/Core/HW/SI/SI_DeviceGBA.h b/Source/Core/Core/HW/SI/SI_DeviceGBA.h index 6e1467b88d..c08ab954e0 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGBA.h +++ b/Source/Core/Core/HW/SI/SI_DeviceGBA.h @@ -45,7 +45,7 @@ class CSIDevice_GBA : public ISIDevice public: CSIDevice_GBA(SIDevices device, int device_number); - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; int TransferInterval() override; bool GetData(u32& hi, u32& low) override; void SendCommand(u32 command, u8 poll) override; diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp b/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp index 3f3d70202c..80d4f22ca7 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp @@ -49,7 +49,7 @@ GCPadStatus CSIDevice_GCAdapter::GetPadStatus() return pad_status; } -int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int length) +int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int request_length) { if (!Core::WantsDeterminism()) { @@ -66,7 +66,7 @@ int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int length) return 4; } } - return CSIDevice_GCController::RunBuffer(buffer, length); + return CSIDevice_GCController::RunBuffer(buffer, request_length); } bool CSIDevice_GCAdapter::GetData(u32& hi, u32& low) diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h b/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h index aa34ff5d84..25fc3a775a 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h +++ b/Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h @@ -16,7 +16,7 @@ public: CSIDevice_GCAdapter(SIDevices device, int device_number); GCPadStatus GetPadStatus() override; - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; bool GetData(u32& hi, u32& low) override; diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCController.cpp b/Source/Core/Core/HW/SI/SI_DeviceGCController.cpp index 11494cae46..024b3d6047 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCController.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceGCController.cpp @@ -36,17 +36,17 @@ CSIDevice_GCController::CSIDevice_GCController(SIDevices device, int device_numb m_origin.substick_y = GCPadStatus::C_STICK_CENTER_Y; } -int CSIDevice_GCController::RunBuffer(u8* buffer, int length) +int CSIDevice_GCController::RunBuffer(u8* buffer, int request_length) { // For debug logging only - ISIDevice::RunBuffer(buffer, length); + ISIDevice::RunBuffer(buffer, request_length); GCPadStatus pad_status = GetPadStatus(); if (!pad_status.isConnected) { u32 reply = Common::swap32(SI_ERROR_NO_RESPONSE); std::memcpy(buffer, &reply, sizeof(reply)); - return 4; + return sizeof(reply); } // Read the command @@ -60,21 +60,21 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length) { u32 id = Common::swap32(SI_GC_CONTROLLER); std::memcpy(buffer, &id, sizeof(id)); - break; + return sizeof(id); } case CMD_DIRECT: { - INFO_LOG(SERIALINTERFACE, "PAD - Direct (Length: %d)", length); + INFO_LOG(SERIALINTERFACE, "PAD - Direct (Request length: %d)", request_length); u32 high, low; GetData(high, low); - for (int i = 0; i < (length - 1) / 2; i++) + for (int i = 0; i < 4; i++) { buffer[i + 0] = (high >> (24 - (i * 8))) & 0xff; buffer[i + 4] = (low >> (24 - (i * 8))) & 0xff; } + return sizeof(high) + sizeof(low); } - break; case CMD_ORIGIN: { @@ -85,8 +85,8 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length) { buffer[i] = *calibration++; } + return sizeof(SOrigin); } - break; // Recalibrate (FiRES: i am not 100 percent sure about this) case CMD_RECALIBRATE: @@ -98,8 +98,8 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length) { buffer[i] = *calibration++; } + return sizeof(SOrigin); } - break; // DEFAULT default: @@ -110,7 +110,7 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length) break; } - return length; + return 0; } void CSIDevice_GCController::HandleMoviePadStatus(GCPadStatus* pad_status) diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCController.h b/Source/Core/Core/HW/SI/SI_DeviceGCController.h index 631ca25064..9c09c50f14 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCController.h +++ b/Source/Core/Core/HW/SI/SI_DeviceGCController.h @@ -84,7 +84,7 @@ public: CSIDevice_GCController(SIDevices device, int device_number); // Run the SI Buffer - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; // Return true on new data bool GetData(u32& hi, u32& low) override; diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp b/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp index 3fe0a3230f..e3563b73aa 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp @@ -20,10 +20,10 @@ CSIDevice_GCSteeringWheel::CSIDevice_GCSteeringWheel(SIDevices device, int devic { } -int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int length) +int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int request_length) { // For debug logging only - ISIDevice::RunBuffer(buffer, length); + ISIDevice::RunBuffer(buffer, request_length); // Read the command EBufferCommands command = static_cast(buffer[0]); @@ -36,14 +36,11 @@ int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int length) { u32 id = Common::swap32(SI_GC_STEERING); std::memcpy(buffer, &id, sizeof(id)); - break; + return sizeof(id); + } } - default: - return CSIDevice_GCController::RunBuffer(buffer, length); - } - - return length; + return CSIDevice_GCController::RunBuffer(buffer, request_length); } bool CSIDevice_GCSteeringWheel::GetData(u32& hi, u32& low) diff --git a/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.h b/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.h index c9afee2773..05bc6bea16 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.h +++ b/Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.h @@ -13,7 +13,7 @@ class CSIDevice_GCSteeringWheel : public CSIDevice_GCController public: CSIDevice_GCSteeringWheel(SIDevices device, int device_number); - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; bool GetData(u32& hi, u32& low) override; void SendCommand(u32 command, u8 poll) override; diff --git a/Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp b/Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp index 6e3cc00ce6..d32fac1455 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp @@ -21,10 +21,10 @@ CSIDevice_Keyboard::CSIDevice_Keyboard(SIDevices device, int device_number) { } -int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length) +int CSIDevice_Keyboard::RunBuffer(u8* buffer, int request_length) { // For debug logging only - ISIDevice::RunBuffer(buffer, length); + ISIDevice::RunBuffer(buffer, request_length); // Read the command EBufferCommands command = static_cast(buffer[0]); @@ -37,21 +37,21 @@ int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length) { u32 id = Common::swap32(SI_GC_KEYBOARD); std::memcpy(buffer, &id, sizeof(id)); - break; + return sizeof(id); } case CMD_DIRECT: { - INFO_LOG(SERIALINTERFACE, "Keyboard - Direct (Length: %d)", length); + INFO_LOG(SERIALINTERFACE, "Keyboard - Direct (Request Length: %d)", request_length); u32 high, low; GetData(high, low); - for (int i = 0; i < (length - 1) / 2; i++) + for (int i = 0; i < 4; i++) { buffer[i + 0] = (high >> (24 - (i * 8))) & 0xff; buffer[i + 4] = (low >> (24 - (i * 8))) & 0xff; } + return sizeof(high) + sizeof(low); } - break; default: { @@ -60,7 +60,7 @@ int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length) break; } - return length; + return 0; } KeyboardStatus CSIDevice_Keyboard::GetKeyboardStatus() const diff --git a/Source/Core/Core/HW/SI/SI_DeviceKeyboard.h b/Source/Core/Core/HW/SI/SI_DeviceKeyboard.h index d703f24fa9..d9bb029d99 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceKeyboard.h +++ b/Source/Core/Core/HW/SI/SI_DeviceKeyboard.h @@ -18,7 +18,7 @@ public: CSIDevice_Keyboard(SIDevices device, int device_number); // Run the SI Buffer - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; // Return true on new data bool GetData(u32& hi, u32& low) override; diff --git a/Source/Core/Core/HW/SI/SI_DeviceNull.cpp b/Source/Core/Core/HW/SI/SI_DeviceNull.cpp index ee4a39c253..f4ed00e7ae 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceNull.cpp +++ b/Source/Core/Core/HW/SI/SI_DeviceNull.cpp @@ -14,11 +14,11 @@ CSIDevice_Null::CSIDevice_Null(SIDevices device, int device_number) { } -int CSIDevice_Null::RunBuffer(u8* buffer, int length) +int CSIDevice_Null::RunBuffer(u8* buffer, int request_length) { u32 reply = Common::swap32(SI_ERROR_NO_RESPONSE); std::memcpy(buffer, &reply, sizeof(reply)); - return 4; + return sizeof(reply); } bool CSIDevice_Null::GetData(u32& hi, u32& low) diff --git a/Source/Core/Core/HW/SI/SI_DeviceNull.h b/Source/Core/Core/HW/SI/SI_DeviceNull.h index 3b5e20ab65..88fd16021c 100644 --- a/Source/Core/Core/HW/SI/SI_DeviceNull.h +++ b/Source/Core/Core/HW/SI/SI_DeviceNull.h @@ -15,7 +15,7 @@ class CSIDevice_Null final : public ISIDevice public: CSIDevice_Null(SIDevices device, int device_number); - int RunBuffer(u8* buffer, int length) override; + int RunBuffer(u8* buffer, int request_length) override; bool GetData(u32& hi, u32& low) override; void SendCommand(u32 command, u8 poll) override; };