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

Add time_rtc example #23

Merged
merged 7 commits into from
Jan 25, 2022
Merged

Conversation

ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain commented Jan 18, 2022

Ref #18

Finally, I was able to get an example running for time/rtc, which as far as I can tell is functionally the same as the devkitpro version! It took me a while to figure out I should be using ctru_sys::* instead of libc::* – with the latter I was getting garbage data from gmtime. Any idea why that is? I suspect it has something to do with the linkage or initialization of these two reentrancy structures, but that's just a suspicion.

extern "C" {
    pub static mut _impure_ptr: *mut _reent;
}
extern "C" {
    pub static _global_impure_ptr: *mut _reent;
}

I also switched the dep to be libc = "0.2" since there is already a patch in the workspace root and it takes precedence AFAIK. If we want a more specific version, or e.g. different features, I can make that change as well.

Edit: oops, I meant to attach screenshots.

ctru_sys functions:

correct

libc functions (plus a debug print which shows the result from time() seems to be correct):

incorrect

@Meziu
Copy link
Member

Meziu commented Jan 18, 2022

I'll put this briefly on hold until #22 is closed, since that will require small changes in the Console::init.

Edit: Also, though it may not look like because of the work done to work with libc, the use of libc is mostly discouraged, since it uses a variety of functions either based on or rewritten for libctru. As much as true Unix compatibility would be cool, I don't think it'll ever be a goal. What is in libc is mainly to work with std.

@ian-h-chamberlain
Copy link
Member Author

Got it, thanks. Re: libctru vs libc, I think I finally figured out why it doesn't work. There is a chain of initialization which creates global state that is used by libctru, which we can probably hook into if we want to:

I guess that calling the libc bindings uses the newlib default reentrancy pointer or something instead of the libctru-initialized one. I wonder if this would potentially cause issues with std compatibility in the future, since it seems like that setup is supposed to make newlib use the dynamic reent (i.e. the libctru versions of things)...

@Meziu
Copy link
Member

Meziu commented Jan 18, 2022

I don't see how libc could be directly linking with those "different" implementations rather than libctru's, since libc only holds external statics and function definition. If the error is in gmtime, the one linked is the one in the devkitARM toolchain.

@ian-h-chamberlain
Copy link
Member Author

If the error is in gmtime, the one linked is the one in the devkitARM toolchain.

Yeah, I thought so too, but the devkitpro-included example in C worked fine, and the Rust example didn't work until I switched from using libc to ctru_sys bindings, which doesn't make much sense to me (shouldn't they both link to the same extern symbol?).

The implementation of gmtime itself must work, since otherwise I wouldn't be able to use ctru_sys::gmtime to get correct behavior. I'll see if I can find any more clues, but the difference in behavior between libc::gmtime and ctru_sys::gmtime seems to me to be like it could cause problems for std itself, if something used by std had a similar issue.

@AzureMarker
Copy link
Member

That chain of calls is pretty interesting! I found out that there's some other initialization that seems to happen automatically:
https://github.com/devkitPro/libctru/search?q=__appInit

@ian-h-chamberlain
Copy link
Member Author

Okay, I think I found the actual cause of the problem. All that reentrancy stuff seems like a red herring, and this was a simple ABI mismatch. With this diff in libc:

diff --git a/src/unix/newlib/mod.rs b/src/unix/newlib/mod.rs
index b1300f169..f1d738cb0 100644
--- a/src/unix/newlib/mod.rs
+++ b/src/unix/newlib/mod.rs
@@ -28,9 +28,16 @@ pub type socklen_t = u32;
 pub type speed_t = u32;
 pub type suseconds_t = i32;
 pub type tcflag_t = ::c_uint;
-pub type time_t = i32;
 pub type useconds_t = u32;
 
+cfg_if! {
+    if #[cfg(target_os = "horizon")] {
+        pub type time_t = ::c_longlong;
+    } else {
+        pub type time_t = i32;
+    }
+}
+
 s! {
     // The order of the `ai_addr` field in this struct is crucial
     // for converting between the Rust and C types.

The code works as expected with libc:: types instead of ctru_sys types. So I guess I can open a PR against the libc fork to cover this, but I'll leave the example as it stands to encourage any new users to stick with the ctru_sys bindings instead.

I suppose it's to be expected that bindgen would catch something like this vs handwritten bindings. To that end, I wonder if there's an easy way to automate type checking between the two and update libc where there is a mismatch.

@AzureMarker
Copy link
Member

Good find! Definitely make a PR against our libc fork

@ian-h-chamberlain
Copy link
Member Author

I found out that there's some other initialization that seems to happen automatically:
https://github.com/devkitPro/libctru/search?q=__appInit

That is also interesting, as it seems to imply there is not truly a need for the Rust code:

let hid = Hid::init().expect("Couldn't obtain HID controller");
let apt = Apt::init().expect("Couldn't obtain APT controller");

That said, the API handle those provide makes sense to me, so I'm not sure this should change much in the way of "idiomatic" code. Rust is all about explicit and those appInit calls seem a bit ... magic, in my mind. But good to know about in general!

@Meziu
Copy link
Member

Meziu commented Jan 19, 2022

Yes, many libctru services are initialiazed (in part or completely, I believe the SD card too works like this) by the init code before execution. The problem is obviously leaving the unsafe/undroppable implementation free to use by the user, which I believe isn't what Ferris would want us to do.

@Meziu
Copy link
Member

Meziu commented Jan 19, 2022

@ian-h-chamberlain Looking back your code I noticed something a bit obvious: you are not using std implementations to get the current time! I know this is the code implemented by the original example, but I believe these examples should show how to have that same functionality using our Rust toolchain. I don't know how far compatibility could go, but using a crate such as chrono would be a fun spin on the example's implementation, while still retaining the same output format.

This obviously depends on the libc implementation though... so that's a problem...

@ian-h-chamberlain
Copy link
Member Author

you are not using std implementations to get the current time!

Yes, I think when I tried before that chrono uses some libc APIs that aren't provided by the devkitARM version of newlib. I can try again though as that was while I was in the midst of trying to get my enviroment set up. I might find other libc changes to be made meanwhile...

@ian-h-chamberlain
Copy link
Member Author

Opened Meziu/libc#2 for libc changes

ctru-rs/Cargo.toml Outdated Show resolved Hide resolved

// Get the current time. ctru_sys bindings should be used rather than
// plain libc ones, for reasons I don't understand yet...
let unix_time: ctru_sys::time_t = unsafe { ctru_sys::time(std::ptr::null_mut()) };
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if this used std, though I see there's some discussion about how that might require some more work...

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't really require more work. It requires libc's PR to be merged...

// Get the current time. ctru_sys bindings should be used rather than
// plain libc ones, for reasons I don't understand yet...
let unix_time: ctru_sys::time_t = unsafe { ctru_sys::time(std::ptr::null_mut()) };
let time = unsafe { ptr::read(ctru_sys::gmtime(&unix_time as *const _)) };
Copy link
Member

Choose a reason for hiding this comment

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

If you aren't able to use std, then maybe replace this ptr::read with a simple dereference?

Suggested change
let time = unsafe { ptr::read(ctru_sys::gmtime(&unix_time as *const _)) };
let time = unsafe { *ctru_sys::gmtime(&unix_time as *const _) };

Co-authored-by: Mark Drobnak <[email protected]>
@ian-h-chamberlain
Copy link
Member Author

Ok, I tried both chrono and time to see if we could leverage std::time for this example.

time fails at runtime, as it seems to lack an implementation for the clock_gettime syscall:
time_runtime

chrono (with default-features = false, features = ["clock"]) fails to build, as it seems libc is missing a field:

error[E0609]: no field `tm_gmtoff` on type `tm`
   --> /Users/ianchamberlain/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.19/src/sys/unix.rs:101:26
    |
101 |         let gmtoff = out.tm_gmtoff;
    |                          ^^^^^^^^^ unknown field
    |
    = note: available fields are: `tm_sec`, `tm_min`, `tm_hour`, `tm_mday`, `tm_mon` ... and 4 others

I could try to add gmtoff to the libc fork, but its existence in newlib appears to be behind a compile-time flag, and I can't seem to find whether or not devkitARM defines it (it seems like the answer is no from what I have checked).

struct tm
{
  int	tm_sec;
  int	tm_min;
  int	tm_hour;
  int	tm_mday;
  int	tm_mon;
  int	tm_year;
  int	tm_wday;
  int	tm_yday;
  int	tm_isdst;
#ifdef __TM_GMTOFF
  long	__TM_GMTOFF;
#endif
#ifdef __TM_ZONE
  const char *__TM_ZONE;
#endif
};

Even after trying to add it to libc, it ends up failing for the same reason as time. So, it seems like we would need to either write our own implementation of clock_gettime to use these mainstream crates, or provide an API in ctru-rs to get the system time in a format compatible with one of those crates.

Let me know what you think makes sense – it would be nice to be able to use some popular crates for stuff like this, so maybe figuring out how to implement clock_gettime would be the best bet (perhaps as a follow-up to this PR).

@Meziu
Copy link
Member

Meziu commented Jan 20, 2022

Wait, clock_gettime isn't implemented? Weird, can't we implement it in linker-fix-3ds? I am pretty sure libctru supports that. Still, not really related to this PR anyways. This will be merged like this first, and then we'll see.

@Meziu
Copy link
Member

Meziu commented Jan 22, 2022

@ian-h-chamberlain make this compatible with the latest ctru so I can merge it.

@ian-h-chamberlain
Copy link
Member Author

@Meziu done, let me know if there's anything else needed. I can look into the clock_gettime implementation in the meantime, but I couldn't find any obvious references to it when I looked.

@Meziu Meziu merged commit 04eb6a5 into rust3ds:master Jan 25, 2022
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.

3 participants