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 ac:u and ac:i services (Wi-Fi info) + example #140

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

adryzz
Copy link
Contributor

@adryzz adryzz commented Oct 27, 2023

Implements the ac service, both ac:u (user) and ac:i methods (intended for mset).

Did this because i needed some of these methods for an app

Methods implemented:

  • wait_internet_connection (returns immediately for some reason independently of network connection, have to investigate)
  • get_wifi_status
  • get_wifi_security
  • get_wifi_ssid
  • get_proxy_enabled
  • get_proxy_port
  • get_proxy_username
  • get_proxy_password
  • load_network_slot (ac:i)
  • ACU_SetAllowApType
  • ACU_SetNetworkArea
  • ACU_SetRequestEulaVersion
  • ACU_CreateDefaultConfig
  • ACU_ConnectAsync, ACU_GetLastErrorCode and ACU_GetLastDetailErrorCode (all at once)
  • ACI_GetNetworkWirelessEssidSecurity

(screenshot didnt work)
PXL_20231028_105524873

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

Mostly looks reasonable to me! Couple clippy lints that could use fixing and a few comments about error handling / enum types but mostly fine I think

ctru-rs/examples/wifi-info.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ac.rs Show resolved Hide resolved
ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ac.rs Show resolved Hide resolved
ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
@adryzz
Copy link
Contributor Author

adryzz commented Oct 30, 2023

also, will add that 2 days ago libctru 2.3.0 released, adding the 2 methods needed for this.

@adryzz
Copy link
Contributor Author

adryzz commented Oct 30, 2023

fixed a few of the issues, will now investigate the bindings, as that's the main issue apart from the libctru version bump

@adryzz adryzz force-pushed the acu branch 2 times, most recently from aa611c4 to 66c884d Compare October 30, 2023 21:49
@adryzz
Copy link
Contributor Author

adryzz commented Oct 30, 2023

never mind, i got the enum issue sorted out.

All that remains now is the libctru version bump for the load_network_slot setting.

also cleaned up the history a bit, now all commits build

@adryzz adryzz marked this pull request as ready for review October 30, 2023 21:50
@adryzz adryzz requested a review from a team as a code owner October 30, 2023 21:50
@adryzz adryzz requested review from AzureMarker and Meziu October 30, 2023 21:50
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait on a review! The new changes look good to me, but I think you have a few leftover comments that aren't relevant anymore, and you probably need to merge in the latest master since a couple things (in particular use_panic_handler()) have changed recently. If you have a chance to clean up some of those bits then I think this should be good to merge!

@adryzz
Copy link
Contributor Author

adryzz commented Nov 25, 2023

will do right now!

@adryzz
Copy link
Contributor Author

adryzz commented Feb 16, 2024

time to rebase this

@adryzz
Copy link
Contributor Author

adryzz commented Feb 16, 2024

perfect! the unit tests failing have nothing to do with this PR afaik, so it all works now!

@Jhynjhiruu
Copy link
Contributor

Seems like, once this and #156 get merged, all 3DS networking functionality besides Download Play is done - that'll be neat to see!

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

LGTM, @rust3ds/active if anyone else would like to have a look I'll wait a little but I think this is ready to merge!

CI may be completely broken due to rust3ds/actions#16 but the linter passed at least so I think we can merge and fix up any test issues that might show up once that's resolved

@adryzz
Copy link
Contributor Author

adryzz commented Feb 17, 2024

Seems like, once this and #156 get merged, all 3DS networking functionality besides Download Play is done - that'll be neat to see!

what other parts of libctru/3ds services are missing? i may just try implementing other stuff too

@Jhynjhiruu
Copy link
Contributor

Seems like, once this and #156 get merged, all 3DS networking functionality besides Download Play is done - that'll be neat to see!

what other parts of libctru/3ds services are missing? i may just try implementing other stuff too

I think at this point it's mostly less important stuff like boss, news, qtm etc. - though ir still needs some work, since the Circle Pad Pro code doesn't work properly on New 3DS. I don't think irrst is done, either, though that should probably be merged into hid in my opinion.
Just go to the libctru repo and compare the source files with what's done on the ctru-rs side.

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

/// Load the selected network slot, if present.
///
/// Note: this method requires `ac:i` access
Copy link
Member

Choose a reason for hiding this comment

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

Is this access given by the normal Ac::new() implementation, or should the user do something more to gain it? Reimplementing IPS calls isn't exactly a straightforward process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rosalina loader (e.g. the default) already gives all the arm11 permissions, but if using the hax2 loader, these permissions are missing unless ran from mset. can probably remove this, only thing is it may fail in the Citra CI because of permissions, although i haven't checked.

Copy link
Member

Choose a reason for hiding this comment

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

I think normally Citra grants basically all permissions since it's just emulated, although it's possible some methods are just unimplemented / stubs, I don't think permissions are usually denied.

// we don't really need space for the terminator
let mut vec = vec![0u8; len as usize];
ResultCode(ctru_sys::ACU_GetSSID(vec.as_mut_ptr()))?;
Ok(String::from_utf8(vec)?)
Copy link
Member

@Meziu Meziu Feb 23, 2024

Choose a reason for hiding this comment

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

Rather than returning the String's error, it might be wiser to check for Ac::get_wifi_status() and return an error based on that. We could try to do that in every case needed, such as proxy getters. Otherwise, we could write a custom ac::Error to hold those errors.

I'd like to avoid implementing Error::Utf8.

@@ -96,6 +96,8 @@ pub enum Error {
},
/// An error that doesn't fit into the other categories.
Other(String),
/// UTF-8 buffer to [String] conversion error
Utf8(std::string::FromUtf8Error),
Copy link
Member

Choose a reason for hiding this comment

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

Refer to my other comment about this.

ctru-rs/src/services/ac.rs Outdated Show resolved Hide resolved
/// # }
/// ```
#[doc(alias = "acWaitInternetConnection")]
pub fn wait_internet_connection(&self) -> crate::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

This function completely locks the thread until a connection is established, without a timeout. It's a bit... radical. We should probably suggest users to implement their own connection checks.

@adryzz
Copy link
Contributor Author

adryzz commented Feb 23, 2024

uhh good news. i tested around the get_wifi_status function, and there are definitely more values.

3dbrew has very little information on this, and i was only able to confirm these:

  • 0: Hardware Wi-Fi switch is disabled
  • 1: Enabled but not connected
  • 2: LAN connected
  • 3: WAN connected

I also was able to get in a few occasions the values 102 and 300 while turning on/off the Wi-Fi network, although i have no clue what they actually mean, and this suggests there may be even more values.

#[non_exhaustive] works i guess

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

LGTM other than fixing linter. CI should be up and working now again!

ctru-rs/src/services/ac.rs Show resolved Hide resolved
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.

The PR is done, but I really can't merge without these last few changes 🙏

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

adryzz commented Mar 28, 2024

oh, i completely forgot this was still open, will update asap

@adryzz
Copy link
Contributor Author

adryzz commented Mar 28, 2024

okay, one small issue: how do we deal with errors related to UTF-8 now? i checked and you can still run into those while being connected (e.g. a proxy username not being valid UTF-8, set from a homebrew app), so the wifi_status()-based error approach wouldn't work.

so do we create a custom AC error instead of propagating FromUtf8Error?

@Meziu
Copy link
Member

Meziu commented Mar 28, 2024

so do we create a custom AC error instead of propagating FromUtf8Error?

Yeah, that should be a good choice.

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

adryzz commented Mar 28, 2024

also, since we know the contents of the buffer are invalid text, we could still return the buffer as a Vec<u8>

and problem: the functions can return both crate::Error and ac::Error. how do we handle that? i see that crate::Error does not propagate any module-specific errors

@adryzz
Copy link
Contributor Author

adryzz commented Nov 13, 2024

i completely forgot about this PR being still open, will get it done with asap then

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.

4 participants