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

RFC for Strobe Generator #26

Closed
wants to merge 3 commits into from
Closed

Conversation

galibert
Copy link

@galibert galibert commented Sep 3, 2023

@whitequark
Copy link
Member

@galibert It is conventional to add a "rendered" link in the description of your RFC. You can get it by going to your branch and finding the file on GitHub: https://github.com/galibert/amaranth-rfcs/blob/strobegen/text/0026-strobegen.md

@whitequark
Copy link
Member

My personal view on this is that I don't think patterns that take less than 5 lines of Amaranth code normally should be added in the standard library. You can just write them out in your code.

In fact I'd like to see these removed from the standard library in the cases where they are added, e.g. the accepted RFC 18: https://amaranth-lang.org/rfcs/0019-remove-scheduler.html

@galibert
Copy link
Author

galibert commented Sep 4, 2023

It's five lines of code, and 70 lines of explanation for why it works. This is the kind of reason why I think best practices have a place in the lib.

@whitequark whitequark added the meta:nominated Nominated for discussion on the next relevant meeting label Sep 4, 2023
@whitequark whitequark added the area:core RFC affecting APIs in amaranth-lang/amaranth label Sep 25, 2023
@galibert galibert closed this Oct 9, 2023
@whitequark whitequark removed the meta:nominated Nominated for discussion on the next relevant meeting label Nov 22, 2023
@whitequark
Copy link
Member

Reopened as requested by the author.

@whitequark whitequark reopened this Nov 22, 2023
@whitequark whitequark added the meta:nominated Nominated for discussion on the next relevant meeting label Nov 22, 2023
@whitequark
Copy link
Member

whitequark commented Jan 8, 2024

We have discussed this RFC on the 2024-01-08 weekly meeting. We did not reach a consensus. The RFC remains nominated.

@whitequark whitequark removed the meta:nominated Nominated for discussion on the next relevant meeting label Feb 19, 2024
@whitequark
Copy link
Member

whitequark commented Feb 19, 2024

We have discussed this RFC on the 2024-02-19 weekly meeting. The disposition was to close, with one abstention (@jfng). The following reasoning was provided:

@whitequark:

my vote on RFC 26 is: close, for the following reasons:

  • it is not difficult to implement this functionality in-place and in my view this will usually be worth it because it will be integrated with some wider reset or clocking scheme
  • having three new classes for this purpose does not have a high enough utility for the increase in our API surface [also emphasized by @tpwrules]
  • the functionality could be implemented in an external library and later pulled in if we find that it was widely useful

and the following note:

  • if we add EnableSignal and platform.unsafe_add_multicycle_constraint, we can add something like FixedStrobeGenerator as it will provide a clear safety benefit

In addition we have noted that:

  • discoverability of Amaranth libraries should be improved (@galibert, @cr1901)
  • lib.coding would not have made it to the standard library if it was proposed as an RFC and should probably be removed

Thank you for this proposal, @galibert.

@whitequark whitequark closed this Feb 19, 2024
@FatsieFS
Copy link

BTW, it was @jfng who abstained; I voted for closure.

@whitequark
Copy link
Member

I misread the message. Thanks, I've updated the resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core RFC affecting APIs in amaranth-lang/amaranth
3 participants