structure
structure copied to clipboard
liranringel
Add `no_std` support to structure
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 - https://github.com/Cosmo-CoDiOS/structure/pull/2 - a mistake on my part.
Hopefully, this can now be reviewed.
Great! I’m quite busy these days, I hope I’ll find time this week to review this PR.
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.
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.
Hey @liranringel, sorry to prod again, but I'm stuck with the PR so far, and need some feedback on its current state. Thanks!
@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?
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?
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.
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.
Thank you @shymega! While I'm a bit busy these days, I'll try to review your changes as soon as I can.
@liranringel Thanks. Shall we see if @birkenfeld has any thoughts on it too?
OK, so after running tests with no_std enabled, looks like there's a lot more to fix. Vec breakages and such.
Yeah, byteorder's WriteBytesExt and ReadBytesExt are not defined on no_std (in hindsight, makes sense, since they use std::io too).
@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.
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.
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...
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.
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.
@shymega sorry for not being here for a long time. What is the PR status? Is it ready for review?
@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 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 :-)