-
Notifications
You must be signed in to change notification settings - Fork 43
Rework MCTP/comms to have separate types/messaging crates for each service #670
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?
Rework MCTP/comms to have separate types/messaging crates for each service #670
Conversation
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 refactors the MCTP/comms architecture to use separate message type crates for each service (battery, thermal, debug) instead of a monolithic enum. This architectural improvement enables third-party services to be implemented without forking the repository.
Key changes:
- Creates new message crates:
battery-service-messages,thermal-service-messages, anddebug-service-messages - Introduces
SerializableMessageandSerializableResponsetraits for message serialization - Removes centralized protocol definitions from
embedded-services - Simplifies service implementations by eliminating cross-service message handling
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| thermal-service-messages/src/lib.rs | New crate defining thermal service request/response types and serialization |
| debug-service-messages/src/lib.rs | New crate defining debug service request/response types and serialization |
| battery-service-messages/src/lib.rs | New crate defining battery service request/response types and serialization |
| thermal-service/src/mptf.rs | Refactored to use new message types, removed local type definitions |
| thermal-service/src/task.rs | Updated to use new request/response pattern |
| thermal-service/src/lib.rs | Removed MCTP-specific infrastructure |
| debug-service/src/task.rs | Updated to use new message types |
| battery-service/src/context.rs | Refactored ACPI handlers to return Result types |
| battery-service/src/acpi.rs | Simplified handler signatures, removed payload mutation |
| espi-service/src/mctp.rs | New routing logic for service-specific message types |
| embedded-service/src/ec_type/message.rs | Added SerializableMessage and SerializableResponse traits |
| embedded-service/src/ec_type/protocols/* | Removed centralized protocol definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dc38959 to
0d93247
Compare
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 36 out of 42 changed files in this pull request and generated 3 comments.
💡 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.
The general structure looks good to me and I don't see anything immediately problematic. When I implement the uart-service I should be able to identify and fix any kinks as they appear (if there are any).
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 36 out of 42 changed files in this pull request and generated 2 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 36 out of 42 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e7005fa to
7aa2115
Compare
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 36 out of 42 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.
|
Starting to work on the uart-service now, and I realize I would probably be needing to reuse a lot of what's in Perhaps it can be placed in |
Yeah, I think we probably want to write a macro that generates most of that file from a list of (service ID, request type, response type) tuples so it's more maintainable; I was going to start looking into that next but didn't want to block the PR on me learning rust macros, lol. I'll start looking into that |
Makes sense, I agree the macro idea sounds cool though more so I mean just as it is now doesn't look eSPI specific, so I've had to copy and paste the file exactly into my uart-service folder (since the module is private). Just wondering if you're okay with it being in a place accessible from multiple services. Though nb. I can also do this as part of my uart-service PR. |
I think the problem is that there are currently two pieces of information tangled together in that code - A) the knowledge of which things to relay, and B) the boilerplate around how to relay arbitrary things. I think we want the boilerplate to be common, but I think the knowledge of which things need to be relayed may vary by vendor customization so I don't think we want that in the embedded-services core. Ideally that'll eventually be a generic parameter passed in at the 'application layer' when the service is instantiated, but I think that's blocked on the work to migrate memory allocations from being statics in the module to being allocated at the 'application' layer and passed in at service instantiation time I think to unblock yourself in the immediate term the easiest thing to do might be to fork it, but hopefully by the time your uart service is ready to go I'll have a macro that can generate the boilerplate implemented and then we can switch to that and deduplicate everything but the service list |
|
Hm okay sounds good. For the time being I've just copy pasted |
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 38 out of 44 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.
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).