Add #![no_std] support
#64
Conversation
# Conflicts: # Cargo.toml # src/convert.rs
There was a problem hiding this comment.
Can we explore making the crate noalloc? I think it makes for a much bigger case for supporting nostd and was what we were hoping to do. I think because of the functionality in the crate, that the size of what is being enc/dec is always predictable, this should be possible. Although I don't know how well the data-encoding crate is at supporting that. Wdyt?
|
Hi @leighmcculloch, I have initially added support for no alloc, the current implementation is quite rough, and I will improve it tomorrow. It would be great if you could review the interface, but please do not focus on the details of the implementation at this time. |
Remove the thiserror crate and replace with the same code that would be generated. This change was authored by @overcat in #64, and has been extracted to commit separately. Co-authored-by: Jun Luo <[email protected]>
There was a problem hiding this comment.
@overcat Thanks for taking a look at what it would take to make this go no-alloc and for the time you put into it ❤️.
In interest of getting the no-std functionality you need merged asap you should revert the last few no-alloc commits, because looking back at it the no-std change is on its own much more surgical and easier to review, and makes no breaking changes to the API.
The no-alloc change as proposed requires breaking changes to the API that I'd like some more time to think about, but I don't want to delay getting a no-std version out.
Thanks for persevering with me on this, I didn't realize scope difference of the no-alloc vs no-std approach.
Remove the thiserror crate and replace with the same code that would be generated. This change was authored by @overcat in #64, and has been extracted to commit separately. Co-authored-by: Jun Luo <[email protected]>
|
@overcat Can you merge |
# Conflicts: # Cargo.toml # src/error.rs
|
CI passed. |
tests/tests.rs
Outdated
| @@ -1,8 +1,10 @@ | |||
| use stellar_strkey::*; | |||
| #![no_std] | |||
There was a problem hiding this comment.
Tests always run with std imported anyway, because the test framework itself uses std, so I don't think we need the changes in this file.
Is there a reason using alloc explicitly here benefits?
There was a problem hiding this comment.
Yes, you are right, I have removed it.
But I am curious if we should add a test to see if it works properly under no std? It seems that there are some testing frameworks that can achieve this: https://github.com/knurling-rs/defmt
There was a problem hiding this comment.
We could add a crate to the repo that imports strkey and gets built as part of testing, that provides it's own panic handler, because the only way it'll build is if std isn't imported because there can't be two panic handler and std provides one.
There was a problem hiding this comment.
On the same note that's a lot of test machinery for checking something that is unlikely to be the case since the no_std attribute is in use.
The risk is that it depends on a crate that pulls in std, but we're unlikely to add new deps to this repo.
I wonder if something like https://github.com/cackle-rs/cackle could detect it.
There was a problem hiding this comment.
Hi @leighmcculloch, I have integrated version 0.0.11 into Ledger, and it's working very well. Currently, I want to make rs-stellar-xdr support no std, so I will work on the tests here and consider adding no alloc support in my free time.
There was a problem hiding this comment.
There's nothing riding on no alloc, so if it's not of value to you, I suggest holding off as there's nothing that I know of needing it right now.
What
add
#![no_std]support.Why
Allow it to work on embedded devices (such as STM32).
Known limitations
N/A