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.
time-alarm-service/src/lib.rs
Outdated
| self.send_acpi_response(respond_to_endpoint, &COMMAND_SUCCEEDED).await; | ||
| match response_payload { | ||
| AcpiTimeAlarmCommandResult::Timestamp(timestamp) => { | ||
| self.send_acpi_response(respond_to_endpoint, ×tamp.as_bytes()) |
There was a problem hiding this comment.
We are sending multiple OOB packets to respond to one request? Can we combine the response into one packet?
There was a problem hiding this comment.
Yeah, I have a note about that on line 407 and definitely intend to revisit this before checking in. I figured the solution for this might look a bit different if we end up adopting something like postcard, so didn't want to invest time in an optimization like that until I had reasonable confidence that it's not going to get deleted before the PR gets published :)
|
@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.
…d pending upcoming changes to the comms system
3 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.