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

nn_fp: Implement GetMyComment and UpdateCommentAsync #1173

Merged
merged 14 commits into from
Aug 24, 2024

Conversation

gf2p8affineqb
Copy link
Contributor

  • When starting a friend session, myComment (nexComment) will be stored in the session (NexFriends) variable
  • Using GetMyComment will then get session->myComment and convert the commentString to UTF16
  • Implemented nexComment.writeData
  • UpdateCommentAsync and UpdatePreferenceAsync are incredibly similiar to each other, so most of the code is copied over from that in order to make it work

@gf2p8affineqb gf2p8affineqb marked this pull request as draft April 12, 2024 22:12
@gf2p8affineqb
Copy link
Contributor Author

I'll have to convert this as a draft for now, still seems to have a few issues and I opened this PR a bit too soon.

@gf2p8affineqb
Copy link
Contributor Author

Something I have noticed, this does not work too well without nn_fp.rpl placed in cafeLibs/, GetMyComment doesn't get called (meaning you can't read your own comment) and UpdateCommentAsync seems to corrupt myPresence->GameKey on the client (comment still updates for other friends and the title remains untouched serverside), I'm not exactly sure on what to do about this.

{
FP_API_BASE();
auto ipcCtx = std::make_unique<FPIpcContext>(iosu::fpd::FPD_REQUEST_ID::UpdateCommentAsync);
ipcCtx->AddInput(newComment, 36);
Copy link
Member

Choose a reason for hiding this comment

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

newComment is a null-terminated big-endian wide string and our current convention for those is to define them as uint16be*
But also I took a quick look into nn_fp.rpl and it behaves differently here. The length passed to AddInput is dynamically calculated. I think this is how its implemented:

uint32 commentLen = CafeStringHelpers::Length(newComment, 17);
if(commentLen>=17)
    return FPResult_InvalidIPCParam;
ipcCtx->AddInput(newComment, sizeof(uint16be) * (commentLen + 1));    

So it seems to work similar to how UpdateGameModeWithUnusedParam passes it's string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked nn_fp.rpl and yeah, that implementation is accurate, though commentLen is added by 2 instead of 1

return FPResult_InvalidIPCParam;
if (!g_fpd.nexFriendSession)
return FPResult_RequestFailed;
DeclareInputPtr(newComment, char16_t, (vecOut[0].size / 2), 0);
Copy link
Member

Choose a reason for hiding this comment

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

char16_t should be uint16be
The way this reads the string from the buffer doesn't match IOSU behavior. Take a look at CallHandler_UpdateGameMode for the general pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code using CallHandler_UpdateGameMode as a reference, and it works fine

DeclareInputPtr(newComment, char16_t, (vecOut[0].size / 2), 0);
IPCCommandBody* cmd = ServiceCallDelayCurrentResponse();

auto utf8_comment = StringHelpers::ToUtf8((const uint16be*)newComment, vecIn[0].size);
Copy link
Member

Choose a reason for hiding this comment

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

You are passing the size in bytes to ToUtf8, but it wants the size in wide characters.

@Exzap
Copy link
Member

Exzap commented Apr 13, 2024

There seem to be some out of bounds reads but no bad writes (unless MY_COMMENT_LENGTH is wrong or we are using it in the wrong places?) So not sure what could be causing the corruption. The call to GetMyComment probably depends on some other state from the friend lib that we provide incorrectly/differently.

@gf2p8affineqb
Copy link
Contributor Author

The call to GetMyComment probably depends on some other state from the friend lib that we provide incorrectly/differently.

Is there a clear indicator to find where that state is?

@gf2p8affineqb
Copy link
Contributor Author

gf2p8affineqb commented Apr 19, 2024

Not much progress has been done lately, however, GetMyComment wasn't being called because it wasn't defined in nn_fp.cpp, also, the last 2 commits seems to have broken UpdateCommentAsync further by giving out an error when it gets called (both without nn_fp.rpl), so there needs to be more work done.

@gf2p8affineqb
Copy link
Contributor Author

gf2p8affineqb commented Apr 20, 2024

Another note, the GameKey corruption seems to come from UpdatePresenceAsync and not from UpdateCommentAsync, not sure what part exactly.

@gf2p8affineqb
Copy link
Contributor Author

gf2p8affineqb commented Aug 7, 2024

I know it has been a really long time since I last pushed a commit, I'd say the PR is almost finished, the only problem I have seen is that the comment doesn't update for the client in Friend List (it still updates for every other friend and if you restart the app the comment does change), I have a feeling this has something to do with the graphics and not the nn_fp library itself, I even tried patching out some friend list functions on real hardware to see where the text gets updated but I wasn't able to find anything.
Also the latest and second latest commit were supposed to be one single commit, but they got separated somehow, I don't know why.

{
FP_API_BASE();
auto ipcCtx = std::make_unique<FPIpcContext>(iosu::fpd::FPD_REQUEST_ID::GetMyComment);
ipcCtx->AddOutput(myPreference, sizeof(uint16be)*0x12);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use MY_COMMENT_LENGTH here instead of hardcoding 0x12. The definition of MY_COMMENT_LENGTH would have to be moved from CallHandler_GetMyComment into iosu_fpd.h

@Exzap
Copy link
Member

Exzap commented Aug 15, 2024

Is this ready to be merged or is more work being done? Asking since it's still marked as draft.

@gf2p8affineqb gf2p8affineqb marked this pull request as ready for review August 18, 2024 14:48
@Exzap Exzap merged commit dc9d99b into cemu-project:main Aug 24, 2024
5 checks passed
@gf2p8affineqb gf2p8affineqb deleted the mycomment branch August 24, 2024 20:28
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.

2 participants