Add `no_std` support to structure #12

shymega · 2022-05-05T19:28:13Z

This is a draft PR that adds no_std support to the crate, thereby enabling embedded hardware to use the crate

It is a collaborative effort with @birkenfeld and @shymega (I), and so the discussion will be moved here.

I had made a grave error in originally opening a PR for this, in that I opened it here - Cosmo-CoDiOS#2 - a mistake on my part.

Hopefully, this can now be reviewed.

liranringel · 2022-05-08T23:00:34Z

Great!
I’m quite busy these days, I hope I’ll find time this week to review this PR.

shymega · 2022-05-08T23:01:54Z

Thanks. It doesn't pass tests quite yet when you use core2 in the macro, I haven't committed those. Should have mentioned it. It still needs work.

liranringel · 2022-05-08T23:05:08Z

Ah, ok.
Do you prefer some feedback now or should I wait?

shymega · 2022-05-12T13:53:26Z

Some feedback now would be good. The problem with adding core2 in the macro is that it breaks all tests. I suspect I'm doing it wrong.

shymega · 2022-08-17T17:37:51Z

Hey @liranringel, sorry to prod again, but I'm stuck with the PR so far, and need some feedback on its current state. Thanks!

liranringel · 2022-08-20T14:53:29Z

@shymega it's looking good.
For now, you just upgraded quote and moved from proc-macro-hack to proc-macro2 which is great!
Do you want to merge these changes now, and make further changes later?

shymega · 2022-08-23T11:02:52Z

Well, it's not complete yet. It doesn't run on no_std, because the macro itself still uses std. I need some help figuring out the macro. Can you help?

liranringel · 2022-09-11T10:15:16Z

@shymega I'm sorry I don't have much time these days...

shymega · 2022-09-14T22:28:17Z

No worries. Been working on it some more. @birkenfeld I've pushed with my latest change, do you have a moment to look through? I'm new to macros with Rust, I barely use them, so I'm blindly coding. Thanks! Specifically, this commit: e0e4c81

I'm also going to squash all these commits into one once tests pass and it works on no_std.

Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk>

shymega · 2022-11-18T19:11:45Z

Hi @liranringel, I've fixed the commits, so this should merge nicely. I would be grateful if you could review my work, as I'm not certain it's clean. But I hope it works.

liranringel · 2022-11-20T20:50:20Z

Thank you @shymega!
While I'm a bit busy these days, I'll try to review your changes as soon as I can.

shymega · 2022-11-26T21:22:25Z

@liranringel Thanks. Shall we see if @birkenfeld has any thoughts on it too?

birkenfeld · 2022-11-27T11:32:09Z

I promise I'll have a look next week.

tests/test.rs

src/lib.rs

birkenfeld · 2022-11-30T17:44:57Z

src/lib.rs

+pub extern crate core2;

+#[doc(hidden)]
+pub use core2::io::{Cursor, Error, ErrorKind, Read, Result, Write};


IMO this should only be used from core2 if std is not active. Otherwise import from std::io

So, I've since squashed this PR into two commits, but it seems this review is out of date, as on my branch I can't see this line. I've since changed it to only use core2 if std isn't enabled, as-per your review.

structure-macro-impl/src/lib.rs

Cargo.toml

src/lib.rs

structure-macro-impl/src/lib.rs

shymega · 2022-12-02T23:14:41Z

OK, so after running tests with no_std enabled, looks like there's a lot more to fix. Vec breakages and such.

birkenfeld · 2022-12-03T06:54:09Z

Yeah, byteorder's WriteBytesExt and ReadBytesExt are not defined on no_std (in hindsight, makes sense, since they use std::io too).

shymega · 2023-01-22T21:16:57Z

@birkenfeld Sorry, family stuff got hold of me. I'm just thinking of the best way to handle these edge cases. We could open an issue with byteorder and request for no_std support.

shymega · 2023-02-02T22:39:03Z

I've just enquired about the byteorder crate repository. Whilst my newer firmware doesn't need to pack data for serial, the older firmware is still heavily reliant on a similar approach to Python's struct.

I will keep this PR updated.

shymega · 2023-02-02T22:56:03Z

The comment was replied to, it's an interesting problem. We can use u64::to_le_bytes in std, but I suppose the question is, can we do that in no_std? Do we want to reimplement {Read,Write}BytesExt in the structure crate?

I'm also wondering if it's just "easier":tm: to implement the packing methods into the firmware transfer protocol on my end, and close this PR...

shymega · 2023-02-26T19:23:08Z

Just wanted to follow this up with both of you (and anyone else watching). I think the easiest solution is just to copy io.rs from byteorder and modify it to use the byteorder crate for the ByteOrder trait exposed by that crate, plus core and core2, depending on the feature flag.

OR we could implement the *Ext traits in structure, and do it that way.

The simplest solution is usually the easiest one, but I'm conscious of licensing, and making sure we don't end up with spaghetti code.

Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk>

@birkenfeld

As per @birkenfeld's review of this branch, proc-macros can use `std` during compile-time - there's no issues with `no-std`.

@birkenfeld

For `proc_macro`, as per @birkenfeld's PR review.

shymega · 2023-08-05T22:27:11Z

OK, so tests now pass on std. I need to squash the remaining commits when ready, but we're just left with byteorder not working without std.

I'm lost with the proc-macro - I haven't really dipped my toes in macros before, so it's very much new to me. I think the main change we need to make for this PR to be merged is to use to_le_bytes() and such from core when std isn't enabled, and vice versa.

Keen to get this PR in. It would mean I could then communicate over UART on a no-std environment.

This adds a slightly modified module from `byteorder`, which should run on `no_std`. The `src/lib.rs` has been modified in light of this change.

liranringel · 2023-11-21T13:19:05Z

@shymega sorry for not being here for a long time.
What is the PR status? Is it ready for review?

shymega · 2023-12-10T19:52:03Z

@liranringel:

@shymega sorry for not being here for a long time. What is the PR status? Is it ready for review?

So, currently, it works with the std feature flag.

I did attempt to copy the ByteorderExt traits into the PR, and make them work with core2. However, the proc-macro didn't like it.

I'm not sure how to proceed, and it would be great to get external thoughts on the PR as it is now.

shymega · 2024-03-21T23:17:34Z

@shymega sorry for not being here for a long time. What is the PR status? Is it ready for review?

Hey @liranringel - were you able to look into the macro?

No rush :-)

shymega marked this pull request as draft May 5, 2022 19:28

shymega force-pushed the birkenfeld branch from e0e4c81 to 240463c Compare September 27, 2022 21:38

Remove use of proc-macro-hack, update to quote 1.0.

648d4be

Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk>

shymega force-pushed the birkenfeld branch from 240463c to 96c1c33 Compare November 18, 2022 19:10

shymega marked this pull request as ready for review November 18, 2022 19:10

birkenfeld suggested changes Nov 30, 2022

View reviewed changes

shymega force-pushed the birkenfeld branch from 6da4216 to 96c1c33 Compare December 2, 2022 23:04

shymega mentioned this pull request Feb 2, 2023

Add impl of ReadBytesExt and WriteBytesExt for core2::io::Write BurntSushi/byteorder#183

Closed

Refactor to run on no_std and std, based on target

17415cc

Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk>

shymega force-pushed the birkenfeld branch from c139cb7 to 17415cc Compare August 5, 2023 18:33

Remove usage of core/alloc

e88f7d1

As per @birkenfeld's review of this branch, proc-macros can use `std` during compile-time - there's no issues with `no-std`.

shymega added 5 commits August 5, 2023 19:38

Remove unneeded extern crate

78bc21c

For `proc_macro`, as per @birkenfeld's PR review.

Export/use relevant byteorder types, dependent on std/no_std

8e541c9

Remove unnecessary imports in tests

eb94f2d

Remove unnecessary unused_imports lint in macro

865508c

macro: Ignore size_of return value, not used

7d570e2

shymega added 2 commits August 6, 2023 22:53

Add slightly modified ByteOrderExt trait impls

44f42d4

This adds a slightly modified module from `byteorder`, which should run on `no_std`. The `src/lib.rs` has been modified in light of this change.

Remove std feature flag guard in macro impl

a5a207f

Add no_std support to structure #12

Are you sure you want to change the base?

Add no_std support to structure #12

Uh oh!

Conversation

shymega commented May 5, 2022

Uh oh!

liranringel commented May 8, 2022

Uh oh!

shymega commented May 8, 2022

Uh oh!

liranringel commented May 8, 2022

Uh oh!

shymega commented May 12, 2022

Uh oh!

shymega commented Aug 17, 2022

Uh oh!

liranringel commented Aug 20, 2022

Uh oh!

shymega commented Aug 23, 2022

Uh oh!

liranringel commented Sep 11, 2022

Uh oh!

shymega commented Sep 14, 2022

Uh oh!

shymega commented Nov 18, 2022

Uh oh!

liranringel commented Nov 20, 2022

Uh oh!

shymega commented Nov 26, 2022

Uh oh!

birkenfeld commented Nov 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

birkenfeld Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

shymega Aug 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shymega commented Dec 2, 2022

Uh oh!

birkenfeld commented Dec 3, 2022

Uh oh!

shymega commented Jan 22, 2023

Uh oh!

shymega commented Feb 2, 2023

Uh oh!

shymega commented Feb 2, 2023

Uh oh!

shymega commented Feb 26, 2023

Uh oh!

shymega commented Aug 5, 2023

Uh oh!

liranringel commented Nov 21, 2023

Uh oh!

shymega commented Dec 10, 2023

Uh oh!

shymega commented Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `no_std` support to structure #12

Add `no_std` support to structure #12