Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discussion] no_std support #12

Closed
SimonIT opened this issue Feb 2, 2023 · 4 comments · Fixed by #13
Closed

[Discussion] no_std support #12

SimonIT opened this issue Feb 2, 2023 · 4 comments · Fixed by #13

Comments

@SimonIT
Copy link
Contributor

SimonIT commented Feb 2, 2023

Hey @NicolasDP,
@connectety and I want to use cddl-codegen with constrained devices.
For this, we need no_std compatibility.
We started making the changes in our fork.

It seems that everything is compatible with no_std except BufRead.
For BufRead exists a drop-in replacement called acid_io.

So I want to get some opinions from you maintainers about this.
Especially because of your statement:

this crate has zero dependencies (and should not need any in the future)

@SimonIT SimonIT mentioned this issue Feb 9, 2023
@NicolasDP
Copy link
Contributor

hello @SimonIT , thanks for the effort in adding no_std support to the crate. I believe you are adding this so that you can use the output of the cddl_codegen to add cardano support on hardware wallets.

The acid_io crate is interesting though I am not sure this is the approach I would have looked at first. I think we should consider a rework of the core component to add the right type of support. I'd like to offer another approach instead:

please have a look at the following: #14

This way the array and bytes data are completely left to the user of the crates and we don't need the BufRead dependency. What do you think?

@SimonIT
Copy link
Contributor Author

SimonIT commented Feb 21, 2023

Good idea.

Another problem I came across is the error handling:

  1. The trait Error in core is not stable Tracking Issue for Error in core rust-lang/rust#103765
  2. Because the trait is not sized, something like Box has to be used
    IoError(Box<dyn error::Error>),

    This is kinda ugly in my opinion.
  3. There seem to be a problem with thread safety and errors?
  error[E0277]: `(dyn core::error::Error + 'static)` cannot be sent between threads safely
   --> src/de.rs:23:17
    |
    23 |             Err(Box::new(Error::ExpectedU8))
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn core::error::Error + 'static)` cannot be sent between threads safely
    |
    = help: the trait `Send` is not implemented for `(dyn core::error::Error + 'static)`
    = note: required for `Unique<(dyn core::error::Error + 'static)>` to implement `Send`
    = note: required because it appears within the type `Box<dyn Error>`
   note: required because it appears within the type `Error`
   --> src/error.rs:11:10
    |
   11 | pub enum Error {
    |          ^^^^^
    = note: required for the cast from `error::Error` to the object type `dyn core::error::Error + Send + Sync`

@NicolasDP
Copy link
Contributor

if we go the path I'm suggesting we don't need IO anymore. We will not need to do things like IoError(Box<dyn error::Error>).

@SimonIT
Copy link
Contributor Author

SimonIT commented Mar 8, 2023

We think everything is resolved. Our PR is working. Feel free to make some suggestions there #13 .

@SimonIT SimonIT closed this as completed Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants