Implement "timeout" support for Rust #78193
Conversation
The fugit crate implements tick-based timers, providing a `Duration` and `Instant` type that we can use for timeouts. This is an external crate, covered under the Apache-2.0 or MIT license. This will need to be vendored due to Rust policies. Signed-off-by: David Brown <[email protected]>
Use specific instances of `fugit::Duration` and `fugit::Instant`, with the clock parameter based on the configured clock rate. As the fugit times are fixed, this precludes using these time types with dynamically changing clocks. With this, is a local Timeout type that simply wraps `k_timeout_t`. Having this wrapper allows us to implement `From` from each of the timeout types. The zephyr timeout type can support either durations or instants, as well as two magic times, forever and no wait. Forever and NoWait are implemented as singleton types that also have conversions. The `zephyr::time::sleep` function then can take anything that can be converted to this wrapped Timeout type, including Forever, and NoWait. This template will be used for future API calls that need a timeout. Signed-off-by: David Brown <[email protected]>
Ensure this file is properly marked. Signed-off-by: David Brown <[email protected]>
lib/rust/zephyr/src/time.rs
Outdated
| //! Time types similar to `std::time` types. | ||
| //! | ||
| //! However, the rust-embedded world tends to use `fugit` for time. This has a Duration and |
There was a problem hiding this comment.
The code looks reasonable to me. Was there perhaps a paragraph missing here in the docs? The language sounds as though it might have moved around or come after something.
There was a problem hiding this comment.
You don't start the beginning of a section with "However"? I'll look through this and try to figure out if something is missing, or just what misfired in my brain.
|
Adding DNM. I'm going to write some tests for this, in addition to fixing the missing file. |
The `zephyr-sys` crate contains all of the generated bindings to the C API in Zephyr. The `zephyr::sys` module wraps these abstractions in as thin of a way as possible, but still allowing them to be used without 'unsafe'. Although the interfaces are "safe", because they are just wrappers around C calls, most aren't directly useful without unsafe, and this crate provides higher-level abstractions around these underlying primitives. This commit adds a definition for `K_FOREVER`, and `K_NO_WAIT`, which are too complicated for bindgen to autogenerate bindings for. We will rely on a test to ensure that the values match those in the C headers. Signed-off-by: David Brown <[email protected]>
Rewrite the doc comment for `zephyr::time`. A little bit of LLM help and this is much clearer. Signed-off-by: David Brown <[email protected]>
lib/rust/zephyr/src/time.rs
Outdated
| // From allows methods to take a time of various types and convert it into a Zephyr timeout. | ||
| impl From<Duration> for Timeout { | ||
| fn from(value: Duration) -> Timeout { | ||
| Timeout(k_timeout_t { ticks: value.ticks() as i64 }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(CONFIG_TIMEOUT_64BIT)] | ||
| impl From<Instant> for Timeout { | ||
| fn from(value: Instant) -> Timeout { | ||
| Timeout(k_timeout_t { ticks: -1 - 1 - (value.ticks() as i64) }) | ||
| } | ||
| } |
There was a problem hiding this comment.
These conversion should capture cases where the tick value conflicts with "well-known values" (K_FOREVER and K_NO_WAIT) and/or exceed the capacity of i64 (provided the underlying k_timeout_t::ticks is always an i64 then only when CONFIG_TIMEOUT_64BIT).
There was a problem hiding this comment.
I will put in checks for the magic values. The overflow will be detected in debug builds by the arithmetic overflow checking in Rust debug builds.
There was a problem hiding this comment.
The overflow check actually seems more important when time is defined as a 32-bit value. I'm not sure how to do the check without explicitly checking ffor debug assertions in the code.
There was a problem hiding this comment.
It also looks like that code probably won't even compile on a system with only 32-bit time.
I'll have to clean this up as well.
tests/lib/rust/time/src/times.c
Outdated
| { | ||
| .name = "K_FOREVER", | ||
| .units = UNIT_FOREVER, | ||
| .uvalue = 0, |
There was a problem hiding this comment.
Could probably use a better comment. I should actually just leave the .uvalue out, since it won't be used. These are hard coded. It is similar to how the .value isn't used with the Duration because it is calculated dynamically.
With these, units kind of indicate a special case, and there is no uvalue, just a value.
tests/lib/rust/time/src/lib.rs
Outdated
| // hack: sleep a bit to see if the twister failure can be earlier. | ||
| // zephyr::time::sleep(Duration::millis_at_least(1)); |
There was a problem hiding this comment.
pointing this out now just in case it gets forgotten
There was a problem hiding this comment.
So this was a stack overflow when twister would run the test, but not when run directly. Well, the stack always overflowed, but the test would pass if there was not tick interrupt during the test.
It seems that HW stack protection can be enabled anywhere, but is only implemented on some platforms.
Add a test to test that the time values in Rust (Duration and Instance) and up calculating the same results as the various macros used in the C code. Signed-off-by: David Brown <[email protected]>
If we have printk enabled, use that to output the desperation panic message when panic is called from Rust code. Signed-off-by: David Brown <[email protected]>
Too much rust code formatting leaks into my C. Fix these various whitespace errors. Signed-off-by: David Brown <[email protected]>
When the panic happens, call into the Zephyr `k_panic()` handler instead of just spinning. This should halt the whole system, not just the current thread. Signed-off-by: David Brown <[email protected]>
Add some additional comments to clarify that not all of the fields are used for all of the test types. Signed-off-by: David Brown <[email protected]>
When debug assertions are enabled, ensure that the time conversions are sensible. If there is an overflow on time with the assertions disabled, just use Default, which will be zero. Signed-off-by: David Brown <[email protected]>
Comment cleanups. Fix several minor typos in the comments. Signed-off-by: David Brown <[email protected]>
| fn panic(info :&PanicInfo) -> ! { | ||
| #[cfg(CONFIG_PRINTK)] | ||
| { | ||
| printkln!("panic: {}", info); | ||
| } | ||
| let _ = info; |
There was a problem hiding this comment.
Alternatively, you can call the argument _info and use it in the printkln!.
| fn panic(info :&PanicInfo) -> ! { | |
| #[cfg(CONFIG_PRINTK)] | |
| { | |
| printkln!("panic: {}", info); | |
| } | |
| let _ = info; | |
| fn panic(_info :&PanicInfo) -> ! { | |
| #[cfg(CONFIG_PRINTK)] | |
| { | |
| printkln!("panic: {}", _info); | |
| } |
There was a problem hiding this comment.
Hmm, is there a typical way this is done? Both seem awkward.
| let ticks: k_ticks_t = checked_cast(value.ticks()); | ||
| debug_assert_ne!(ticks, crate::sys::K_FOREVER.ticks); | ||
| debug_assert_ne!(ticks, crate::sys::K_NO_WAIT.ticks); | ||
| Timeout(k_timeout_t { ticks: -1 - 1 - ticks }) |
There was a problem hiding this comment.
The overflow checks be done after the offset (-1 -1) is applied.
There was a problem hiding this comment.
Depends on how valuable having a single point of failure is. Integer overflow on the subtraction will also panic when debug assertions are enabled.
|
Rust development is happening in zephyr-lang-rust instead of on the zephyr repo. This change has been merged there as zephyrproject-rtos/zephyr-lang-rust#1 |
Implement timeouts in Rust, in a way that fits with how well this is typically done in embedded Rust code.
This PR brings in the first external dependency that will be linked into Zephyr. I'm not sure if we want to handle these one at a time (I would guess the total would be less than 10), or to merge this into the collab branch, and then work through the vendoring solution (into modules) all together with the TSC before merging into main.
I expect all of the code to be licensed as is common with Rust, under a dual "or" license Apache-2.0 and MIT. The library in question in this change is Fugit which is the commonly used time (Duration and Instant) type used in embedded Rust. The interface is similar to these types in the std, but with more of a focus that the times will be quantities to a tick. As such, it provides both rounding down and rounding up constructors for converting, say, milliseconds into the Duration and Timeout types.
The underlying fugit types will be configured based on what type is used for Zephyr's
k_timeout_t, either a 32 or 64 bit time. The time base is entirely handled at compile time, and should result in similarly optimized code that you could get from usingK_MSECin C code.The only API change this provides is the addition of
zephyr::time::sleep()which accepts either a Duration, an Instant, or the singleton values Forever, or NoWait, which correspond to the various values that ak_timeout_tcan have. This template will be used for future APIs that use timeouts.