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

How should collections like pins, regs, sub_blocks be accessed? #22

Open
ginty opened this issue Dec 10, 2019 · 10 comments
Open

How should collections like pins, regs, sub_blocks be accessed? #22

ginty opened this issue Dec 10, 2019 · 10 comments
Labels
feature discussion Discussion on a feature's requirements or implementation

Comments

@ginty
Copy link
Member

ginty commented Dec 10, 2019

In O1 we had various ways of accessing items from a collection, e.g. for a reg I think all of these worked:

dut.reg("my_reg")
dut.regs("my_reg")
dut.reg["my_reg"]
dut.regs["my_reg"]
dut.my_reg

While that's quite nice from the pov of making whatever a user might guess have a good chance of working, I also think there's a lot to be said for Python's "one way of doing things" mantra and that perhaps removing all the aliases would make things less confusing to novice eyes.
I think there's also an argument that you wouldn't feel like you need to guess if there's only one style that you would ever see being used and it was consistent across all APIs.

So for O2, should we select a single one of the above and then consistently use that style only across all APIs (regs, pins, sub_blocks, specs, etc.)?

Pesonally, I think we should and I vote for the pluralalized square-bracket variant:

dut.regs            # => A collection that behaves like a hash (a dict in Python)
dut.regs["my_reg"]  # => An item from the collection
@ginty ginty added the feature discussion Discussion on a feature's requirements or implementation label Dec 10, 2019
@ginty ginty mentioned this issue Dec 10, 2019
Merged
@coreyeng
Copy link
Member

I agree, except I'm okay with having both a singleton and pluralized method name. For example:

dut.reg(..) #=> return a single reg.
dut.regs[...] #=> Indexed from a dictionary (or dictionary-like) class.

I think its clear that dut.reg yields a single item and dut.regs[] is returning an object which is then indexed.

Python also has some 'close but not exactly the same' methods we can justify the above with. For example, if you do {}["hi"] you'll get a KeyError, but {}.get("hi") will return None. We could do something similar here:

reg("blah") #=> None
reg("blah") #=> Raises KeyError (from underlying dictionary/dictionary-like)

coreyeng added a commit that referenced this issue Dec 11, 2019
…ts ensuring pin API is available on subblocks as well as the DUT.
@ginty
Copy link
Member Author

ginty commented Dec 11, 2019

OK, that sounds good.

Just to clarify, this is what you mean for the missing item case:

reg("blah")  #=> None
regs["blah"] #=> Raises KeyError (from underlying dictionary/dictionary-like)

That's quite nice and means that we don't need to add a has_reg and similar methods.

if dut.pin("tdo"):
  # ...
else:
  #...

@pderouen
Copy link
Member

I'd love to have the consistency across object types (pins, regs, etc). I recall being very confused about not being able to pin.write and pin.read (until the aliases were added).

I'm neither for nor against having multiple aliases for the same functionality. I like the simplicity of a single way to do it. But, I also see the value of allowing a different dialect like pin.assert if that's how you're used describing, "check the state."

@ginty
Copy link
Member Author

ginty commented Dec 11, 2019

@pderouen, funnily enough I was just thinking of that over on #23.
I think I would like us to try and aim for one method right now, rather than throwing in a bunch of aliases.
So think about whether it should be read/write, set/get, send/receive, etc....

@coreyeng
Copy link
Member

@pderouen Just a side note: we can't use assert in python as its a keyword, even as a member name. We might be able to get it to work in a hacky way through monkeypatching, but I think we'd want to avoid that.

I've got verify as "check the state now" because back when I was introducing O1 to some new users, they were getting confused by read meaning "check the state" vs "capture the value".

@coreyeng
Copy link
Member

I'd also say that I'm on board with reverting it if that's the consensus. Just something I threw in for now since as it occurred to me.

@ginty
Copy link
Member Author

ginty commented Dec 11, 2019

This seems pretty self-explanatory:

reg.write
reg.verify
reg.capture

pin.write
pin.verify
pin.capture

@chrishume
Copy link

chrishume commented Dec 17, 2019

Late to the discussion, but is anything related to access mechanisms for reg/regs tied to how one can iterate on them? I frequently have to iterate on a block's (and recursively through a block's sub-blocks) registers, but O1's nesting access for registers isn't very friendly to nested iteration (IMO). It leads to this below - regblk.regs(reg[0]).addr, instead of something direct like "regblk.reg(reg).address":

def build_headers(regblk, blk_count)
  #Loop through 8bit registers to create 16-bit regs
  new_regs_by_addr = Array.new
  unless regblk.regs.empty?
    # Build an address-based array of hashes that covers the span of registers
    regs_by_addr = []
    regblk.regs.each do |reg|
      regs_by_addr << {:addr => regblk.regs(reg[0]).addr, :reg => regblk.regs(reg[0])}
    end
…
end

I'll take this to a separate issue if the two aren't tied together. I'm very much a fan of a singular access method for each type - singular and plural.

@ginty
Copy link
Member Author

ginty commented Dec 17, 2019

Hi @chrishume, is the problem that you are treating regblk.regs like an Array when it is actually a Hash? i.e. it should be:

regblk.regs.each do |name, reg|
  puts "The address of #{name} is: #{reg.address}"
end

I expect O2 to be similar in this respect, but perhaps better by being more consistent - sub_blocks, regs and pins will all behave like this, whereas I think that may not have been true for O1.
In O2 all collections will return something (a custom class) which behaves like a Python Dictionary, which is basically a Ruby Hash.

@chrishume
Copy link

That's entirely possible, @ginty. I'll mess around with it while doing the other updates (spec testing would be a good place to test the iteration) and see how things work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature discussion Discussion on a feature's requirements or implementation
Projects
None yet
Development

No branches or pull requests

4 participants