no_std support?

[shymega]

Hi,

I'm working on embedded firmware, and I need to use this crate for communicating
with the embedded microcontroller over UART. However, it needs to be used in
no_std environment.

I was wondering if there's any way to bring no_std support to this crate?

Thanks!

[liranringel]

It is definitely possible.
I think proc-macro-hack does not support it, but I believe there isn't any true limitation there also.
Perhaps we should no longer depend on proc-macro-hack, because function-like proc macros are already available for a long time.

Feel free to submit a PR if you want.

[shymega]

Yeah, the main issues are:

• byteorder
• proc-macro-hack.

The former in my tests can have no_std support, and that compiles fine. It's proc-macro-hack that is the issue. I think all I'd need to change really is the macro; not the BTS things?

[shymega]

Work so far: https://github.com/Cosmo-CoDiOS/structure/tree/no-std-support

[birkenfeld]

I'd also be interested!

One problem is that structure generates std::io::Error, and io is not available in core. Either a custom error type needs to be introduced (which can have a Into<io::Error> for easy porting), or the error type must change between std and no_std mode (which is a bit ugly?)

[shymega]

Latter could work, example of Cargo.toml:

[features]
std = []

So, we'd have no_std by default, and std as opt-in. Breaking change, sure, but it's the approach I've seen the most...

And then for things like Error usages, we'd have this:

#[cfg(feature = "std")]

which would be when std is available and enabled.

Or for when we don't have std available, i.e, embedded:

#[cfg(not(feature = "std"))]

Personally, I'd rather have something that allows both environments to be used with the same code.

[birkenfeld]

So, we'd have no_std by default, and std as opt-in.

Usually that's combined with default = ["std"] to avoid a) breaking and b) having to specify features for the common case.

Personally, I'd rather have something that allows both environments to be used with the same code.

I agree.

[shymega]

Oh, yeah - my crates do it the way I outlined, as by default they should run on no_std, and when running on the Cosmo Communicator aarch64 chip, it then should opt-in to std. But good point, thanks for the correction.

I can't think of a way to solve the io::Error issue though... do you want to collaborate on my repo? I can grant you access, and then we can submit a PR.

[birkenfeld]

Sure!

[shymega]

Added. It's part of a wider project, but I think I've limited your access scope to only the fork.

[liranringel]

@shymega @birkenfeld Indeed std::io is used here, not only for the error type but also the Read and Write traits are used (Read is used to read the input from any type that implements it, and similarly for Write).
What we can do is to create a custom error type, and new traits.
The new traits will be similar to what Read and Write gives. They will accept as input/output u8 slices, and when std is enabled, they will support anything that implements Read and Write (as happens now).

So changing the error type seems necessary.

[shymega]

OK, so I've managed to swap out raw::c_void to use the cty crate. I've also switched the String import to use core's, I'll create some conditionals soon. Main issue now is quote crate. It requires std, and whilst there was a draft PR for no_std support, it didn't seem to get actioned.

[shymega]

@birkenfeld My proposal is that we split the tasks between us, work on separate branches for each task, then merge into one branch, and I/you can submit a PR against this repo. Does that sound good? @liranringel You're welcome to be added to my fork if you want as well.

[birkenfeld]

I don't think cty is necessary, c_void is available at core::ffi.

Also, note that the proc-macro crate doesn't need to work on no_std, since it's only used at compile time. So quote is fine to use there.

[birkenfeld]

@liranringel are you fine with a MSRV of 1.45? Then we could drop proc-macro-hack.

[birkenfeld]

So one unfortunate point is that most of the byteorder features used for reading/writing are on Write/ReadBytesExt, which is inextricably linked to std::io (they inherit from Write/Read). We'd have to reimplement most of this functionality...

[liranringel]

@liranringel are you fine with a MSRV of 1.45? Then we could drop proc-macro-hack.

Yes, it's ok.

So one unfortunate point is that most of the byteorder features used for reading/writing are on Write/ReadBytesExt, which is inextricably linked to std::io (they inherit from Write/Read). We'd have to reimplement most of this functionality...

Notice that we can still use ByteOrder to act on u8 slices.

[birkenfeld]

Great, MSRV 1.45 should simplify a bit.

Notice that we can still use ByteOrder to act on u8 slices.

Ok, so the internals would then have to differ quite a bit, one part using WriteBytesExt::write_u32 etc. and the other LE::write_u32 etc.

[shymega]

I don't think cty is necessary, c_void is available at core::ffi.

Yeah, I didn't think core had it, but that reduces deps.

Also, note that the proc-macro crate doesn't need to work on no_std, since it's only used at compile time. So quote is fine to use there.

OK.

[shymega]

And yeah, I think this will take some work. @birkenfeld Are you planning to use features to not use std and to use std? (By default, std). I tried doing this in Cargo.toml:

[features]
default = "std"

It didn't like that. No idea why. I think we should also have a look at upping the Cargo Rust edition to 2021, maybe?

[shymega]

Never mind. I just saw you fixed that in the fork. By the way, you might have started on an older variant of my no_std port, so you'll be working on things that might be outdated.

[shymega]

OK, so I'm now adding conditionals for using core or std. My code uses conditionals such as: #[cfg(not(feature = "std"))] - sound good?

[birkenfeld]

That looks correct. There is no need for a feature for "core".

I think we should also have a look at upping the Cargo Rust edition to 2021, maybe?

That would dramatically increase MSRV, and there's no need for it.

[shymega]

That looks correct. There is no need for a feature for "core".

Alright, submitting to your branch now and pushing. if you have unmerged changes that affect that region, let me know and we can resolve conflicts. There is however a few errors transforming the variable input_string to &str, but that was present before the conditionals change.

I think we should also have a look at upping the Cargo Rust edition to 2021, maybe?

That would dramatically increase MSRV, and there's no need for it.

Sure, that's understandable. I've found Cross still doesn't support 2021 either, so my other crates may well go back to 2018 until that's fixed.

[shymega]

Pushed.

[shymega]

Possibly worth subscribing to for revisiting in the future: rust-lang/rfcs#2262

[shymega]

@birkenfeld I force-pushed an amended commit to your branch. Should have really squashed two commits.

[shymega]

I had a thought - what about this crate? https://github.com/technocreatives/core2

I know depending on external crates may not be ideal, but this would really help with the std::io side of things. Thought?

[liranringel]

I had a thought - what about this crate? https://github.com/technocreatives/core2

I know depending on external crates may not be ideal, but this would really help with the std::io side of things. Thought?

Can you give an example to how it's useful?

[shymega]

It's useful as it's a no_stdimplementation of std::io.

I've tested it on an XMODEM/YMODEM/ZMODEM I'm working on (WIP), and it compiles pretty well. The benefit for us is that we can either replace our uses of std::io completely with core2::io or use feature flags. It seems to be a drop-in replacement, and we would not need to rewrite our usages of std::io elements.

It's up to you really, but I feel that using core2 would work for implementing support on no_std. I almost got it working, but it's when using the quote! macro that the crate isn't "seen".

Personally, my preference really would be to use core2::io on no_std, and std::io on std. But that might introduce complexity.

[liranringel]

It's useful as it's a no_stdimplementation of std::io.

I've tested it on an XMODEM/YMODEM/ZMODEM I'm working on (WIP), and it compiles pretty well. The benefit for us is that we can either replace our uses of std::io completely with core2::io or use feature flags. It seems to be a drop-in replacement, and we would not need to rewrite our usages of std::io elements.

It's up to you really, but I feel that using core2 would work for implementing support on no_std. I almost got it working, but it's when using the quote! macro that the crate isn't "seen".

Personally, my preference really would be to use core2::io on no_std, and std::io on std. But that might introduce complexity.

I like the idea to use core2::io only for no_std. If it's really a drop-in replacement then I think we should add it as a dependency (for no_std).

[birkenfeld]

Problem is, you can't remove dependencies with feature flags. So core2 will have to be always a dependency, just not used if std is set.

[shymega]

Agreed. I think it should only be used on no_std as well. I don't know how fully complete it is, but we can see from the docs... maybe?

[liranringel]

Hmm on the other when I think about it, it may require users of structure to also make a conditional compilation.
So perhaps we should introduce a new error type.
For now, you can just return core2::io::Error. I think the new error type will be in another pull request.

[shymega]

Surely if we introduce the usage of core2 in the main structure crate as an import, just like with byteorder, and use the same 'features' as we do before, that wouldn't matter?

[liranringel]

Surely if we introduce the usage of core2 in the main structure crate as an import, just like with byteorder, and use the same 'features' as we do before, that wouldn't matter?

I'm not sure I understand you correctly.

[shymega]

Sorry, I meant something like how byteorder is imported in the main structure crate. L137 to L138 on src/lib.rs.

I was then thinking that if we're approaching this issue with the introduction of features for std and no_std enablement, we could just enable core2 in the main structure crate when no_std is targeted, and use std in the main structure crate we're not targeting no_std.

From my tests, it looks like core2::io is a drop-in replacement, and we shouldn't need to make too many changes.

[liranringel]

Sorry, I meant something like how byteorder is imported in the main structure crate. L137 to L138 on src/lib.rs.

I was then thinking that if we're approaching this issue with the introduction of features for std and no_std enablement, we could just enable core2 in the main structure crate when no_std is targeted, and use std in the main structure crate we're not targeting no_std.

From my tests, it looks like core2::io is a drop-in replacement, and we shouldn't need to make too many changes.

But if another library that also supports no_std uses structure, they might need conditional compilation to save/handle the error type, and I'd like to avoid that.

But again, for now you can submit a PR that makes it this way. New error type can be in another PR.

[shymega]

I think I'd rather submit a PR that supports the Error type in one PR. What sort of errors are you thinking?

[shymega]

This is my initial work so far - https://gist.github.com/shymega/328dd231fcd96711361996b6a739025b

It compiles but doesn't pass tests. I haven't committed yet as I'd like it to pass tests - hence the diff.

[liranringel]

I think I'd rather submit a PR that supports the Error type in one PR. What sort of errors are you thinking?

If the user assumed the type is IO error and return it and the return type is IO error, then different error type will cause a compilation error.

[shymega]

Not sure I fully understand. Been working on this some more. The main issue is the macro not importing the core2 dependencies. I'm going to commit to a different branch for further debugging, then merge into the PR branch when ready. I'd be grateful for any thoughts on my implementation. As for the Error type, I'm going to submit that in the same PR to avoid in-between PR breakages - what are your thoughts on adding a fmt::Display trait implementation to Error?

[shymega]

For reference: 46f87c2

[liranringel]

@shymega can you open a PR so I can give you feedback?

And about what I said about the error type - using core2 error type will just work since it uses std::error instead of its error module when std is enabled (I read the source code now). So you really can just replace it with the error type from core2.

About the suggestion to add fmt::Display - I don't think it's possible, since it's a trait defined on another crate, that you will try to implement on a type from another crate. It is not possible in Rust.

[shymega]

Sure, opening a PR now. I rebased the birkenfeld branch to merge my commits into one - that was necessary to tidy up the PR. So, as an FYI, this will break local history...

With the fmt::Display crate, we can ignore that then. Good to hear about core2::Error being alright, thanks.

[shymega]

I made a mistake and opened the PR on the fork. I've since reopened this repo as of tonight. A bit stupid of me to not realise for this long! I'm hoping we can work through the PR, as then I can use it in my embedded firmware daemon.