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

rust: no_std support #148

Merged
merged 7 commits into from
Dec 10, 2023
Merged

rust: no_std support #148

merged 7 commits into from
Dec 10, 2023

Conversation

Sympatron
Copy link
Contributor

@Sympatron Sympatron commented Dec 10, 2023

The Rust crate currently depends heavily on std. Since OSDP is a protocol mostly for embedded devices I think no_std support is necessary.

This PR adds that support by:

  • Using core::* and alloc::* types instead of std::* where possible.
  • Defining a feature "std", which users can opt out of. Things like UnixChannel or the file API will not be available then.
  • Using parking_log and spin as a replacement for std::sync::Mutex. I chose parking_lot, because it is widely used, cross platform and supports lock_api and can therefore be used just like spin::Mutex for no_std without any changes. The use of spin should be temporary, because this is almost never what you really want. In the future we should probably let the user choose the concrete Mutex implementation that makes sense for their platform. I didn't do this, because I wanted to keep the changes backwards compatible for now.
  • Defining custom Read and Write traits instead of std::io::{Read, Write}, because the latter is not no_std compatible. I couldn't just replace them with embedded_io::{Read, Write} either, because those are not object safe and can therefore not be used with Box<dyn ...>. My traits are just very thin wrappers around the others with blanket implementations for everything implementing std::io::{Read, Write} or embedded_io::{Read, Write}.

I am open for discussion about my decisions in this PR. :)

@sidcha
Copy link
Member

sidcha commented Dec 10, 2023

Thank you for the PR @Sympatron. no_std support is here earlier than I anticipated. :) Let me take a look.

Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the PR.

pub use unix_channel::UnixChannel;

use crate::osdp_sys;
use crate::{osdp_sys, Mutex};
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have an explicit

#[cfg(feature = "std")]
use parking_lot::Mutex;
#[cfg(not(feature = "std"))]
use spin::Mutex;

insread of importing from crate::? It's confusing as it appears as though we are providing a mutex type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. This feels wrong. I wanted to have the Mutex selection only in one place, but I agree this was not the best solution.

I solved this now, by using Arc instead of Mutex in lib.rs.

@@ -183,4 +178,5 @@ impl Drop for PeripheralDevice {
}
}

#[cfg(feature = "std")]
Copy link
Member

Choose a reason for hiding this comment

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

I think we can have a no_std compliant file implementation as well. Was this left out just for simplicity or is there a design problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know of any way to solve this generally without std so I left it out. I think it would make more sense to let the user handle that themselves in the command callback.

@sidcha
Copy link
Member

sidcha commented Dec 10, 2023

I am in the middle of the -sys crate split. I guess it makes sense to merge this and then do the split (otherwise this branch would have way too many conflicts). I will fix any fallouts from this change post merge since no one is using this create ATM.

@Sympatron
Copy link
Contributor Author

Sounds good. I just pushed a fix to your comment about Mutex.

@sidcha sidcha merged commit 0fa4334 into goToMain:master Dec 10, 2023
9 of 10 checks passed
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