Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IOS/Network/KD: Implement GetSchedulerStat #11736

Merged
merged 1 commit into from May 1, 2023

Conversation

noahpistilli
Copy link
Contributor

Implements the GetSchedulerStat Ioctl which is required for proper rendering of errors in WiiConnect24 Channels.

Currently if an error arises, the only way to know what went wrong is to look at logs. Even then, it doesn't give an HTTP status code if the server errored.

Before
image

After
Screenshot 2023-04-06 at 10 16 02 PM

Source/Core/Core/IOS/Network/KD/NetKDRequest.h Outdated Show resolved Hide resolved
Source/Core/Common/HttpRequest.cpp Outdated Show resolved Hide resolved
Source/Core/Common/HttpRequest.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/KD/NetKDRequest.h Outdated Show resolved Hide resolved
Copy link
Member

@lioncash lioncash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two other minor things I accidentally missed in the previous review

Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp Outdated Show resolved Hide resolved
std::lock_guard lg(m_scheduler_buffer_lock);

m_scheduler_buffer[32 + m_error_count] = Common::swap32(new_code);
m_error_count++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never seems to be reset, so this might go out of bounds over time I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, best practice should be to check if it will go out of bounds. However, no official title will call KD Download enough, let alone resume after an error to cause a bounds error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at actual behaviour, it will reset the index back to 0 once there are 32 errors, but keep the error count.


std::lock_guard lg(m_scheduler_buffer_lock);

m_scheduler_buffer[32 + (m_error_count & 0x1f)] = Common::swap32(new_code);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
m_scheduler_buffer[32 + (m_error_count & 0x1f)] = Common::swap32(new_code);
m_scheduler_buffer[32 + (m_error_count % 32] = Common::swap32(new_code);

IMO that's a clearer way to express the intended behaviour ("reset the index back to 0 once there are 32 errors but keep the error count").

@@ -52,11 +51,24 @@ class NetKDRequestDevice : public Device
return std::nullopt;
}

enum class ErrorType
{
ACCOUNT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use SCREAMING_CASE for enum class values (because unlike C-style enumerators they do not leak into the surrounding scope, so there is less of a need for them to stand out)

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, sorry for the late approval

@leoetlino leoetlino merged commit 0eeeac2 into dolphin-emu:master May 1, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants