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

Add TrueSettle #552

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lekcyjna123
Copy link
Contributor

@lekcyjna123 lekcyjna123 commented Dec 31, 2023

In #547 there is a point:

# TODO: wait for the end of cycle

I have decided to implement a draft of such operator, but I have some doubts about the semantic. Should we allow changes in testbench signal inputs after issuing TrueSettle? From one side I can imaginate that we can want to do after all process are in steady state, but this can cause the next round of changes in signal values. So now I think that after TrueSettle no changes in input signals should be allowed.

@lekcyjna123 lekcyjna123 mentioned this pull request Dec 31, 2023
5 tasks
stubs/amaranth/sim/core.pyi Outdated Show resolved Hide resolved
Comment on lines +217 to +225
def run(self) -> bool:
deadline = self.deadline * 1e12
assert self._engine.now <= deadline
last_now = self._engine.now
while self.advance() and self._engine.now < deadline:
if last_now == self._engine.now:
if self._check_true_settle_ready():
self._unblock_sync_processes()
last_now = self._engine.now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that you had to dig into Amaranth's internals this much. But still, this gives us the functionality we need.

Comment on lines +41 to +53
def true_settle_process(self):
for k in range(self.test_cycles):
yield TrueSettle()
self.assertTrue(self.flag)
self.flag = False
yield

def flag_process(self):
for k in range(self.test_cycles):
for i in range(random.randrange(0, 5)):
yield Settle()
self.flag = True
yield
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these kinds of tests prone to race condition?

@whitequark
Copy link

FYI, the upcoming async testbench RFC will interact with this implementation. (It's not entirely clear to me whether it'll block it, or eliminate the need for it; I'd have to dig deeper into how exactly this works.)

@whitequark
Copy link

@lekcyjna123 Can you tell me more about the issue that triggered the need for TrueSettle? It is possible that this is something I would consider a bug in Settle (now add_testbench in main branch) but I need more details to understand whether or not that's the case.

@lekcyjna123
Copy link
Contributor Author

@lekcyjna123 Can you tell me more about the issue that triggered the need for TrueSettle?

In some cases we would like to wait with some action till the read-only phase where all signals have been already modified and have stabilized, but before the next clock cycle. One of examples is #547 where we would like to record signal value on the end of clock cycle to generate methods usage profiles. Currently we use either:

yield Delay((1 - 1e-4) * clk_period) # shorter than one clock cycle

or
yield Tick("sync_neg")

but both are some kinds of workaround. First use the fact that we know how long is our cycle and second use the fact that we don't do any operations on negative clock edge.

TrueSettle was created to omit this problem and to have a mechanism which will wait till all processes will do TrueSettle or will end the cycle. This is a little bit different way of working from Settle which simple put the process on the end of the execution queue.

But now, when I think after some time about my changes, I am not sure if our whole testing idea isn't wrong. Because we entangle two points: operations on circuit signals and imperative operations on python structures. There shouldn't be any need to wait more than one Settle on operations on circuits. But if we take into account python mocks in circuits and python auxiliary data structures then operations on them have to be executed in correct order and we enforce this order by using Settle, which wasn't created for that. Whats more python mocks will potentially cause invalidation of already settled signals and the need to do reevaluation in circuit. I am going to analyse our testing framework because now I think that instead of adding new synchronization primitives on top of amaranth we should create correct abstraction in our tests.

now add_testbench in main branch

I haven't checked this new functionality yet. I am going to do that today afternoon.

@whitequark
Copy link

But now, when I think after some time about my changes, I am not sure if our whole testing idea isn't wrong. Because we entangle two points: operations on circuit signals and imperative operations on python structures.

This was my concern, but I wanted to understand more about the use case before bringing it up.

Whats more python mocks will potentially cause invalidation of already settled signals and the need to do reevaluation in circuit.

Yes. In fact, you can drive a clock domain from a process. Does this work with TrueSettle?

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.

4 participants