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

Enable support for espidf (esp32 family of microcontrollers) #245

Merged
merged 4 commits into from
Feb 5, 2022

Conversation

thorhs
Copy link
Contributor

@thorhs thorhs commented Jan 29, 2022

Shamelessly forked from https://github.com/esp-rs-compat/getrandom.

Has been tested on my local MCU.

@newpavlov
Copy link
Member

newpavlov commented Jan 29, 2022

Shouldn't we prefer esp_fill_random? IIUC getrandom is a compatibility wrapper around it. We even may prefer esp_random since it can allow Rust compiler to optimize filling loop in some cases.

Also, would it be possible to add a build-only test for an ESP-IDF target to our CI config?

@thorhs
Copy link
Contributor Author

thorhs commented Jan 30, 2022

That was my initial implementation, but then I found the esp-rs-compat repo and figured it was a better solution. Using esp_fill_random requires including a dependency on esp-idf-sys, which includes the burden on updating that dependency as time goes by.

I have a version standing by using esp_fill_random, if that is preferred.

As for the build test, I think this requires a patched rust compiler from espressif, so I'm not sure how to write that in a portable manner.

src/lib.rs Outdated Show resolved Hide resolved
@josephlr
Copy link
Member

@thorhs if you resync onto master you should be able to fix the CI issues.

That was my initial implementation, but then I found the esp-rs-compat repo and figured it was a better solution. Using esp_fill_random requires including a dependency on esp-idf-sys, which includes the burden on updating that dependency as time goes by.

This is a good point (about avoiding dependencies), which is why we generally just manually include the extern "C" declarations for various platforms. For example, our implementations for Windows, iOS, or SOLID do this. So we would probably just have something like

extern "C" {
  fn esp_random() -> u32;
}

In general, we try to use the lowest level RNG API available on a given platform (provided that it is safe and stable). So depending directly on the esp_random functions is fine.

We even may prefer esp_random since it can allow Rust compiler to optimize filling loop in some cases.

@newpavlov's point is a good one, although using esp_random avoids being able to inline the esp_random implementation into esp_fill_random, so it might not really matter. This stuff also isn't fast to begin with.

However, it seems like esp_fill_random wasn't added until v3.2 of esp-idf, so if supporting older versions matters, we should use esp_random. If not, I think we should use esp_fill_random for its simple implementation.

As for the build test, I think this requires a patched rust compiler from espressif, so I'm not sure how to write that in a portable manner.

If support for espidf hasn't been merged into upstream Rust yet, that's fine. Just add a TODO to the build-std section of tests.yml. However, it seems like rust-lang/rust#87666 added the necessary support to rust, what else is left to do upstream?

ivmarkov and others added 2 commits January 30, 2022 14:34
Import esp_fill_random by using extern "C" and skip depending of
esp-idf-sys.

Opted for esp_fill_random instead of esp_random since esp_fill_random
implements logic for filling the u8 array efficiently from a u32 source.
Figured it was more efficient than what I would implement.
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using esp_random avoids being able to inline the esp_random implementation into esp_fill_random, so it might not really matter

The most common use-case for getrandom is seeding of user-space PRNGs, which often use u32 or u64 as seeds. Using esp_random would allow generated data to stay in registers without round-tripping through stack. But I agree that it's a minuscule difference, so I am fine with esp_fill_random.

src/espidf.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@thorhs
Copy link
Contributor Author

thorhs commented Jan 31, 2022

Could this also be merged into 0.1 branch? Should I submit a new PR for that?

@newpavlov
Copy link
Member

If you really need it in practice, then we could do it. But otherwise I don't think it's worth the trouble, since it would be better for your dependencies to use up-to-date rand crates.

@newpavlov newpavlov merged commit cf02327 into rust-random:master Feb 5, 2022
henrysun007 pushed a commit to mesatee/getrandom that referenced this pull request Nov 24, 2022
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 this pull request may close these issues.

4 participants