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/KD: Implement Download Scheduler #12165

Merged

Conversation

noahpistilli
Copy link
Member

This PR implements the Downloader portion of the scheduler. This allows for WC24 files to be downloaded while emulation is running. This is mostly useful for channels that post announcements to the Wii Message Board, or ones that update their banners.

Please be aware that the former's functionality is currently not implemented as KDDownload does not support saving to the message board. Support for that is coming in a later PR.

The easiest way to go about testing this is to grab a NAND dump that has WC24 channels already installed. A mail task will dispatched first to determine time in minutes that a download task should be dispatched. If KDCheckMail cannot connect to the mail server or it cannot find the HTTP header X-Wii-Download-Span, it will set the download span to 2 and 1 minutes respectively.

Alternatively if you don't have WC24 channels installed but you have a NAND that isn't the default one, you can run system update to grab the Forecast and News Channels.

@noahpistilli noahpistilli force-pushed the scheduler-downloader branch 2 times, most recently from f36254f to 41f4879 Compare September 6, 2023 10:31
Comment on lines 164 to 177
u16 NWC24Dl::GetRetryTime(u16 entry_index) const
{
return Common::swap16(m_data.entries[entry_index].retry_frequency) * SECONDS_IN_MINUTE;
}

u16 NWC24Dl::GetDownloadMargin(u16 entry_index) const
{
return Common::swap16(m_data.entries[entry_index].dl_margin) * SECONDS_IN_MINUTE;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These feel like prone to overflow with the 60 multiplier, maybe return them as u32?

u32 NWC24Dl::GetNextDownloadTime(u16 record_index) const
{
// Timestamps are stored as a UNIX timestamp but in minutes. We want seconds.
return Common::swap32(m_data.records[record_index].next_dl_timestamp) * SECONDS_IN_MINUTE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, might be better to return an u64 here.

// We first need current UTC.
auto time_device =
std::static_pointer_cast<NetKDTimeDevice>(GetIOS()->GetDeviceByName("/dev/net/kd/time"));
const u32 current_utc = time_device->GetAdjustedUTC();
Copy link
Contributor

Choose a reason for hiding this comment

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

This implicitly truncates from u64 to u32. I'm guessing a real IOS does do this check in 32 bits but it might make sense to do this in 64 bits in Dolphin?

There's a couple more places where this could apply too, since you're passing around seconds but store to the file in minutes, maybe just do all the time calculations in 64 bits.

Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

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

Some small remarks.

There are many swap calls so feel free to use the Common::BigEndianValue helper in Common/Swap.h if needed (this can be done in another PR).

Source/Core/Core/IOS/Network/KD/NWC24DL.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/KD/NWC24DL.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.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp Outdated Show resolved Hide resolved
@noahpistilli
Copy link
Member Author

I will probably defer using Common::BigEndianValue to another PR like you said.

@JMC47
Copy link
Contributor

JMC47 commented Sep 10, 2023

@AdmiralCurtiss is this good to go now?

Comment on lines +181 to +188
if (subtask_id)
{
m_data.entries[record_index].subtask_timestamps[*subtask_id] =
Common::swap32(static_cast<u32>(value / SECONDS_PER_MINUTE));
}

m_data.records[record_index].next_dl_timestamp =
Common::swap32(static_cast<u32>(value / SECONDS_PER_MINUTE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, the fact that next_dl_timestamp is set even if subtask_id exists is intentional, yeah?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, yes.

@AdmiralCurtiss
Copy link
Contributor

Yeah, completely untested by me but looks reasonable. I have one question (see comment above) but if that is indeed intentional then feel free to merge if you tested it.

@AdmiralCurtiss AdmiralCurtiss merged commit 308a52a into dolphin-emu:master Sep 12, 2023
11 checks passed
@JosJuice
Copy link
Member

JosJuice commented Sep 16, 2023

This change seems to be triggering stack corruption: https://bugs.dolphin-emu.org/issues/13353

EDIT: PR #12181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants