adding no_std support

[dscso]

It seams to pass all tests locally and also runs on the ESP32 as far as I see. it seams to address #50 as well. I am not that familiar with wasm though.

Fixes #58.

[zeenix]

It seams to pass all tests locally and also runs on the ESP32 as far as I see. it seams to address #50 as well.

Nice! Thanks for this PR. I'm sorry I didn't get to review it yet. I just started a new job so a bit busy with that. :) I'll try to have a look soon.

[zeenix]

Please add details to git commit message and PR description, summarizing the changes and approach you took. It doesn't have to be long at all.

[zeenix]

Do we want to use this unconditionally, even with std?

[dscso]

Aaaactually this is a very good note! I was way too fast in replacing this. I found this in another implementation of channels, but there are some significant downsides. I could change the implementation to only use spin in no_std environments... Unfortunately that means I have to do this quite a lot:

#[cfg(feature = "std")]
return self.inner.read().unwrap().queue.len();
#[cfg(not(feature = "std"))]
return self.inner.read().queue.len();

Alternatively I could also use parking_lot as drop in replacement for std::rwlock since it should be faster as well.
What do you think?

[zeenix]

#[cfg(feature = "std")]
return self.inner.read().unwrap().queue.len();
#[cfg(not(feature = "std"))]
return self.inner.read().queue.len();

You can abstract this away so the #cfg is only done in a few places. i-e implement wrapper type for RwLock that only provides the few methods we use.

Alternatively I could also use parking_lot as drop in replacement for std::rwlock since it should be faster as well.
What do you think?

Let's consider that if we bump into any issues with my suggestion for a simple abstraction type. :)

[dscso]

I just did a horrible thing:

#[cfg(not(feature = "std"))]
use spin::RwLock as Orig_RwLock;
#[cfg(not(feature = "std"))]
use spin::{RwLockWriteGuard, RwLockReadGuard};

#[cfg(feature = "std")]
use std::sync::RwLock as Orig_RwLock;
#[cfg(feature = "std")]
use std::sync::{RwLockWriteGuard, RwLockReadGuard};


#[derive(Debug)]
pub(crate) struct RwLock<T> {
    rwlock: Orig_RwLock<T>
}

impl<T> RwLock<T> {
    #[inline]
    pub(crate) const fn new(t: T) -> RwLock<T> {
        return RwLock {
            rwlock: Orig_RwLock::new(t),
        }
    }
    #[inline]
    pub(crate) fn read(&self) -> RwLockReadGuard<'_, T> {
        #[cfg(feature = "std")]
        return self.rwlock.read().unwrap();
        #[cfg(not(feature = "std"))]
        return self.rwlock.read();
    }
    #[inline]
    pub(crate) fn write(&self) -> RwLockWriteGuard<'_, T> {
        #[cfg(feature = "std")]
        return self.rwlock.write().unwrap();
        #[cfg(not(feature = "std"))]
        return self.rwlock.write();
    }
}

Is this what u ment? If yes, I'd commit. The only problem I could not solve yet is that there is no need to compile spin if running with std feature enabled. Do you know how do disable a dependency if a feature is not selected?

[zeenix]

Is this what u ment?

I did, yes. However,

• I'd suggest not using any imports but rather using full paths to keep complexity to minumim.
• don't forget to format it (empty lines between methods).

The only problem I could not solve yet is that there is no need to compile spin if running with std feature enabled. Do you know how do disable a dependency if a feature is not selected?

Hmm.. actually no and I think it's not possible. :( Cargo features are designed to be additive only.

Now we could consider parking_lot but:

1. It's not faster than std's locks anymore. We actually use to use it and it got removed in Use std RwLock instead of parking_lot. #38.
2. In parking_lot docs, I don't see any mention of nostd, nor std feature listed there.

[zeenix]

Ok perfect I fixed the remaining issues

Not sure I follow. What's the solution for only depending on spin when !std?

[dscso]

Actually I've read this article.

I think the optimal solution would be to use spin for everything except std, wasm and single core no interrupt cores. Here it should check whether the lock is already locked and if so panic.
But as I see here, in concurrent-queue they also use spinning in no_std environment.

I think the main problem emerges if you use a spin lock like this:

lock.read()
{
    lock.write()
}

Then we would have a deadlock.
But since this is not happening, a spin lock should be ok for this library's internal usage in a no_std environment.

[zeenix]

I think the optimal solution would be to use spin for everything except std

Sure but I'm talking about the dependency issue. How do we not have a dependency on spin when std is enabled?

lock.read()
{
    lock.write()
}

This is a very usual footgun with locks. I don't think we need to do anything smart about this except ensure that we don't do this. Our locks are internal so we don't need to care about our users having such issues.

[dscso]

I think there is no solution to this honestly. we could add a feature no_std that enables spin but it is not how other libraries handle it.
Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

[zeenix]

I think there is no solution to this honestly. we could add a feature no_std that enables spin but it is not how other libraries handle it.

That's something I'd be interested in. How are other libs doing this?

Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

Looks like spin has a std feature:

std enables support for thread yielding instead of spinning.

If I understand this correctly, we can use spin but enable std feature when std is enabled on async-broadcast? If so, this would also eliminate the need for the abstractions.

Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

Yeah but if we can't come up with another solution, I'd still consider it as it's preferable to depending on an a completely unused crate. We'll only want to import the bits we need though so it could be a not-too-bad solution.

[dscso]

Cool! Thanks for the clarification and no worries there is no need to rush.
I hope you can also use Rust in your Job :D

[zeenix]

Cool! Thanks for the clarification and no worries there is no need to rush. I hope you can also use Rust in your Job :D

I wouldn't take the job, otherwise. :)

[zeenix]

@dscso Ping. Do you think you can finish this soon? We're hoping to do a 1.0 soon and it'd be great to have this done before that.

[notgull]

I'd like to avoid using spinlocks in concurrent code.

[dscso]

I'd like to avoid using spinlocks in concurrent code.

I also think spinlocks are not the way to solve the issue. In my setting I was using the library in a single processor no interrupt setting, and there spin locks would be valid a solution.

I don't have the resources to implement a complete and clean no_std version without spinlocks.

[zeenix]

I don't have the resources to implement a complete and clean no_std version without spinlocks.

Fair enough. Thanks so much for your efforts here, still! I learnt a lot from the PR so it was useful even it it didn't get merged. 👍