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

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

Open
wants to merge 1 commit into
base: cycacc
Choose a base branch
from

Conversation

lord-ne
Copy link

@lord-ne lord-ne commented Jun 12, 2022

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 }>()

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 }>().

Since this syntax is a bit clunky and not very user friendly, macros
are added for each delay function such that delay_sec!(5 * 60) expands
to delay_sec::<{ 5 * 60 }>()
@bombela
Copy link

bombela commented Jun 13, 2022

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
Copy link
Author

lord-ne commented Jun 13, 2022

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
Copy link

bombela commented Jun 13, 2022

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
Copy link
Member

stappersg commented Jun 13, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants