Improve code generation. #76

reitermarkus · 2022-03-10T10:30:46Z

Generate Rust enums and implement correct default value for wifi_init_config_t.

ivmarkov

Two questions on the justification of those changes and not the changes themselves. :)

And one on the topic of if I merge this PR, what happens to esp-idf-hal and esp-idf-svc?

ivmarkov · 2022-03-20T09:31:16Z

build/build.rs

@@ -5,7 +5,10 @@ use std::env;
 use std::iter::once;
 use std::path::PathBuf;

-use ::bindgen::callbacks::{IntKind, ParseCallbacks};
+use ::bindgen::{


How would that affect esp-idf-hal and esp-idf-svc? Would these continue to compile just fine after these changes, or do we need a PR for each of those to fix any compilation errors?

Also: why do we want Rust enums?

How would that affect esp-idf-hal and esp-idf-svc?

I guess there will need to be some changes to those crates. I can have a look at that of course.

Also: why do we want Rust enums?

They are more ergonomic to use and the compiler will check if all variants are handled.

How would that affect esp-idf-hal and esp-idf-svc?

I guess there will need to be some changes to those crates. I can have a look at that of course.

Would appreciate that. :)

Also: why do we want Rust enums?

They are more ergonomic to use and the compiler will check if all variants are handled.

I think I tried this once, but then for some reason abandoned it. Perhaps once you try to adjust esp-idf-svc and esp-idf-hal to work with the new enum-based constants, the problem (if there was a real one) will resurface itself.

ivmarkov · 2022-03-20T09:31:33Z

build/build.rs

@@ -95,6 +98,10 @@ fn main() -> anyhow::Result<()> {
            .parse_callbacks(Box::new(BindgenCallbacks))
            .ctypes_prefix("c_types")
            .header(header_file.try_to_str()?)
+            .default_enum_style(EnumVariation::Rust {


ivmarkov · 2022-03-20T09:32:34Z

src/defaults.rs

@@ -0,0 +1,30 @@
+use crate::bindings::*;


Why would we want to do this here and not in esp-idf-svc?

You cannot implement Default in another crate, so it needs to be defined here.

No I mean given that we have a type-safe wrapper for Wifi already, why would you need these defaults here? Or do you plan on implementing another set of type-safe wrappers besides esp-idf-svc? I would be supper happy if you could help us improve esp-idf-svc instead. :)

One more point: we have discussed with Espressif, that the best possible solution is to have ESP-IDF expose C functions that implement these defaults and that we can consume - in addition to their C macros available today that we cannot consume. Timelines unclear though.

I have implemented a different wrapper in one of my projects which used my own bindgen crate before these crates existed. This would enable me to switch to this crate. Also, this could still be used in esp-idf-svc which currently does the same thing as in this Default implementation.

I would be supper happy if you could help us improve esp-idf-svc instead. :)

I will try to have a more detailed look at the esp-idf-svc crate. I think my approach is a more high level API so it could potentially depend on the esp-idf-svc crate.

Ok so I'm all for switching to Rust enums if you can make it work in esp idf svc and esp idf hal. As for the default method, in light of the above idea of having it centralized in ESP IDF and consumable via CAPI call, perhaps we should wait with it a bit?

By the way, EspWifi is pretty high level if you put aside the fact that it is mostly async (for which there are good reasons and you can workaround that). Not sure how much higher level you can get?

perhaps we should wait with it a bit?

I don't think it makes a difference. default can easily be changed to use the CAPI call once it is available.

Not sure how much higher level you can get?

I'm also not sure. As I said, I implemented my wrapper before these crates existed, so I have also not yet reviewed in detail how EspWifi is implemented.

ivmarkov · 2022-03-25T05:58:57Z

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 · 2022-03-25T18:54:55Z

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 · 2022-03-25T19:41:38Z

@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 · 2022-03-26T13:23:49Z

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 · 2022-03-26T17:43:01Z

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 · 2022-03-26T18:33:46Z

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 · 2022-03-27T14:53:03Z

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 · 2022-04-04T15:10:46Z

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.

ivmarkov · 2023-09-19T13:33:26Z

I'm closing this as I don't see us switching to other type of enum representation anytime soon.

reitermarkus force-pushed the improve-codegen branch 2 times, most recently from b60df08 to a3f3dae Compare March 10, 2022 10:36

ivmarkov requested changes Mar 20, 2022

View reviewed changes

reitermarkus mentioned this pull request Mar 25, 2022

Fix enums from esp-idf-sys. esp-rs/esp-idf-svc#59

Closed

reitermarkus force-pushed the improve-codegen branch 3 times, most recently from 8e7d14f to 238b1cb Compare March 25, 2022 05:31

reitermarkus mentioned this pull request Mar 25, 2022

Fix enums from esp-idf-sys. esp-rs/esp-idf-hal#54

Closed

reitermarkus force-pushed the improve-codegen branch from e77e1c6 to 0ef5381 Compare March 25, 2022 17:54

reitermarkus force-pushed the improve-codegen branch from cdb233a to 9b2dd2c Compare March 25, 2022 20:07

reitermarkus added 6 commits March 25, 2022 21:13

Generate Rust enums from C enums.

22c69e8

Implement correct default value for wifi_init_config_t.

b1073ba

Use newtypes for enums instead.

94a7b59

Implement Default for esp_log_level_t`.

4ca7f62

Simplify Default for esp_log_level_t.

49dc87d

Allow trailing comma in macros.

96ea334

reitermarkus force-pushed the improve-codegen branch from 9b2dd2c to 9c95788 Compare March 25, 2022 20:13

Fix CI.

1fdd1e0

reitermarkus force-pushed the improve-codegen branch from 9c95788 to 1fdd1e0 Compare March 25, 2022 20:13

ivmarkov closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code generation. #76

Improve code generation. #76

reitermarkus commented Mar 10, 2022

ivmarkov left a comment

ivmarkov Mar 20, 2022

reitermarkus Mar 21, 2022 •

edited

Loading

ivmarkov Mar 21, 2022

ivmarkov Mar 20, 2022

ivmarkov Mar 20, 2022

reitermarkus Mar 21, 2022

ivmarkov Mar 21, 2022 •

edited

Loading

reitermarkus Mar 21, 2022

ivmarkov Mar 21, 2022

ivmarkov Mar 21, 2022

reitermarkus Mar 25, 2022

ivmarkov commented Mar 25, 2022

ivmarkov commented Mar 25, 2022

reitermarkus commented Mar 25, 2022

ivmarkov commented Mar 26, 2022

reitermarkus commented Mar 26, 2022 •

edited

Loading

ivmarkov commented Mar 26, 2022 •

edited

Loading

N3xed commented Mar 27, 2022 •

edited

Loading

MabezDev commented Apr 4, 2022

ivmarkov commented Sep 19, 2023

Improve code generation. #76

Improve code generation. #76

Conversation

reitermarkus commented Mar 10, 2022

ivmarkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Mar 25, 2022

ivmarkov commented Mar 25, 2022

reitermarkus commented Mar 25, 2022

ivmarkov commented Mar 26, 2022

Constified "enums"

Default::default for this and that

reitermarkus commented Mar 26, 2022 • edited Loading

ivmarkov commented Mar 26, 2022 • edited Loading

N3xed commented Mar 27, 2022 • edited Loading

MabezDev commented Apr 4, 2022

ivmarkov commented Sep 19, 2023

reitermarkus Mar 21, 2022 •

edited

Loading

ivmarkov Mar 21, 2022 •

edited

Loading

`Default::default` for this and that

reitermarkus commented Mar 26, 2022 •

edited

Loading

ivmarkov commented Mar 26, 2022 •

edited

Loading

N3xed commented Mar 27, 2022 •

edited

Loading