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

Services/APT: Implemented the Store/LoadSysMenuArg functions. #5196

Merged
merged 1 commit into from Apr 18, 2020

Conversation

Subv
Copy link
Member

@Subv Subv commented Apr 12, 2020

They are called by the Home Menu during initialization.

These functions will not see much use until we actually implement application jumping and system rebooting. For now we just need them to prevent some unmapped reads in the Home Menu due to the static buffers not being properly set up.


This change is Reviewable

@Subv Subv marked this pull request as draft Apr 12, 2020
@Subv Subv marked this pull request as ready for review Apr 12, 2020
@ghost
Copy link

ghost commented Apr 12, 2020

Home Menu (USA), region in settings: USA
image


// This service function does not clear the buffer.

std::vector<u8> buffer;
Copy link

@ghost ghost Apr 12, 2020

Choose a reason for hiding this comment

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

Suggested change
std::vector<u8> buffer;
std::vector<u8> buffer(size);

@ghost
Copy link

ghost commented Apr 12, 2020

sys_menu_arg_buffer is empty

@Subv
Copy link
Member Author

Subv commented Apr 12, 2020

sys_menu_arg_buffer is empty

Yes, it's supposed to work in a similar way to the screen capture info buffer, an application first has to call StoreSysMenuArg to fill it

@ghost
Copy link

ghost commented Apr 12, 2020

size: 24
buffer: empty
sys_menu_arg_buffer: empty

@Subv Subv marked this pull request as draft Apr 12, 2020
@@ -650,6 +650,34 @@ void Module::APTInterface::CloseLibraryApplet(Kernel::HLERequestContext& ctx) {
rb.Push(apt->applet_manager->CloseLibraryApplet(std::move(object), std::move(buffer)));
}

void Module::APTInterface::LoadSysMenuArg(Kernel::HLERequestContext& ctx) {
IPC::RequestParser rp(ctx, 0x36, 1, 0); // 0x00360040
u32 size = std::min(rp.Pop<u32>(), 0x40u);
Copy link
Contributor

@B3n30 B3n30 Apr 12, 2020

Choose a reason for hiding this comment

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

magic number

void Module::APTInterface::StoreSysMenuArg(Kernel::HLERequestContext& ctx) {
IPC::RequestParser rp(ctx, 0x37, 1, 2); // 0x00370042
u32 size = rp.Pop<u32>();
ASSERT(size == 0x40);
Copy link
Contributor

@B3n30 B3n30 Apr 12, 2020

Choose a reason for hiding this comment

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

magic number

Copy link
Contributor

@B3n30 B3n30 Apr 12, 2020

Choose a reason for hiding this comment

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

Does apt really crash here? or does it return some error?

@Subv Subv marked this pull request as ready for review Apr 12, 2020
@ghost
Copy link

ghost commented Apr 12, 2020

now, the emulator freezes and memory usage increases

@Subv Subv force-pushed the apt_sys_menu_arg branch 2 times, most recently from fbca822 to afb1119 Compare Apr 12, 2020
@Subv
Copy link
Member Author

Subv commented Apr 12, 2020

Thanks for all the feedback, @vvanelslande, it seems i've made a ton of mistakes on this PR. I've pushed a new version which hopefully fixes it, could you test again please?

@ghost
Copy link

ghost commented Apr 12, 2020

it's fixed

@@ -42,6 +43,8 @@ struct CaptureBufferInfo {
};
static_assert(sizeof(CaptureBufferInfo) == 0x20, "CaptureBufferInfo struct has incorrect size");

static const std::size_t SysMenuArgSize = 0x40;
Copy link
Member

@lioncash lioncash Apr 14, 2020

Choose a reason for hiding this comment

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

Suggested change
static const std::size_t SysMenuArgSize = 0x40;
constexpr std::size_t SysMenuArgSize = 0x40;

static can be omitted, since file-scope const/constexpr has internal linkage by default.


void Module::APTInterface::StoreSysMenuArg(Kernel::HLERequestContext& ctx) {
IPC::RequestParser rp(ctx, 0x37, 1, 2); // 0x00370042
auto size = std::min<std::size_t>(rp.Pop<u32>(), SysMenuArgSize);
Copy link
Member

@lioncash lioncash Apr 14, 2020

Choose a reason for hiding this comment

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

Suggested change
auto size = std::min<std::size_t>(rp.Pop<u32>(), SysMenuArgSize);
const auto size = std::min(std::size_t{rp.Pop<u32>()}, SysMenuArgSize);

@@ -650,6 +650,37 @@ void Module::APTInterface::CloseLibraryApplet(Kernel::HLERequestContext& ctx) {
rb.Push(apt->applet_manager->CloseLibraryApplet(std::move(object), std::move(buffer)));
}

void Module::APTInterface::LoadSysMenuArg(Kernel::HLERequestContext& ctx) {
IPC::RequestParser rp(ctx, 0x36, 1, 0); // 0x00360040
auto size = std::min<std::size_t>(rp.Pop<u32>(), SysMenuArgSize);
Copy link
Member

@lioncash lioncash Apr 14, 2020

Choose a reason for hiding this comment

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

Suggested change
auto size = std::min<std::size_t>(rp.Pop<u32>(), SysMenuArgSize);
const auto size = std::min(std::size_t{rp.Pop<u32>()}, SysMenuArgSize);

They are called by the Home Menu during initialization.

These functions will not see much use until we actually implement application jumping and system rebooting. For now we just need them to prevent some unmapped reads in the Home Menu due to the static buffers not being properly set up.
@Subv
Copy link
Member Author

Subv commented Apr 17, 2020

Addressed @lioncash comments

@zhaowenlan1779 zhaowenlan1779 merged commit 8967b40 into citra-emu:master Apr 18, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants