Improve code generation.

[reitermarkus]

Generate Rust enums and implement correct default value for wifi_init_config_t.

[ivmarkov]

The changes to esp-idf-svc look quite OK, thank you. Accidentally however, I found this this morning. Sorry, I know this is late and we should have discussed it before I asked you to make the changes in esp-idf-svc, but I think better late than never. So given that ESP-IDF is a shared library, shall we be switching to Rust enums in the first place? It seems Bindgen folks had a good reason to choose a different default?

@MabezDev @N3xed What do you think?

[ivmarkov]

The changes to esp-idf-svc look quite OK, thank you. Accidentally however, I found this this morning. Sorry, I know this is late and we should have discussed it before I asked you to make the changes in esp-idf-svc, but I think better late than never. So given that ESP-IDF is a shared library, shall we be switching to Rust enums in the first place? It seems Bindgen folks had a good reason to choose a different default?

@MabezDev @N3xed What do you think?

@reitermarkus ^^^

[reitermarkus]

@ivmarkov, I changed the enums to use newtypes instead of Rust enums. This doesn't have the problem with UB. This of course get's rid of the benefit of having exhaustiveness checked by the compiler, but type-safety is still improved a bit, and we don't have to deal with ugly constant names and non_upper_case_globals all over the place.

[ivmarkov]

OK so here's my take on this (to be discussed on the upcoming esp-rs meeting this Tuesday): I think we are chasing diminishing results.

Constified "enums"

• These are not true Rust enums, just constants pushed in their own Rust modules, if I understand it correctly.
• They can remove one Clippy warning (the one about casing), but the other one would stay.
• From usage point of view, not that much of a difference.

If we switch to these what do we win?

• slightly better syntax (i.e. instead of wifi_type_t_FOO we get wifi_type_t::FOO

What do we lose?

• We'll break everybody who is currently using raw esp-idf-sys. Granted, we are not at V1, yet, breaking everybody should not be a light decision.
• What worries me even more (after the "retreat" from "real" Rust enums) is that these are again not the default Bindgen generates. Why? If they are really better than straight constants? I would like to know the answer here.

Default::default for this and that

Over time what I have tried is to slim down esp-idf-sys to the bare minimum. esp-idf-sys is supposed to be a raw set of bindings to ESP-IDF, with little added on top. Hence why stuff like mutex is no longer here. In fact, the only "sugar" which is still around (aside from the no_std stuff like the alloc and panic handlers that nobody uses and is indeed controversial and the start feature which simply doesn't have a better place to live) is the EspError error implementation.

Now with these defaults we are going in the opposite direction, which I don't think is well justified. It makes everything asymmetric, in that we have now a handful (how many?) custom Default::defaults, and every other default auto-generated by esp-idf-sys will just do C-style zeroing, which I'm sure is often not a default users can use "just like that". How do we even explain that semantic difference to users? Moreover, this is likely temporary, until we have actual C-level bindings for the same, and once we have those this sugar becomes irrelevant and either we should remove it (breaking changes again) or we should rewrite it to delegate to the C level API (do we really want these to call into ESP-IDF though?).

@MabezDev @N3xed @reitermarkus feel free to comment.

[reitermarkus]

Constified "enums"

Constified enums are what is currently in this crate. You are talking about constified enum modules. This PR switches to newtype enums, which uses associated constants on the type itself.

As to why the newtype style is not the default: I guess simply because it was added after the current default.

If we switch to these what do we win?

Yes, as you said better syntax. But also type safety since you cannot pass just any integer as before, you have to pass a newtype containing that integer.

Default::default for this and that

Regarding Default I think the best bet would be to not implement the zeroing Default for any types since it's not guaranteed to be a correct default value for most types. That only works if the type isn't just an alias to an integer type. And then use MaybeUninit when getting such a type from a C API.

[ivmarkov]

Default::default for this and that

Regarding Default I think the best bet would be to not implement the zeroing Default for any types since it's not guaranteed to be a correct default value for most types. That only works if the type isn't just an alias to an integer type. And then use MaybeUninit when getting such a type from a C API.

But then again: it is not us who did this? It is Bindgen (since recently I think) - and by default. Which brings again the topic: if this is the default, perhaps they had good reason to make it the default (as in its usefulness is bigger than its caveats). And MaybeUninit is of course not the same at all - and more dangerous IMHO, as even if Default::default per se might not be good enough, initializing some fields and leaving the rest to 0 (with ..Default::default()) often works fine.

In general, I trust Bindgen defaults better than my own judgement, as they have seen much more C bindings than us.

[N3xed]

I don't think having Newtype enums is a good idea.

(For example, the C enum

enum ip_event_t {
    IP_EVENT_STA_GOT_IP,
    IP_EVENT_STA_LOST_IP,
   ...
};

representation in rust would turn from

pub type ip_event_t = c_types::c_uint;
pub const ip_event_t_IP_EVENT_STA_GOT_IP: ip_event_t = 0;
pub const ip_event_t_IP_EVENT_STA_LOST_IP: ip_event_t = 1;

into

#[repr(transparent)]
#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
pub struct ip_event_t(pub c_types::c_uint);

impl ip_event_t {
    pub const IP_EVENT_STA_GOT_IP: ip_event_t = ip_event_t(0);
    pub const IP_EVENT_STA_LOST_IP: ip_event_t = ip_event_t(1);
}

).

Now I think it's really tempting to have the newtype representation. It's more structured, each enum has its own type (not just a type alias) with associated constants for the variants, which is similar to normal C or Rust enums.

But I don't think we gain enough from this syntax to make it worth it, especially because this is a low level bindings crate. The type safety is only a little bit better, as one can trivially construct such an enum value without using the associated constants and thus can pass any value. And I'd argue that the syntax for using such a variant is not really better than the old way as you still have to spell the whole name out to use it, the only difference being the :: instead of _. Better would be if these variants can be imported and used without the type prefix, or we could event get rid of the type prefix and just have the variant names by itself (e.g. for ip_event_t having all variants in its own module, or having all variants in the crate module: IP_EVENT_STA_GOT_IP instead of the current ip_event_t_IP_EVENT_STA_GOT_IP), which would be the same as in C.

The biggest downside though is that manipulating these constants at compile-time (in a const context) becomes a pain, which we do pretty often in embedded code, this is especially true for the bitfield-ified enums. And also since this is a low-level bindings crate having separate concrete or wrapper types for the enums may not be desirable as implementing higher-level type-safe and zero-cost abstractions become more difficult, in my opinion.

[MabezDev]

I don't really have any strong opinions about this. My gut is saying to have faith with the defaults of bindgen, but some added type safey would be nice, but to be honest not a huge deal. All these bindings are unsafe and should be used and reviewed with care.