-
Notifications
You must be signed in to change notification settings - Fork 44
Implement ACPI time-alarm device #683
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
base: v0.2.0
Are you sure you want to change the base?
Implement ACPI time-alarm device #683
Conversation
|
Replaces #501, which was targeting the main branch (this now depends on the comms rework that's currently only available in v0.2.0). |
Cargo Vet Audit Passed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements an ACPI time-and-alarm device as defined in section 9.18 of the ACPI 6.4 specification. The implementation includes a new service for managing AC and DC power timers with wake capabilities, timer status tracking, and timezone/DST management. Power service integration points are stubbed for future implementation.
Changes:
- Adds
time-alarm-servicecrate with timer logic, persistent storage, and ACPI command handling - Adds
time-alarm-service-messagescrate with message serialization/deserialization for ACPI time/alarm operations - Integrates the service into the eSPI MCTP relay system
- Removes legacy TimeAlarm structures from EC memory map in favor of the new service-based approach
- Updates dependency versions (num_enum 0.7.4 → 0.7.5, PAC updates, embassy-imxrt updates)
Reviewed changes
Copilot reviewed 15 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| time-alarm-service/src/lib.rs | Main service implementation with ACPI command handling and power source management |
| time-alarm-service/src/timer.rs | Timer state machine with wake policy handling and expiration logic |
| time-alarm-service/src/task.rs | Async task definitions for command handling and timer tasks |
| time-alarm-service-messages/src/lib.rs | ACPI message types and serialization/deserialization |
| time-alarm-service-messages/src/acpi_timestamp.rs | ACPI timestamp structure with timezone and DST support |
| time-alarm-service/Cargo.toml | Service dependencies and feature configuration |
| time-alarm-service-messages/Cargo.toml | Message library dependencies |
| espi-service/src/mctp.rs | Integration of TimeAlarm service into MCTP relay |
| espi-service/Cargo.toml | Added time-alarm-service-messages dependency |
| embedded-service/src/relay/mod.rs | Explicit trait qualification to avoid ambiguity |
| embedded-service/src/ec_type/structure.rs | Removed legacy TimeAlarm structure |
| embedded-service/src/ec_type/mod.rs | Removed legacy time alarm memory map functions |
| embedded-service/src/ec_type/generator/ec_memory_map.yaml | Removed TimeAlarm from memory map definition |
| examples/rt685s-evk/src/bin/time_alarm.rs | Example demonstrating service usage with mock eSPI |
| examples/rt685s-evk/Cargo.toml | Added time-alarm service dependencies |
| Cargo.toml | Added time-alarm service workspace members and updated num_enum version |
| Cargo.lock, examples/*/Cargo.lock | Dependency lock file updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 22 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kurtjd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The mctp.rs file could use the same treatment in uart-service as is done here for eSPI service, but can be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 18 out of 24 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jerrysxie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@williampMSFT How can we do integration testing with the time-alarm service? Mocking out the underlying datetime and storage and checking that the correct fields has been set in response to ACPI commands? Using a mock timer to generate expiration and wake events?
2b56070
I have been testing with the ec-test-app + this PR (OpenDevicePartnership/ec-test-app#41), some changes to soc-embedded-controller to instantiate the TAD service (although that repo just broke due to another breaking change in the v0.2.0 branch that hasn't been uptaken yet), and some updates to the ACPI tables for my test device (in ADO, I can send you a pointer offline if you're interested). I'm hoping once we land on a widely-available reference platform, we can make that capability more publicly available. I've confirmed that I can manipulate the time by fiddling with the Windows time settings and see those reflected on the EC (and vice-versa), and I've implemented all the read operations in the EC test app. Write operations are tricker because they require a rework of how we're doing input in the ec-test-app, but I'd like to do that eventually too. I did have a PR go in yesterday to fix the build for tests in this repo (#696), so I'll look into writing some unit tests for this module as well. |
@williampMSFT Not blocking, it is tough to review the correctness of this kind of application logic, so having some mock to allow this to be unit testable would be ideal. If we can stand thinking about and looking at this, that would great! 👍 |
This change implements an ACPI time-and-alarm device as defined in section 9.18 of the ACPI 6.4 spec.
Such a device needs to interface with the not-yet-existent power service to trigger wakes and subscribe to power source notifications, so those interface points are stubbed out for now.
Resolves #138, #139, #140, #141, #142, #143, #144