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

Improve readability of assignments and add contexts for clock domains #345

Open
ildus opened this issue Apr 5, 2021 · 0 comments
Open

Comments

@ildus
Copy link

ildus commented Apr 5, 2021

HI, I started having a look to nmigen and I see a lot of eq statements which are decrease readability of clauses. I think there is a room of improvement. So I created a little of POC patch which adds contexts for clock domains. Please tell me if it was discussed before.

That patch is using slicing mechanism to assign new values to signals. The [:] construction is used to assign to whole variable, and slices with numbers to assign to the part of values. This works if the context was created for a clock domain. The modifed ALU example as a demonstration:

    def elaborate(self, platform):
        m = Module()

        with m.Domain(sync=True):
            with m.If(self.sel == 0b00):
                self.o[:] = self.a | self.b
            with m.Elif(self.sel == 0b01):
                self.o[0:5] = (self.a & self.b)[0:5]
            with m.Elif(self.sel == 0b10):
                self.o[:] = self.a ^ self.b
            with m.Else():
                cat = Cat(self.o, self.co)
                cat[:] = self.a - self.b

        return m

The patch itself:

diff --git a/nmigen/hdl/ast.py b/nmigen/hdl/ast.py
index af1d831..1f41db5 100644
--- a/nmigen/hdl/ast.py
+++ b/nmigen/hdl/ast.py
@@ -24,6 +24,21 @@ __all__ = [
 ]


+class CurrentClockDomain:
+    _current_clock_domain = None
+
+    @classmethod
+    def set_current(cls, domain):
+        if domain is not None and cls._current_clock_domain is not None:
+            raise SyntaxError("clock domain already set")
+
+        cls._current_clock_domain = domain
+
+    @classmethod
+    def current(cls):
+        return cls._current_clock_domain
+
+
 class DUID:
     """Deterministic Unique IDentifier."""
     __next_uid = 0
@@ -239,6 +254,15 @@ class Value(metaclass=ABCMeta):
         else:
             raise TypeError("Cannot index value with {}".format(repr(key)))

+    def __setitem__(self, key, value):
+        item = self.__getitem__(key)
+
+        domain = CurrentClockDomain.current()
+        if domain is None:
+            raise ValueError("not in clock domain context")
+
+        domain += Assign(item, value, src_loc_at=0)
+
     def as_unsigned(self):
         """Conversion to unsigned.

diff --git a/nmigen/hdl/dsl.py b/nmigen/hdl/dsl.py
index 85ae472..0f35678 100644
--- a/nmigen/hdl/dsl.py
+++ b/nmigen/hdl/dsl.py
@@ -301,6 +301,19 @@ class Module(_ModuleBuilderRoot, Elaboratable):
             self._ctrl_context = None
         self._pop_ctrl()

+    @contextmanager
+    def Domain(self, domain=None, sync=True):
+        from nmigen.hdl.ast import CurrentClockDomain
+
+        if domain is None and not sync:
+            domain = self.d.comb
+        elif domain is None and sync:
+            domain = self.d.sync
+
+        CurrentClockDomain.set_current(domain)
+        yield
+        CurrentClockDomain.set_current(None)
+

The implementation could be different but I think the idea is shown.

@ildus ildus changed the title Improve readability of assignments Improve readability of assignments and add contexts for clock domains Apr 5, 2021
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

No branches or pull requests

1 participant