Skip to content

Conversation

@williampMSFT
Copy link
Collaborator

@williampMSFT williampMSFT commented Jan 23, 2026

This change implements read operations for the ACPI time-alarm device / real-time clock.
As part of debugging this change, also fixed a resource leak in eclib and propagated an error code that we were previously dropping.

@williampMSFT williampMSFT marked this pull request as ready for review January 29, 2026 18:08
@williampMSFT williampMSFT requested review from a team and philgweber as code owners January 29, 2026 18:08
Copy link
Contributor

@kurtjd kurtjd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the RtcSource trait methods be added to the Source trait? The original idea was the one trait handles interacting with all services. Though you might have other reasons for keeping it separate, not sure.

@williampMSFT
Copy link
Collaborator Author

Can the RtcSource trait methods be added to the Source trait? The original idea was the one trait handles interacting with all services. Though you might have other reasons for keeping it separate, not sure.

Yeah, I wanted to get feedback on this - I thought splitting it out into individual traits for each service and then having a single aggregate trait that implements all those other traits might make it easier to tell what belongs to which tab and potentially modularize in future, but I can pretty easily merge it back in if we want to go the single-trait route

@kurtjd
Copy link
Contributor

kurtjd commented Jan 29, 2026

Can the RtcSource trait methods be added to the Source trait? The original idea was the one trait handles interacting with all services. Though you might have other reasons for keeping it separate, not sure.

Yeah, I wanted to get feedback on this - I thought splitting it out into individual traits for each service and then having a single aggregate trait that implements all those other traits might make it easier to tell what belongs to which tab and potentially modularize in future, but I can pretty easily merge it back in if we want to go the single-trait route

Sounds good to me then, I can see the benefit of that. I'll just submit a PR after this then that splits battery and thermal

@kurtjd
Copy link
Contributor

kurtjd commented Jan 29, 2026

d'oh, just as I said that saw you updated the PR

@williampMSFT
Copy link
Collaborator Author

d'oh, just as I said that saw you updated the PR

I can flip it back if we want to go with the traits-per-interface approach - I personally slightly favor the trait-per-interface thing but I don't feel that strongly about it :)

@kurtjd
Copy link
Contributor

kurtjd commented Jan 29, 2026

d'oh, just as I said that saw you updated the PR

I can flip it back if we want to go with the traits-per-interface approach - I personally slightly favor the trait-per-interface thing but I don't feel that strongly about it :)

If you want to revert to what you had I'm fine with that :) (though might want to see if any other folks have opinions first)

@williampMSFT
Copy link
Collaborator Author

d'oh, just as I said that saw you updated the PR

I can flip it back if we want to go with the traits-per-interface approach - I personally slightly favor the trait-per-interface thing but I don't feel that strongly about it :)

If you want to revert to what you had I'm fine with that :) (though might want to see if any other folks have opinions first)

Sounds good, pushed out an iteration that flips it back; we'll see what everyone has to say and if we want to go the other route I can flip back again

Copy link
Contributor

@kurtjd kurtjd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as far as the Rust changes go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants