Skip to content
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

Document convention / guidelines / best practices on using peripherals in different modules, safely. #1780

Closed
AnthonyGrondin opened this issue Jul 10, 2024 · 3 comments · Fixed by #1947
Assignees
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@AnthonyGrondin
Copy link
Contributor

As the peripheral part of this library has come to stabilization, It could be relevant to write down a convention and guidelines on using different peripherals in different manners for common use cases.

Here are a few relevant questions that I can think of, that the document should answer in a way or another:

  • Is it better to pass an instance of the peripheral singleton, or pass the PeripheralRef, and instantiate the peripheral in the component?

  • How to properly initialize, destroy peripherals between multi-component usages (should user handle cleanup)?

  • Is it better to pass by owned value, or mutably borrow the peripheral instance? What peripherals are safe to borrow immutably? If we call a function in a loop, we must &mut T because passing T would drop the peripheral on the second iteration, etc.

  • What if we implement a trait that only takes &self, is there a way to safely access peripherals that might require a mutable access?

  • What about using optional peripherals Option<>?

@MabezDev
Copy link
Member

Some of these have a partial answer in the API-GUIDELINES document we created, I think this covers the first three.

What if we implement a trait that only takes &self, is there a way to safely access peripherals that might require a mutable access?

This probably needs to be accessed on a case-by-case basis, but generally if the trait is &self and the impl requires &mut self then worst case you'd need interior mutability via a mutex.

What about using optional peripherals Option<>?

Could you expand on this a little bit, does this mean options within constructor arguments or something else?

@jessebraham jessebraham added the documentation Improvements or additions to documentation label Jul 10, 2024
@jessebraham jessebraham added this to the 0.20.0 milestone Jul 10, 2024
@AnthonyGrondin
Copy link
Contributor Author

AnthonyGrondin commented Jul 11, 2024

Some of these have a partial answer in the API-GUIDELINES document we created, I think this covers the first three.

Oops, I didn't see this document before opening the issue.

What about using optional peripherals Option<>?

Could you expand on this a little bit, does this mean options within constructor arguments or something else?

So I recently had to implement edge_nal::TcpAccept for esp-mbedtls for an experiment, and esp-mbedtls has support for hardware acceleration using RSA. The Session needs Rsa in a mutable access (owned or mutably borrowed), due to the requirement of the Rsa driver. Now, for the Option<> part, this is simply an implementation part; the peripheral can be omitted on a per-session basis, to instead use the software implementation. So Session takes the peripheral wrapped in an Option.

For implementing a trait that takes &self I ended up defining my struct with:

rsa: core::cell::RefCell<Option<esp_hal::peripherals::RSA>>,

and creating a session with:

let session = Session::new(
    socket,
    "",
    Mode::Server,
    self.version,
    self.certificates,
    if let Ok(ref mut rsa) = self.rsa.try_borrow_mut() {
        rsa.as_mut()
    } else {
        None
    },
)?;

to get a mutable access to an optional peripheral.

EDIT: According to the guidelines, Session in esp-mbedtls should use the builder pattern, and I agree that this would simplify things up, I might open a PR for that.

@MabezDev MabezDev self-assigned this Jul 24, 2024
@jessebraham
Copy link
Member

@MabezDev has there been any progress here? Do you still intend to do some work here for the 0.20.0 release, or should we punt this down the road a bit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants