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

Support for the ESP-IDF framework #128

Merged
merged 4 commits into from
Aug 5, 2023
Merged

Conversation

ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented Aug 3, 2023

(This has been long supported in a fork of the polling crate, but the fork was against an ancient version. So I finally jumped the gun to rebase it against the latest polling and try to upstream now.)

ESP IDF already does support the poll syscall, so this PR is only necessary because - ESP IDF being an embedded OS (or toolkit?) - does not support anything process-related, including the pipe syscall.

However it does support a subset of the eventfd syscall API, so what this PR does is to augment the poll module - specifically for ESP IDF - with an alternative eventfd codepath to where pipe is usually utilized. The same idea (use eventfd for the ESP IDF) was I believe re-used by @jasta in the recently upstreamed support for ESP IDF in tokio's reactor.

In terms of changes, two main items:

  • As per above, the poll module is augmented with alternative code-path based on eventfd
  • I had to also update the rustix dependency from 0.37.X to 0.38.X as the support for ESP IDF in rustix (including the eventfd syscall) lands in the 0.38.X version. I hope you don't mind and I noticed other crates in the smol ecosystem already use rustix 0.38.X so I guess it was just a matter of time and somebody taking the effort to update it for this crate as well
    • The changes for the new rustix version are really mainly addressing latest rustix shuffling code around (the new event module). Two changes worth noting are this one and this one where I believe in the latter, the cast to time_t was incorrect in the first place.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I don't like the way that the alternating pipe/eventfd code is scattered throughout the module. It would be better if there was a single Notifier type that contained either the pipe on non-ESP and the eventfd on ESP. I did a similar thing in kqueue.rs to switch between pipes and user events.

Looks good otherwise.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Aug 3, 2023

OK. Will schedule some time over the weekend to do that.

@daggerrz
Copy link

daggerrz commented Aug 4, 2023

@ivmarkov, this is a related: #129

I rant into this while using your branch for embedded development and building tests for my MacOS host. The Rustix dependency update breaks some targets.

@notgull
Copy link
Member

notgull commented Aug 4, 2023

I merged #123 because I don't want to build up changes that are going to get stale. Just a heads up. This also takes care of the rustix v0.38 port.

@daggerrz
Copy link

daggerrz commented Aug 4, 2023

Ah, thanks, did not see that it was already pending!

@ivmarkov ivmarkov marked this pull request as draft August 4, 2023 04:57
@ivmarkov ivmarkov force-pushed the espidf branch 2 times, most recently from 73f43c9 to 4d26dba Compare August 4, 2023 04:58
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Aug 4, 2023

@notgull Two additional follow up questions:

  • Looking at your unix domain socket Notify implementation in the kqueue module, it is pretty much identical in behavior to the pipe-based notification mechanism in the poll module, except based on unix domain sockets. Shall I re-base the one in poll that would be #[cfg(not(target_os = "espidf"))] to be also based on unix domain sockets (not sure these are available everywhere else?) or shall I keep it based on pipes, as it is now?
  • If we go for domain sockets, do you want the domain socket impl somehow centralized and outside of kqueue and poll (not sure that's easily possible but I can try), or do you want two separate implementations - one in kqueue and one in poll?

I'm in favor of a minimal variant where poll still has its own, private implementation based on pipe (and eventfd for the ESP IDF) as this stuff is difficult to test with so many variants of polling mechanisms and operating systems, but also would like to understand where you want to drive this in the end, so that I don't have to redo the PR multiple times.

@notgull
Copy link
Member

notgull commented Aug 4, 2023

  • Unfortunately UnixStream does not work on Fuchsia. As Fuchsia isn't technically Unix, libstd does not expose it. So pipes are necessary for this use case.
  • I would prefer to keep the Notifier type in this module. The goal is to have each module be as self contained as possible. Not to mention, the code can't really be shared between Unix and Windows.

@ivmarkov ivmarkov marked this pull request as ready for review August 4, 2023 21:45
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Aug 4, 2023

  • Unfortunately UnixStream does not work on Fuchsia. As Fuchsia isn't technically Unix, libstd does not expose it. So pipes are necessary for this use case.
  • I would prefer to keep the Notifier type in this module. The goal is to have each module be as self contained as possible. Not to mention, the code can't really be shared between Unix and Windows.

OK it should be ready for another review.

  • I have run the unit tests for both the pipe based approach as well as the eventfd based one on Linux as it supports both approaches.
  • The one weird thing I found is that Linux really wants the eventfd handle to be registered with PollFlags::IN - as documented actually, while ESP IDF doesn't care.

@ivmarkov ivmarkov requested a review from notgull August 4, 2023 21:50
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me. Can you add a test to CI to make sure this code builds when targeting ESP-IDF? Even a simple cargo check would be enough.

Ignore the Wine and Cross failures in CI, they aren't your fault.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Aug 5, 2023

Overall this looks good to me. Can you add a test to CI to make sure this code builds when targeting ESP-IDF? Even a simple cargo check would be enough.

Ignore the Wine and Cross failures in CI, they aren't your fault.

cargo check should be doable, yes, but with the following two caveats:

  • Caveat 1: It will be a rust nightly-only exercise. The problem is, ESP IDF - being a Tier 3 target - does not get a pre-built STD by default. So we do need a command line like rustup component add rust-src --toolchain nightly; cargo +nightly check --target riscv32imc-esp-espidf -Zbuild-std=std,panic_abort, where the -Zbuild-std part is unfortunately a nightly-only feature of Cargo. (UPDATE: rustix already does that for the ESP IDF and a bunch of others, so yeah, why not?)
  • Caveat 2: The eventfd binding in rustix for ESP IDF specifically, just got merged the other day, and thus missed the 0.38.6 release, so it is only in rustix's mainline. Let me actually open a PR against their repo for issuing another rustix point release. (UPDATE: rustix release PR here).

So if you are OK with "Caveat 1", yes, I'll work on that.

@ivmarkov ivmarkov marked this pull request as ready for review August 5, 2023 06:45
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Aug 5, 2023

OK, CI is ready. For now (and until rustix 0.38.7 is released), cargo check for ESP IDF runs with the rustix dependency patched against its mainline (and there is a weird warning that the patched dependency is not used, but surely it is or else the build would've failed).

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

NetBSD failure looks spurious to me, not sure why that's happening.

@notgull notgull merged commit 5379338 into smol-rs:master Aug 5, 2023
@notgull notgull mentioned this pull request Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants