avr-rust / delay

arduino-like delay routines based on busy-wait loops
Apache License 2.0
15 stars 11 forks source link

[cyacc] Add convenience macros to simplify calling delay functions #33

Open lord-ne opened 2 years ago

lord-ne commented 2 years ago

Calling delay functions with non-trivial inputs requires using the turbofish operator and braces. For example, a 5 minute delay would have to be called as delay_sec::<{ 5 * 60 }>().

This syntax is a bit clunky and not very user-friendly. Turbofish is an obscure syntax; even the person who literally wrote these functions forgot that you need to use the turbofish to call them (#32), so I think expecting users to remember it just to use the basic functionality of this library is an unnecessary burden.

This PR adds macros for every delay function (delay_cycles!, delay_us!, delay_ms!, delay_sec!) such that for example delay_sec!(5 * 60) expands to delay_sec::<{ 5 * 60 }>()

bombela commented 2 years ago

I am against this. Its merely cosmetic. You start by hiding the turbofish and inline const expression, and you endup with everything as a macro before your know it. Then you get a split in the community where sometimes code is behind a macro, sometimes it is not, just for cosmetic reasons.

() is for runtime parameters. ::<> for compile time parameters. It's not that hard. And the {} is the same as a block expression to avoid any syntactic ambiguity. Which is already used everywhere in Rust: let foo = { 42 + 1 };, if true { 42 } etc.

rustc will even tell you all about it:

error: comparison operators cannot be chained
 --> src/main.rs:5:5
  |
5 |  foo<42 + 2>();
  |     ^      ^
  |
help: use `::<...>` instead of `<...>` to specify lifetime, type, or const arguments
  |
5 |  foo::<42 + 2>();
  |     ++
  error: expressions must be enclosed in braces to be used as const generic arguments
 --> src/main.rs:5:8
  |
5 |  foo::<42 + 2>();
  |        ^^^^^^
  |
help: enclose the `const` expression in braces
  |
5 |  foo::<{ 42 + 2 }>();
  |        +        +

With that said, at the end of the day, I don't care enough to spend time fighting it.

lord-ne commented 2 years ago

You're right that it's basically just cosmetic. In general I don't mind the turbofish, but when we're just trying to implement "a function whose parameters have to be const" I find it annoying how much more cluttered it is compared to a standard function.

Moving the library over from using regular functions to using const generics is already a big change, and I feel more comfortable making it if it still looks a little bit more like a regular function to the user.

bombela commented 2 years ago

Except they are not regular functions. Macros are also visibly not regular functions. The reason behind the ! is to make it clear that you are using a macro. And that it implies rust code generation.

You could argue that the functions are actually generating machine code without ever producing a function call. That's the whole point. You want an exact number of cycles right here and now. And so maybe you could furthermore argue that a macro reflects this better in abstract.

At this point I wouldn't know what to choose. Having both style might be confusing. I like the purity of keeping it what it is: a function with compile time arguments. Because a function is an abstract concept anyways. If you start doing stuff like delay_s::<{60*5}>() maybe you should use some hardware counter/timer with an interrupt instead and save energy; literally. But I also appreciate the appeal of the clutter free syntax.

At the end of the day, I care more about the cycle accurate codegen than the syntax. I will let others chime in and bring new arguments. If the macro style means the code merges into master faster and is available to all, then so be it.

stappersg commented 2 years ago

On Sun, Jun 12, 2022 at 10:22:48PM -0700, François-Xavier Bourlet wrote:

.... I will let others chime in

Do not wait for me