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

Make libosdp no_std compatible #17

Merged
merged 5 commits into from
Oct 2, 2024
Merged

Make libosdp no_std compatible #17

merged 5 commits into from
Oct 2, 2024

Conversation

Sympatron
Copy link
Contributor

Depends on #16.

The feature arc is introduced, because some targets like thumbv6m-none-eabi don't support CAS operations and therefore don't support Arc. By disabling this feature these targets can be supported, too. I will probably looking into implementing get_version() and get_source_info() some other way that does not rely on CAS operations, but this way libosdp can at least be built for those targets now.

@Sympatron
Copy link
Contributor Author

I changed get_version() and get_source_info() to return &str. That way once_cell is not needed anymore. This is fine since the C implementation returns a statically allocated const char*.

libosdp-sys/src/lib.rs Outdated Show resolved Hide resolved
@sidcha
Copy link
Member

sidcha commented Oct 2, 2024

So, two things,

  • I added 8b8416c to make rebase on this branch easy. Please rebase it on master an the libosdp-sys commit in this PR should be dropped.
  • Why does libosdp-sys need std feature at all? maybe we can make both libosdp-sys and libosdp no_std only? Do you see any downsides to this?

@Sympatron
Copy link
Contributor Author

  • I added 8b8416c to make rebase on this branch easy. Please rebase it on master an the libosdp-sys commit in this PR should be dropped.

Ok, I will do that.

  • Why does libosdp-sys need std feature at all? maybe we can make both libosdp-sys and libosdp no_std only? Do you see any downsides to this?

For libosdp-sys you are definitely right. It does not use anything from std and should be no_std only.
libosdp does use no_std incompatible stuff like thiserror and std::io::Error though.

@Sympatron
Copy link
Contributor Author

Done

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.

Apart from those two nits, LGTM. Thanks.

libosdp/src/commands.rs Outdated Show resolved Hide resolved
fn from(value: E) -> Self {
match value.kind() {
//TODO determine if this is the correct error kind
// embedded_io::ErrorKind::TimedOut => ChannelError::WouldBlock,
Copy link
Member

Choose a reason for hiding this comment

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

embedded_io::ErrorKind::TimedOut cannot be put to ChannelError::WouldBlock since embedded_io defines read/write to always block, there is no equivalent to ChannelError::WouldBlock. so this comment-line can be removed.

This probably indicates a deeper issue as LibOSDP expects all read/write to be non-blocking. We need to explore ReadReady/WriteReady traits and adjust the channel callbacks to handle the outcomes correctly.

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. But currently we are only using embedded_io::Error, so that is mostly a problem implementers of Channel will have, not of libosdp directly.

This should probably be pointed out more clear in the doc comments of Channel though.

A quick google search revealed that none of the HALs I could find implement ReadReady/WriteReady currently. I think WriteReady is not stricly necessary, but Read alone will simply not work as expected. That is indeed problematic.

Nevertheless, I don't think this has to block this PR. It can be handled in a future PR.

@Sympatron
Copy link
Contributor Author

BTW this is a semver breaking change, because of the change to get_version() and get_source_info().

@sidcha
Copy link
Member

sidcha commented Oct 2, 2024

True, but I think we should wait until we have some level of API stability before enforcing SemVer.

@sidcha sidcha merged commit 33f7076 into goToMain:master Oct 2, 2024
2 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