Add service to implement an ACPI time-and-alarm device #501
Conversation
Cargo Vet Audit Failed
If the unvetted dependencies are not neededPlease modify Cargo.toml file to avoid including the dependencies. If the unvetted dependencies are neededPost a new comment with the questionnaire below to the PR to help the auditors vet the dependencies. Copy and paste the questionnaire as a new comment and provide your answers:1. What crates (with version) need to be audited? 2. How many of the crates are version updates vs new dependencies? 3. To confirm none of the already included crates serve your needs, please provide a brief description of the purpose of the new crates. 4. Any extra notes to the auditors to help with their audits. |
williampMSFT
force-pushed
the
user/williamp/es-tad
branch
from
60158ae to
3bc05dc
Compare
| service_storage: &'static mut OnceLock<Service>, | ||
| spawner: &embassy_executor::Spawner, | ||
| backing_clock: &'static mut impl DatetimeClock, | ||
| tz_storage: &'static mut dyn NvramStorage<'static, u32>, |
There was a problem hiding this comment.
This implementation requires 5 NVRAM slots. Most MCU have between 4-8 u32 words. To use 5 slots for timealarm service is a lot. Can we compact some of them into a single slot? Does all these need to be u32s?
There was a problem hiding this comment.
The other alternative is to put this in flash instead of NVRAM if the frequence to update these values are low.
There was a problem hiding this comment.
Yeah, we need 2x u32 for 'power source policy', 2x u32 for the alarm time, 2 bits for DST and 12 bits for time zone, so if the granularity we have for storage is u32s then we need at least 5 (I'm packing DST and time zone into the same register to save space).
We can definitely move this to using flash as persistent storage, though - might even be able to do an implementation of the NvramStorage trait that's backed by flash and leverage that? That way we could give the application layer control over how they want to spend their NVRAM registers
Also, tangentially related, that analysis that we need 5x u32 assumes we're willing to fall over in 2106 when a u32 overflows, which this currently does - I was kicking around the idea of using some of the leftover bits from the DST + TZ register as some sort of global offset for stored unix time but figured that might be out of scope for the first version of this.
|
@williampMSFT This service as it is should be able to be tested with a mock eSPI service or mock host service like https://github.com/OpenDevicePartnership/embedded-services/blob/main/examples/std/src/bin/debug.rs or https://github.com/OpenDevicePartnership/embedded-services/blob/main/examples/rt685s-evk/src/bin/mock_espi_service.rs. We should do that to test it out. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a service to provide ACPI time-and-alarm device functionality. The service handles timer operations for both AC and DC power sources and manages real-time clock operations with timezone support.
- Adds a new time-alarm-service crate with complete ACPI timer and clock management functionality
- Updates communication infrastructure to route messages to the new service
- Removes legacy time-alarm data structures from the embedded-service crate
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| time-alarm-service/src/timer.rs | Core timer implementation with persistent storage and power source state management |
| time-alarm-service/src/lib.rs | Main service implementation handling ACPI commands and timer coordination |
| time-alarm-service/src/acpi_timestamp.rs | ACPI timestamp format handling with timezone and daylight savings support |
| time-alarm-service/Cargo.toml | Package configuration and dependencies for the new service |
| espi-service/src/espi_service.rs | Updates message routing to remove legacy time-alarm handling |
| embedded-service/src/ec_type/structure.rs | Removes legacy TimeAlarm structure definition |
| embedded-service/src/ec_type/protocols/mctp.rs | Adds MCTP endpoint mapping for time-alarm service |
| embedded-service/src/ec_type/mod.rs | Removes legacy time-alarm message handling functions |
| embedded-service/src/ec_type/message.rs | Removes legacy TimeAlarmMessage enum |
| embedded-service/src/ec_type/generator/ec_memory_map.yaml | Removes TimeAlarm structure from YAML configuration |
| Cargo.toml | Adds time-alarm-service to workspace and required dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…penDevicePartnership#595) (OpenDevicePartnership#630) Mergeback commit 7a7f754 from main into v0.2.0 Co-authored-by: zhuyi2024 <79184862@qq.com> Co-authored-by: zhuyi <zhuyi@microsoft.com>
v0.2.0 mergeback: 01-05-2026
…ship#672) Mergeback commit c7f8e86 from OpenDevicePartnership#669 into v0.2.0 , which is failing CI.
…rvice (OpenDevicePartnership#670) This change breaks requests and responses sent over the comms bus into distinct types for each kind of service that leverages the bus to communicate to the host rather than a single enum with all possible request and response types. Previously, these were all defined in the embedded-service crate, which meant that new services couldn't be implemented without forking this repo and extending that enum. With this change, each service has an associated 'messages' crate that defines the types of requests and responses associated with that service and how to serialize/deserialize them. This incidentally removes a lot of boilerplate across all services associated with handling message types that are not intended for the service that receives them, since services no longer need to reason about other services' message types. In the course of this work, a few bugs in message serialization were found and fixed (out of order fields and the like).
…re (OpenDevicePartnership#673) This change factors out the boilerplate associated with generating relay types from the information about the set of types that are to be relayed. This should enable easier bringup of additional message relay services (e.g. UART). In the course of this work, I noticed that some of the nomenclature around results vs responses was a bit conflicting in the relay code, so also standardized that on Result meaning 'a specific generic type of the Result enum' and Response meaning 'the success case of a Result'. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adds a basic uart service which can act a host service when eSPI is not available. Still uses the `SmbusEspiMedium` even though the SMBUS header isn't necessary just to keep code changes on the host side minimal when switching between UART/ESPI. Did some testing on the IMXRT board and can send/receive MCTP packets from host over a COM port successfully. Might catch additional bugs as I work on getting the ratatui app working over UART and will fix them as they come. Resolves OpenDevicePartnership#605
…nDevicePartnership#679) Some serialization/deserialization was omitted because it wouldn't be used by the EC, however the service-message crates can be used outside the EC, so add them in. I experimented with zerocopy for this in OpenDevicePartnership#678 but it's a bit clunky and getting it to work for Battery messages will be challenging, so just rolled it by hand for now since that's what we are already doing. Can revisit later once we've decided on a serialization strategy. Tested these by using the service message crates to serialize/deserialize messages over UART in the ratatui and works fine for the ones I could test (though many of the battery commands are not yet supported by the ratatui app). Was going to wait until OpenDevicePartnership/embedded-batteries#39 is merged, but it seems relying on an updated `embedded-batteries` causes a ton of breakage and headache, so just have functions for converting in here and can update a later time.
…context handling (OpenDevicePartnership#668) - Removed `static_cell` dependency from Cargo.lock and Cargo.toml in examples and type-c-service. - Updated Type-C service methods to accept a reference to `intrusive_list::IntrusiveList` for better context management. - Modified various service methods to pass the controllers list where necessary, ensuring proper context usage. - Commented out unused code related to power policy channels and service initialization. - Adjusted the task closure to work with the updated service structure. - Enhanced error handling and logging throughout the service methods.
…#681) The cargo-vet CI job is currently failing with 36 unvetted dependencies with a note `These audits may have been made with a more recent version of cargo-vet`. Updating to the latest version of cargo-vet gets rid of these unvetted dependencies as it seems the vetting was done with cargo-vet@0.10.2. Why this is not a breaking minor change is beyond me. ``` Vetting Failed! 42 unvetted dependencies: aho-corasick:1.1.3 missing ["safe-to-deploy"] anyhow:1.0.99 missing ["safe-to-deploy"] cc:1.2.33 missing ["safe-to-deploy"] encoding_rs:0.8.35 missing ["safe-to-deploy"] libc:0.2.175 missing ["safe-to-deploy"] loom:0.7.2 missing ["safe-to-deploy"] memchr:2.7.5 missing ["safe-to-deploy"] paste:1.0.15 missing ["safe-to-deploy"] proc-macro2:1.0.101 missing ["safe-to-deploy"] regex-automata:0.4.13 missing ["safe-to-deploy"] ryu:1.0.20 missing ["safe-to-deploy"] scoped-tls:1.0.1 missing ["safe-to-deploy"] serde_json:1.0.143 missing ["safe-to-deploy"] syn:1.0.109 missing ["safe-to-deploy"] syn:2.0.106 missing ["safe-to-deploy"] thiserror:1.0.69 missing ["safe-to-deploy"] thiserror:2.0.16 missing ["safe-to-deploy"] thiserror-impl:1.0.69 missing ["safe-to-deploy"] thiserror-impl:2.0.16 missing ["safe-to-deploy"] unicode-segmentation:1.12.0 missing ["safe-to-deploy"] unicode-width:0.1.14 missing ["safe-to-deploy"] windows:0.61.3 missing ["safe-to-deploy"] windows-collections:0.2.0 missing ["safe-to-deploy"] windows-core:0.61.2 missing ["safe-to-deploy"] windows-future:0.2.1 missing ["safe-to-deploy"] windows-implement:0.60.0 missing ["safe-to-deploy"] windows-interface:0.59.1 missing ["safe-to-deploy"] windows-numerics:0.2.0 missing ["safe-to-deploy"] windows-result:0.3.4 missing ["safe-to-deploy"] windows-strings:0.4.2 missing ["safe-to-deploy"] windows-sys:0.52.0 missing ["safe-to-deploy"] windows-sys:0.59.0 missing ["safe-to-run"] windows-targets:0.52.6 missing ["safe-to-deploy"] windows-threading:0.1.0 missing ["safe-to-deploy"] windows_aarch64_gnullvm:0.52.6 missing ["safe-to-deploy"] windows_aarch64_msvc:0.52.6 missing ["safe-to-deploy"] windows_i686_gnu:0.52.6 missing ["safe-to-deploy"] windows_i686_gnullvm:0.52.6 missing ["safe-to-deploy"] windows_i686_msvc:0.52.6 missing ["safe-to-deploy"] windows_x86_64_gnu:0.52.6 missing ["safe-to-deploy"] windows_x86_64_gnullvm:0.52.6 missing ["safe-to-deploy"] windows_x86_64_msvc:0.52.6 missing ["safe-to-deploy"] ```
williampMSFT
force-pushed
the
user/williamp/es-tad
branch
from
22f4f00 to
7e78b6c
Compare
|
This change now depends on some of the comms rework stuff that's only available in the v0.2.0 branch, so closing this and creating a new PR that targets v0.2.0 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 84 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match expiration_time { | ||
| Some(dt) => { | ||
| timer_state.persistent_storage.set_expiration_time(expiration_time); | ||
| timer_state.wake_state = WakeState::Armed; | ||
|
|
||
| // Note: If the expiration time was in the past, this will immediately trigger the timer to expire. | ||
| self.timer_signal.signal(Some( | ||
| dt | ||
| .to_unix_time_seconds() | ||
| .saturating_sub(Self::get_current_datetime(clock_state).to_unix_time_seconds()).try_into() | ||
| .expect("Users should not have been able to program a time greater than u32::MAX seconds in the future - the ACPI spec prevents it") | ||
| )); |
There was a problem hiding this comment.
set_expiration_time computes the delay until expiration using i64::saturating_sub followed by try_into::<u32>(). If the programmed expiration time is in the past (which the comment says should be supported), saturating_sub will produce a negative value that cannot be converted to u32, causing this expect to panic instead of immediately expiring the timer. Consider explicitly handling the "already expired" case (e.g., clamping the delay at zero and signalling an immediate expiry) rather than relying on saturating_sub + try_into.
5 participants
This is an attempt to implement a service that can respond to requests that would be needed to implement an ACPI time-and-alarm device.