Skip to content

Conversation

tremho
Copy link
Collaborator

@tremho tremho commented Sep 21, 2025

This PR is for the "final" integration of the components built in the previous exercises -- battery/charger/sensor/fan.

Although we did a preliminary integration earlier for battery/charger, this one is much more than an extension of that experiment.

What I've tried to do here is to circle back on some of the more dubious aspects of prior implementation patterns used in previous examples and create this integration with no unsafe code, making better use of message channels, proper attachment of ODP services, and other hopefully better, if not best, practices.

The overall goal is an introduction to using ODP for EC builds, not a gold reference, after all. Still, I think this cuts a little above the previous exercises. As I have learned, I hope the reader will too.

The end product of this integration is a 3-fold application that is an interactive simulator with an app-like display, with a logging display, and an integration test app with reporting.

As always, a more-readable pre-published version of this may be found at https://tremho.github.io/odp-documentation/guide/how/ec/integration/1-integration.html

@tremho tremho requested review from kurtjd, philgweber and tullom and removed request for tullom September 21, 2025 01:44
@kurtjd
Copy link

kurtjd commented Sep 23, 2025

I think this is a good base, but in the future I would like to see this more closely align to some of our existing patterns (good demonstrations can be found in the examples folder).

The docs introduce patterns and abstractions which I don't feel necessarily reflect how we envision these components interacting. For thermal specifically, the docs don't appear to mention a lot of features the thermal service provides such as sensor profiles, threshold management, and fan auto-control, instead providing some manual implementations of these. In the future it would be nice for the docs to demonstrate how to use some of these provided features.

Others can chime in here, but I feel some of the sections focused on display rendering can be condensed or removed entirely. I think OEMs would be more interested in just simple examples of how to use these components instead of detailed explanations on how to set up proper rendering within the terminal. I also feel similarly on some of the detailed sections on building a test harness as I imagine OEMs might have their own way of doing that.

In a nutshell, my recommendation is the Integration section should be a bit more condensed and focused on providing a simple, clear example of how to use the components and how they interact that matches some of our existing patterns as seen in the examples linked above. However, this is a very good baseline for tweaking in the future and building off of.

@tremho
Copy link
Collaborator Author

tremho commented Sep 24, 2025

@kurtjd - Thanks for pointing out the non-alignment to ODP resources on thermal. 🤦🏻‍♂️ I have notes to this effect that I had intended to implement in this pass, but somehow (I blame my days of being sick) this focus got lost to other hassles as I worked through this. I am revisiting this implementation now and hoping to have revisions for a better aligned version posted here soon.

Copy link

@tullom tullom left a comment

Choose a reason for hiding this comment

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

I have a similar sentiment to @kurtjd.

This is a good baseline and you are using the components we have built out to interact with the various services. However, there are some nits i have with using the wrong structs, or not using structs (in the battery-service, we have a Wrapper struct that combines the Device and the Controller structs for you), or building out your own channels for IPC when we have that already integrated within the subsystems. I agree that std level display rendering should be condensed and/or removed because it adds complexity to the whole system when I would rather have documentation let the ODP framework shine and let the OEMs handle test harnesses and terminal rendering.

Same recommendation as Kurtis, try to condense this integration section and try to match the patterns we use in our example code. Regardless, great job with this and I think that this is valuable as a baseline and we can build off of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants