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

[WIP] API changes #1

Closed
wants to merge 2 commits into from
Closed

[WIP] API changes #1

wants to merge 2 commits into from

Conversation

cfelton
Copy link

@cfelton cfelton commented Jul 7, 2017

Recently after the first release I wanted to propose some API changes, I implemented the changes and tested. Now I see that development has progressed ... before merging with the latest I figured I better post the proposed changes before investing more development time. The following is a description of the proposed API changes.

Proposing a change where a Peeker object can be created with a set of signals instead of a Peeker foreach.

p = Peeker(z=z, a=a, b=b, sel=sel)
p.to_wavedrom()

This creates a Peeker object with the signals to be "peeked" at, in the keyword parameters the names can be specified. This should work the same as the existing but you don't need to create a Peeker for each signal, just a Peeker for a set of signals. With this approach you can also create various Peekers at different levels.

The tests and examples have been updated in these changes to reflect the proposed API changes. If this proposal is of interest I can merge it with the latest (???) and update this merge request.

Proposing a change where a `Peeker` object can be created with a set of
signals instead of a `Peeker` foreach.

```python
p = Peeker(z=z, a=a, b=b, sel=sel)
p.to_wavedrom()
```

This creates a `Peeker` object with the signals to be "peeked" at, in the
keyword parameters the names can be specified.  This should work the same
as the existing but you don't need to create a `Peeker` for each signal,
just a `Peeker` for a set of signals.  With this approach you can also
create various `Peeker`s at different levels.

Things that should be added:

  `Peeker.add_signal(sig)``: method that allows a signal to be added after
    creation, could even overload the `__add__`; `p += mysig`

  `Peeker.rm_signal(name)`

  Add checkers to `to_wavejson` to verify no parameters where passed
  via `names` (\*args).
With the API changes one feature was missing from the previous and that
was the ability to have a single global peeker that included the signals
of all the peekers added through the design.  This is handled by keeping
track of all the keepers in a global dict.

Things that need to be added:

1. add __del__ to remove a peeker from the global list when it is deleted.
2. clear_all function to clear all the keepers from the global list.
@xesscorp
Copy link
Collaborator

xesscorp commented Jul 8, 2017

Thanks for the proposal, Chris. I have a few questions:

I like being able to specify the Peeker name as a keyword argument. Would there still be a way to set the Peeker name to something that wasn't a legal Python identifier? (For example, if I wanted to use a + b as a Peeker name.)

One advantage you list is that only a single Peeker is needed for multiple signals. Is the advantage that it allows more concise MyHDL source code? Or is the advantage that it requires less processing resources to handle a single @always_comb construct rather than multiple ones?

You say the proposed change lets you "create various Peekers at different levels". The current implementation already lets you do this at different levels of the hierarchy. Are you referring to something else? Or is there some other capability this gives you w.r.t. hierarchy?

How would I do triggering with a Peeker that contains multiple signals? In the example you gave, would I trigger on a positive edge of the select signal using something like p['sel'].posedge()?

If I had a Peeker that only monitored one signal, would I still have to reference it for a trigger as p['sel']? Or would just p be sufficient?

Another question (somewhat related to the previous two) is what type of object is returned by p['sel']? Is it the internal Trace object that stores the signal time & value samples? Or is it a specially-created Peeker that only contains a single signal?

Let me know your thoughts on this.

@cfelton
Copy link
Author

cfelton commented Jul 20, 2017

I apologize for my late response, unfortunately will be the case for a while :)

Would there still be a way to set the Peeker name to something that wasn't a legal Python identifier?

Yes there could be a method to do this. If this is a rare case then it might make sense, if this is a common case (using a non-valid Python identifier as the name) then it might be more of a pain compare to the existing. What I would propose is:

p = Peeker(a=a, b=b, c=c)
p.c.name = "a+b"

You say the proposed change lets you "create various Peekers at different levels".

I should of said "maintains" this feature.

How would I do triggering with a Peeker

I don't know at this point in time, I made these changes shortly after you posted the first version - no triggering existed. I haven't looked at the triggering mechanism you have subsequently added. All triggering issues are TBD. Yes, I would say it would be p.sel.posedge and I guess a Peeker could have a global property where it is an "or" of all traces, so p.posedge would also work.

Another question (somewhat related to the previous two) is what type of object is returned by p['sel']?

Currently __getitem__ in class Peeker isn't implemented (I think this is the case for the version I worked off?). I would propose p.sel and the sel attribute would be of type Trace.

@xesscorp
Copy link
Collaborator

Chris, can we get the features you need by defining a new class called PeekerGroup which is essentially a dictionary containing references to individual Peekers? Then you could do:

p = PeekerGroup(z=z, a=a, b=b, sel=sel)
p.to_wavedrom()

but the original Peeker class doesn't have to change. And doing p.sel or p['sel'] would fetch the individual Peeker object.

@xesscorp
Copy link
Collaborator

Chris, I implemented my version of your idea and placed it in the peekergroup branch. Try it out and let me know if it meets your needs or if it requires enhancement.

@xesscorp
Copy link
Collaborator

Hey, Chris. I'll assume my changes satisfied enough of your concerns to be acceptable. I'll close this PR but we can re-open it if you disagree.

@xesscorp xesscorp closed this Nov 28, 2017
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

2 participants