A lot of "Attempt to prime an already primed trigger for cbValueChange!" messages #729
Comments
|
I would suspect this is due to the changes from 435f7c9, im not convinced the change now keeps a single value change callback for the signal |
|
Will take a look. Do you have a test that reproduces this? |
|
It sounds like the trigger is being primed, but the scheduler never actually holds onto it. I think calling unprime in The alternative would be the reverse, to ensure that a trigger is kept alive by virtue of having a callback registered (edit: this looks like it is already the case?) |
|
Why was the change made that changed this behaviour? |
|
To:
|
|
So there should only have been three objects per signal that were persistent. |
|
That should still be true, with the caveat that they now only persist while being used. I think something very weird is happening here - I don't see the problem on my machine running the tests, and it looks like the trigger is kept alive by the simulator anyway. It would be great if @mciepluc could post an example that causes it. |
|
Ok, confirmed in https://api.travis-ci.org/v3/job/469643596/log.txt |
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Dec 20, 2018
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
We need to override class-construction without allowing `__init__` to run a second time.
One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.
I'm not convinced this is the best approach, but it does fix the problem.
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Dec 20, 2018
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
We need to override class-construction without allowing `__init__` to run a second time.
This introduces a simple metaclass to allow overriding the behavior of `RisingEdge.__call__(...)`
Closed
|
Thanks @eric-wieser for very quick resolution! I can confirm that both #733 and #734 resolve the problem. |
|
Well, I broke it so felt a quick fix was necessary! |
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 7, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
We need to override class-construction without allowing `__init__` to run a second time.
This introduces a simple metaclass to allow overriding the behavior of `RisingEdge.__call__(...)`
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 7, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
We need to override class-construction without allowing `__init__` to run a second time.
One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.
I'm not convinced this is the best approach, but it does fix the problem.
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 7, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
We need to override class-construction without allowing `__init__` to run a second time.
This introduces a simple metaclass to allow overriding the behavior of `RisingEdge.__call__(...)`
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 7, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
B = SomeClass(1)
```
We need to override class-construction without allowing `__init__` to run a second time.
One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.
I'm not convinced this is the best approach, but it does fix the problem.
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 7, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 8, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
eric-wieser
added a commit
to eric-wieser/cocotb
that referenced
this issue
Jan 8, 2019
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
|
Thanks all, issue fixed in current master |
ktbarrett
pushed a commit
to ktbarrett/cocotb
that referenced
this issue
Mar 6, 2020
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
B = SomeClass(1)
```
We need to override class-construction without allowing `__init__` to run a second time.
One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.
I'm not convinced this is the best approach, but it does fix the problem.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Seems one of the latest merges caused additional production of a lot of such log messages:
I don't quite have time to look into it now, maybe someone know what is going on?
The text was updated successfully, but these errors were encountered: