[v0.2.0 Refactor] [Type-C] Remove static_cell dependency and improve context handling #668
Conversation
tullom
force-pushed
the
type-c/remove-global-statics
branch
3 times, most recently
from
35a66ba to
ffb0e7e
Compare
…context handling - Removed `static_cell` dependency from Cargo.lock and Cargo.toml in examples and type-c-service. - Updated Type-C service methods to accept a reference to `intrusive_list::IntrusiveList` for better context management. - Modified various service methods to pass the controllers list where necessary, ensuring proper context usage. - Commented out unused code related to power policy channels and service initialization. - Adjusted the task closure to work with the updated service structure. - Enhanced error handling and logging throughout the service methods. - Initializing the service will now register all PD controllers before running the type-c task WIP
tullom
force-pushed
the
type-c/remove-global-statics
branch
from
8c975f6 to
16aadd1
Compare
tullom
requested review from
RobertZ2011,
asasine,
felipebalbi,
gjpmsft and
jerrysxie
asasine
left a comment
asasine
left a comment
There was a problem hiding this comment.
Looks functional so approved. All of my comments are therefore non-blocking.
&'static->&'ais a step for testability.- Adding methods to
Contextand getting rid of the external public static functions is a usability improvement. - Using
self.controllers()instead of passing a field as an arg to your own methods is hygeine and usability.
| /// Base storage | ||
| pub struct Storage<const N: usize, M: RawMutex> { | ||
| // Registration-related | ||
| context: &'static embedded_services::type_c::controller::Context, |
There was a problem hiding this comment.
Does this have to be &'static or can it be a lifetime parameter?
There was a problem hiding this comment.
Messed around with generisizing the lifetimes, but due to 'static restrictions in our intrusive list, its not feasible
There was a problem hiding this comment.
The above comment is a generalization for the entire service, in this case, it is possible to make Storage generic on a 'a lifetime for Context, and i have made that change.
| async fn process_external_command(&self, command: &external::Command) -> external::Response<'static> { | ||
| async fn process_external_command( | ||
| &self, | ||
| controllers: &intrusive_list::IntrusiveList, |
There was a problem hiding this comment.
Service stores the reference to &'a IntrusiveList so this doesn't need to be an arg to this method (or others like it); you can just refer to self.controllers() within this type's impls.
| id: controller_id, | ||
| data: ControllerCommandData::Reset, | ||
| })) | ||
| pub async fn reset_controller(ctx: &Context, controller_id: ControllerId) -> Result<(), PdError> { |
There was a problem hiding this comment.
The signature of these functions looks a lot like it should be a method on Context. Since the caller needs the Context object, they can simply context.reset_controller(id) rather than external::reset_controller(context, id)
impl Context {
pub async fn reset_controller(&self, controller_id: ControllerId) -> Result<(), PdError> {
// --snip--
}
}| id: controller_id, | ||
| data: ControllerCommandData::Reset, | ||
| })) | ||
| pub async fn reset_controller(ctx: &Context, controller_id: ControllerId) -> Result<(), PdError> { |
There was a problem hiding this comment.
nit: ctx -> context here and elsewhere. Dont use abbreviations when not necessary.
4 participants
static_celldependency from Cargo.lock and Cargo.toml in examples and type-c-service.intrusive_list::IntrusiveListfor better context management.