esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
667 stars 189 forks source link

Feedback on examples #1432

Open Firstyear opened 4 months ago

Firstyear commented 4 months ago

I think that while it's great there are so many examples, the issue is that because they all depend on one large Cargo.toml, it's very difficult to see which parts of the cargo.toml are actually needed to build any feature or part of the example in isolation. It would be better if the examples were broken out in some way so that it was clear which features they require, and which dependencies they rely on.

MabezDev commented 4 months ago

Regarding dependencies, this is sadly a general Rust problem which every project with examples will run into. We are pushing people towards using https://github.com/esp-rs/esp-template and picking parts from examples across for the time being. We want to improve the template further, but there are currently some limitations with cargo-generate: https://github.com/cargo-generate/cargo-generate/issues/1054.

As for features, each example has its features and supported chip listed at the top level docs. We use this metadata in our xtask subcommand to auto enable the features (instead of having to specify them yourself if using cargo directly).

Thanks for the feedback! In this case I don't think we have anything we can action at this time, but if you some concrete ideas on how to improve our current situation please file another issue :).

Firstyear commented 4 months ago

The problem I think is that going from an example to something standalone is really hard.

Lets take embassy_dhcp as an example. You can't just lift that file up and out to a new project. What do I need in Cargo.toml? There is a maze of features to unpick to work out what's needed because all the examples proxy via a single Cargo.toml. And when you do have all of them, you still get spurious other errors.

I think my "frustration" is that it's hard to isolate what each example actually depends on and requires. it's hard to pick it apart into what you really need.

That's why I think there needs to be a way to split this up, because while it may be convenient for tests, it's not serving as a useful example if I can't take it and action what it contains. An example is after all meant to educate on basic operation of a task and how to create and configure the ecosystem so that example can run. Especially when embassy is so "feature complex".

Firstyear commented 4 months ago

Additionally it would be great if the cargo-template included an embassy variant too :)

Firstyear commented 4 months ago

How about this as an idea - there needs to be a "glue" crate.

Currently, a cargo.toml to build something looks like this:

 # cat Cargo.toml
[package]
name = "no-std-template"
version = "0.1.0"
authors = ["William Brown <william@blackhats.net.au>"]
edition = "2021"
license = "MIT OR Apache-2.0"

[profile.dev]
# Rust debug is too slow.
# For debug builds always builds with some optimization
opt-level = "s"

[profile.release]
codegen-units = 1 # LLVM can perform better optimizations using a single thread
debug = 2
debug-assertions = false
incremental = false
lto = 'fat'
opt-level = 's'
overflow-checks = false

[dependencies]
defmt = { version = "0.3.6", optional = true }
esp-hal = { version = "0.16.1", default-features = false }
smoltcp = { version = "0.11.0", default-features = false, features = [
    "medium-ethernet",
    "socket-raw",
], optional = true }
critical-section = "1.1.1"
log = { version = "0.4.20", optional = true }
embedded-hal = { version = "0.2.4", default-features = false }
embedded-svc = { version = "0.27.0", default-features = false, features = [], optional = true }
enumset = { version = "1.1.3", default-features = false, optional = true }
linked_list_allocator = { version = "0.10.5", default-features = false, features = [
    "const_mut_refs",
] }
embedded-io = { version = "0.6.1", default-features = false }
embedded-io-async = { version = "0.6.0", optional = true }
fugit = "0.3.7"
heapless = { version = "0.8", default-features = false, features = [
    "portable-atomic",
] }
num-derive = { version = "0.4" }
num-traits = { version = "0.2", default-features = false }

no-std-net = { version = "0.6.0", optional = true }

esp-wifi = { version = "0.4.0" }
esp-wifi-sys = { version = "0.3.0" }

embassy-sync = { version = "0.5.0", optional = true }
embassy-futures = { version = "0.1.0", optional = true }
embassy-net-driver = { version = "0.2", optional = true }
toml-cfg = "0.2.0"
libm = "0.2.7"
cfg-if = "1.0.0"
portable-atomic = { version = "1.5", default-features = false }
portable_atomic_enum = { version = "0.3.0", features = ["portable-atomic"] }

futures-util = { version = "0.3.28", default-features = false, features = [
    "portable-atomic",
] }
atomic-waker = { version = "1.1.2", default-features = false, features = [
    "portable-atomic",
] }

# Former dev deps

esp-println = { version = "0.9.0", default-features = false, features = [
    "log",
    "uart"
] }
esp-backtrace = { version = "0.11.0" , features = [
    "panic-handler",
    "exception-handler",
    "println",
] }
embassy-executor = { version = "0.5.0", package = "embassy-executor", features = [
    "nightly",
    "integrated-timers",
] }
embassy-time = { version = "0.3.0" }
embassy-net = { version = "0.4.0", features = [
    "tcp",
    "udp",
    "dhcpv4",
    "medium-ethernet",
] }
bleps = { git = "https://github.com/bjoernQ/bleps", package = "bleps", rev = "9371d7d4d510ba5c936c1eef96674f8fd4f63e8a", features = [
    "macros", "async"
] }
embedded-hal-async = { version = "1.0.0" }
static_cell = { version = "2.0", features = ["nightly"] }

[build-dependencies]
toml-cfg = "0.2.0"

[features]
default = [ "log" ]

# chip features
esp32c2 = [ "esp-hal/esp32c2", "esp-wifi-sys/esp32c2", "esp-println/esp32c2", "esp-backtrace/esp32c2" ]
esp32c3 = [ "esp-hal/esp32c3", "esp-wifi-sys/esp32c3", "esp-println/esp32c3", "esp-backtrace/esp32c3" ]
esp32c6 = [ "esp-hal/esp32c6", "esp-wifi-sys/esp32c6", "esp-println/esp32c6", "esp-backtrace/esp32c6" ]
esp32h2 = [ "esp-hal/esp32h2", "esp-wifi-sys/esp32h2", "esp-println/esp32h2", "esp-backtrace/esp32h2" ]
esp32   = [ "esp-hal/esp32",   "esp-wifi-sys/esp32",   "esp-println/esp32",   "esp-backtrace/esp32" ]
esp32s2 = [ "esp-hal/esp32s2", "esp-wifi-sys/esp32s2", "esp-println/esp32s2", "esp-backtrace/esp32s2" ]
esp32s3 = [ "esp-hal/esp32s3", "esp-wifi-sys/esp32s3", "esp-println/esp32s3", "esp-backtrace/esp32s3" ]

# async features
async = [
  "dep:embassy-sync",
  "dep:embassy-futures",
  "dep:embedded-io-async",
  "esp-hal/embassy",
  "esp-hal/async",
]

embassy-net = ["dep:embassy-net-driver", "async"]

# misc features
coex = []
wifi-logs = []
dump-packets = []
smoltcp = [ "dep:smoltcp" ]
utils = [ "smoltcp" ]
enumset = []
wifi = [ "dep:enumset", "dep:no-std-net" ]
embedded-svc = [ "dep:embedded-svc" ]
ble = [ "esp-hal/bluetooth" ]
phy-enable-usb = []
ps-min-modem = []
ps-max-modem = []
esp-now = [ "wifi" ]
ipv6   = ["wifi", "utils", "smoltcp?/proto-ipv6"]
ipv4   = ["wifi", "utils", "smoltcp?/proto-ipv4"]
tcp    = ["ipv4", "smoltcp?/socket-tcp"]
udp    = ["ipv4", "smoltcp?/socket-udp"]
icmp   = ["ipv4", "smoltcp?/socket-icmp"]
igmp   = ["ipv4", "smoltcp?/proto-igmp"]
dns    = ["udp",  "smoltcp?/proto-dns", "smoltcp?/socket-dns"]
dhcpv4 = ["wifi", "utils", "smoltcp?/proto-dhcpv4", "smoltcp?/socket-dhcpv4"]
wifi-default = ["ipv4", "tcp", "udp", "icmp", "igmp", "dns", "dhcpv4"]
defmt = [
  "dep:defmt",
  "smoltcp?/defmt",
  "esp-hal/defmt",
]
log = [
  "dep:log",
  "esp-hal/log",
]

[package.metadata.docs.rs]
features = ["esp32c3", "wifi", "ble", "coex", "async", "embassy-net", "esp-hal/embassy-time-systick", "esp-hal/default"]
default-target = "riscv32imc-unknown-none-elf"

And I had to copy paste huge parts of this from esp-wifi and it's multiple Cargo.toml files, and it still doesn't work.

The issues are:

Going from cargo-generate to a working build has been impossible for 4 days now. I simply haven't got anything non trivial to work, because I can't work out what's needed in the Cargo.toml.

This is why I think a "glue" crate would be useful.

This could be something like 'esp-glue'. It could have features that describe high level concepts like:

This way a consumer's cargo.toml could be:

[dependencies]
esp-glue = { version = 0.1.0, features = [
    "esp32c3",
    "wifi",
    "ble",
    "embassy"
] }

Examples could then be part of the glue crate since it's much easier to include.

This would allow a tokio style library https://docs.rs/tokio/latest/tokio/#feature-flags where you enable what you are using, or could just opt into "full" if you wanted.

It would also allow board selection to be much easier.

Within the glue crate it could exposed tested and coordinated sets of libraries that are known to work together, via things like esp-glue::{wifi, net, embassy} etc. That way a user never has to worry about things like "which version of embassy and embassy net worktogethr with esp-hal, and what features do I need? "

I think this would greatly improve the new user experience as well as existing users.

MabezDev commented 4 months ago

Thank you for the feedback, it's really valuable!

Something like esp-glue is on our roadmap, but we have some other things, mainly in esp-hal, that we need to work on first.

I think my "frustration" is that it's hard to isolate what each example actually depends on and requires. it's hard to pick it apart into what you really need.

I agree, I've run into this with other projects too. My strategy is to copy all the deps, and get it building, then cut away deps one by one. This is obviously not ideal though. Having something like esp-glue and a more advanced esp-template would be great.

I think as an actionable item here, we can also try to document our deps and what they're needed for in examples/Cargo.toml, as a stop gap solution. So I'll reopen to track this.

Firstyear commented 4 months ago

It's pretty likely something like the "glue" crate could also just make it easier in general to upgrade versions too since it defines a working combination of parts.

So I'd be more than happy to test that when you start to work on it (heck, I was tempted to throw together a basic one and try to expand it a bit if I got more parts working).

Firstyear commented 4 months ago

While this doesn't seem like a bad idea on the surface, I feel this would be a nightmare for us to maintain with the rate that breaking changes are introduced to the embedded ecosystem.

I think a comment was added and then removed, but to address this point because I think it's really important.

We have to look at this as "costs". By having a glue crate, you the maintainers of the project are in control of saying "what parts work together, and what doesn't". Yes, that does take work. Yes it does come with challenges when breaking changes are made to re-asses "what parts do work together?".

But today you already have that cost. You need to pay this cost to keep every example working under all these moving parts. So this "glue" suggestion actually helps you, because instead of having to upgrade every crate and example, you can fix the part moving puzzle in a single location rather than many locations.

In addition, think about how this feels as a consumer of the ecosystem - without the maintainers putting up some of this work (work that you already have to do because of the examples as noted), your consumers now also need to pay this cost. Not only this, they need to do so without the same level of subject matter expertise as you have. There are many smart users of your ecosystem, but they may not have all the insight you have to enable them to resolve which changes work together.

For example , every time I go to update an esp-rs project, I can't just "upgrade" it. I have to create a new project from a new version of your cargo template, and then lift and move bits at a time. Because when I do try and upgrade, everything breaks in subtle and unexpected ways that I simply don't have the knowledge to solve.

So I think while yes, a glue crate is work for you as maintainers, it's work you already have to perform in your projects, and it hugely benefits your users. The more users you have, and the more you empower and enable them to use your libraries, the more they will be able to step up and help and contribute in other ways.

danclive commented 4 months ago

I've thought about this, and I've just created a repository, esp-examples, to try and add some standalone examples to illustrate the dependency options. I'll add them as soon as I have time.

Firstyear commented 4 months ago

I'll check it out and try to help out if I can make some more. Thank you so much <3

MabezDev commented 2 months ago

We've recently merged many dependencies into the esp-hal tree directly. This doesn't solve all the issues you've mentioned, and a glue crate is still something we're thinking about for the future, but we hope it will improve the situation a little bit for now.

Firstyear commented 2 months ago

Thank you! I did notice this actually, so I'll have to try it later.