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

Implement UDS service with example #156

Merged
merged 14 commits into from
Apr 9, 2024
Merged

Conversation

Jhynjhiruu
Copy link
Contributor

This PR adds basic local networking functionality with the UDS service.
It's very much not feature-complete, but enough is implemented to be able to interface with the devkitPRO local networking example - i.e. joining a network, creating a network, sending and pulling packets, and basic configuration. This is probably enough functionality for most homebrew applications, and I plan to try and implement a Download Play sniffer with it.

Todo: finish implementing the rest of the functions; test properly; cleanup code?

An example is included (local-networking.rs; this is a mostly-but-not-entirely-complete port of the devkitPRO local networking example).

@Jhynjhiruu Jhynjhiruu requested a review from a team as a code owner February 8, 2024 01:24
Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

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

Great PR! A bit long for my liking, but I saw you followed our standards for the public API. Sadly, I have no way to thoroughly test the changes since I can't make a local network (I only have one 3DS). Is there any way I can test things out, maybe with an emulator?

@@ -150,7 +150,7 @@ impl IrUser {
shared_mem.shared_memory_layout,
);

Ok(())
Ok::<_, Error>(())
Copy link
Member

Choose a reason for hiding this comment

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

@AzureMarker Ehm, do you remember why a closure is used here? It doesn't seem to be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

This was done so I could use ?, like the unstable try block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was the easiest way to fix a compile error introduced by my implementation of FromResidual. If I handle errors a different way, this change can be reverted.

ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved

/// Returns the current [`ctru_sys::udsConnectionStatus`] struct.
///
/// TODO: should this return an error if not connected?
Copy link
Member

Choose a reason for hiding this comment

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

I believe so, the internal ctru_sys::udsConnectionStatus contains informations specific to an existing network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like calling it when not connected actually returns success. On New 3DS, I get

ConnectionStatus {
    status: 3,
    unk_x4: 0,
    cur_node_id: 2,
    unk_xa: 0,
    unk_xc: [0, 0, 0, 0, 0, 0, 0, 0],
    total_nodes: 0,
    max_nodes: 0,
    node_bitmask: 0,
}

and on Old 3DS I get

ConnectionStatus {
    status: 3,
    unk_x4: 0,
    cur_node_id: 0,
    unk_xa: 0,
    unk_xc: [0, 0, 0, 0, 0, 0, 0, 0],
    total_nodes: 0,
    max_nodes: 0,
    node_bitmask: 0,
}

consistently (they're identical besides cur_node_id).

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. At this point I wonder whether the status member itself specifies whether or not the client is connected to a network. We should probably try to understand its meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libctru doesn't describe it at all, so it would very much be a reverse-engineering effort. From my 20 seconds of experimentation, it looks like a value of 6 represents connected. Presumably it's a bitfield.

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain Feb 25, 2024

Choose a reason for hiding this comment

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

I think I found some details, seems like it's more of a weird enum with sparse value choices? https://www.3dbrew.org/wiki/NWMUDS:GetConnectionStatus#Status_values

Copy link
Member

Choose a reason for hiding this comment

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

The TODO doc comment should be removed now that it's fixed.

ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved
@Jhynjhiruu
Copy link
Contributor Author

Is there any way I can test things out, maybe with an emulator?

I've looked a little bit into it and I think it's maybe possible to connect to a 3DS's network with another device, but it'd require more knowledge about how Wi-Fi works than I have.
I think Citra supports relaying local networking data for multiplayer, but I don't know how to set it up.

@Jhynjhiruu
Copy link
Contributor Author

Jhynjhiruu commented Feb 9, 2024

Functions left to implement:

  • udsGenerateNodeInfo (not really intended for use standalone, I think, and doesn't really need to be in the safe API)
  • udsGetNodeInfoUsername (the safe API replaces the NodeInfo type and that decodes the string, so there's no need for this)
  • udsCheckNodeInfoInitialized (I have no idea why this is supposed to be useful, since you always know this already)
  • udsGenerateDefaultNetworkStruct (also not really intended for use standalone, used as part of Uds::new())
  • udsWaitDataAvailable (would be good to have, but not really needed since you can just check whether Uds::pull_packet() returns Some or None)
  • udsEjectClient
  • udsEjectSpectator
  • udsUpdateNetworkAttribute (definitely not really intended for use standalone, other functions call this internally)
  • udsSetNewConnectionsBlocked
  • udsAllowSpectators
  • udsGetNodeInformation

So, only about 5½ that would be actually useful, and they're mostly trivial.

ctru-rs/src/services/uds.rs Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/uds.rs Show resolved Hide resolved

/// Returns the current [`ctru_sys::udsConnectionStatus`] struct.
///
/// TODO: should this return an error if not connected?
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain Feb 25, 2024

Choose a reason for hiding this comment

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

I think I found some details, seems like it's more of a weird enum with sparse value choices? https://www.3dbrew.org/wiki/NWMUDS:GetConnectionStatus#Status_values

Comment on lines +850 to +855
!= ctru_sys::MAKERESULT(
ctru_sys::RL_STATUS as _,
ctru_sys::RS_OUTOFRESOURCE as _,
ctru_sys::RM_UDS as _,
ctru_sys::RD_BUSY as _,
)
Copy link
Member

Choose a reason for hiding this comment

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

What does this status represent? it still seems like it would be an error of some kind but we
return Ok for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See libctru and the UDS example. There's probably a nicer way to handle this, but this does work.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so this error code basically means a packet was still sent but some other error occurred, I guess? Seems fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, something like that. Maybe it'd be a good idea to hardcode it as a constant somewhere rather than calling it in the function, though - MAKERESULT presumably isn't const so it'd have to be a magic number with some explanation.

Copy link
Member

Choose a reason for hiding this comment

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

If it isn't const, we probably can/should make it const, same for any of those other helper functions that just do bit operations on Result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably, this would need to be changed in ctru_sys. Once somebody does that, I'll change this to be a little more elegant.

Doh

Co-authored-by: Ian Chamberlain <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Wow, this is a pretty intense example but it seems necessary given how many features there are in the uds service. I was wondering if some of this functionality could go onto Uds itself instead of just in this example, but honestly on first read I don't see any obvious things that make sense, so 👍 good with me


println!("UDS initialised");

enum State {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a lot like a manually implemented future state machine. could this have worked better with async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very possibly! I'm scared of async.

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've done some basic reading about futures, and I don't understand how this could be done with async. Could you explain how that would work? It would definitely be nice to clean up the code if it's possible.


/// Returns the current [`ctru_sys::udsConnectionStatus`] struct.
///
/// TODO: should this return an error if not connected?
Copy link
Member

Choose a reason for hiding this comment

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

The TODO doc comment should be removed now that it's fixed.

ctru-rs/src/services/uds.rs Outdated Show resolved Hide resolved
@Meziu Meziu merged commit cd80a33 into rust3ds:master Apr 9, 2024
2 of 4 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.

5 participants