uefi-raw: time feature with conversions to/from OffsetDateTime#1899
uefi-raw: time feature with conversions to/from OffsetDateTime#1899
Conversation
7b5985f to
9fe899b
Compare
9fe899b to
e8b000e
Compare
e8b000e to
710b3d4
Compare
This makes it clearer that this value can be any of those three. Reference: https://github.com/tianocore/edk2/blob/b7a715f7c03c45c6b4575bf88596bfd79658b8ce/EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.c#L253
53f2bbd to
3b96089
Compare
ba4f75b to
730cd38
Compare
By adding a convenient integration with `OffsetDateTime` of the `time` crate, we make `Time` much more usable. This commit adds the helpers `to_offset_date_time()` and `to_offset_date_time_with_default_timezone()`: the latter helps to better cope with Time::UNSPECIFIED_TIMEZONE.
730cd38 to
59ae1af
Compare
This helps to better keep an overview.
|
|
||
| //! Date and time types. | ||
|
|
||
| use bitflags::bitflags; |
There was a problem hiding this comment.
Hint: this looks quite invasive in github UI. I just moved everything feature( = "time") related into mod time_crate_integration
| /// Indicates the time should be interpreted as local time. | ||
| pub const UNSPECIFIED_TIMEZONE: i16 = 0x07ff; | ||
|
|
||
| #[cfg(feature = "time")] |
There was a problem hiding this comment.
Hint: this looks quite invasive in github UI. I just moved everything feature( = "time") related into mod time_crate_integration
| } | ||
|
|
||
| #[cfg(feature = "time")] | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
Hint: this looks quite invasive in github UI. I just moved everything feature( = "time") related into mod time_crate_integration
| pub struct Daylight: u8 { | ||
| /// Daylight information not available or not applicable to the time | ||
| /// zone. | ||
| const NONE = 0; |
There was a problem hiding this comment.
The bitflags docs recommend against defining a zero flag.
There was a problem hiding this comment.
ah, that's good to know. How about using newtype_enum!() then?
|
|
||
| [features] | ||
| default = ["time"] | ||
| time = ["dep:time"] |
There was a problem hiding this comment.
A convention I've seen recommended for crates where the major version can be expected to change (which is the case here; I think time is planning on a 1.0 transition at some point), is to include the version in the feature name. Something like time03.
| uguid.workspace = true | ||
|
|
||
| [features] | ||
| default = ["time"] |
There was a problem hiding this comment.
I think we should remove this, uefi-raw should not enable optional deps by default.
| bitflags = "2.0.0" | ||
| log = { version = "0.4.5", default-features = false } | ||
| ptr_meta = { version = "0.3.0", default-features = false, features = ["derive"] } | ||
| time = { version = "0.3", default-features = false, features = [] } |
There was a problem hiding this comment.
nit:
| time = { version = "0.3", default-features = false, features = [] } | |
| time = { version = "0.3", default-features = false } |
| } | ||
|
|
||
| #[cfg(feature = "time")] | ||
| mod time_crate_integration { |
There was a problem hiding this comment.
Can we move this module (and the tests below) to a separate file? I think that'd be a little easier to read.
| } | ||
|
|
||
| impl Time { | ||
| fn _to_offset_date_time( |
There was a problem hiding this comment.
nit: we should avoid underscore-prefixed functions as that turns off unused function warnings. example
| /// let odt: OffsetDateTime = t.to_offset_date_time_with_default_timezone(default_offset).unwrap(); | ||
| /// assert_eq!(odt.offset(), default_offset); | ||
| /// ``` | ||
| pub fn to_offset_date_time_with_default_timezone( |
There was a problem hiding this comment.
Rather than providing this, I think we should have a conversion to PrimitiveDateTime. The caller can then choose to convert that to an OffsetDateTime with assume_offset.
There was a problem hiding this comment.
Oh that's a better design. Thanks!
|
|
||
| /// Errors that can happen when converting a [`Time`] to a [`OffsetDateTime`]. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| pub enum ToOffsetDateTimeError { |
There was a problem hiding this comment.
I'm not convinced that having detailed error enums is going to be useful to end users, and it potentially makes the API harder to evolve later without a major version bump. The jiff crate for example opted for a mostly-opaque error type: https://docs.rs/jiff/latest/jiff/struct.Error.html.
What do you think about having just one public type like pub struct TimeError(InternalTimeError)? InternalTimeError would not be public, and would be an enum allowing us to provide the detailed error info via Display, but not make it programmatically acessible.
There was a problem hiding this comment.
I think having a single time error type is fine. thanks for the pointer!
|
Closing this in favor of a new PR with a new design to keep the discussion clean and focused |
This PR introduces a new
timefeature that enables interoperability between our EFITimestruct and thetimecrate'sOffsetDateTimetype. The goal is to make it easier to work with standard Rust time types while maintaining the safety and validation guarantees of theTimestruct.The PR only focuses on uefi-raw,
uefifollows in a next step.This replaces #1896 as it is clearly a better strategy to move as much of the time handling as possible into a bulletproof crate. I decided to go with
timeas it is the defacto standard of the ecosystem.Highlights
New conversions
Time::to_offset_date_time()-> converts a validTimeinto anOffsetDateTimein UTC or using a provided default local timezone.Time::to_offset_date_time_with_default_timezone(local: UtcOffset)-> handlesUNSPECIFIED_TIMEZONEsafely with an optional local offset.TryFrom<OffsetDateTime>forTime-> allows converting standard Rust times into EFITime, with proper error handling for unrepresentable years or offsets with non-zero seconds.Error handling
ToOffsetDateTimeErrorandFromOffsetDateTimeErrorfor detailed conversion errors.Comprehensive unit tests
Benefits
Timemore ergonomic to use with standard Rust time APIs.Checklist