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

Pins #21

Merged
merged 23 commits into from
Dec 31, 2019
Merged

Pins #21

merged 23 commits into from
Dec 31, 2019

Conversation

coreyeng
Copy link
Member

Hello,

This is a (pretty elementary) starting point for adding pins. So far, all I've got is setting a pin value (posturing) then setting an action (Drive, Verify, Capture, HighZ). See here for the Python-side API.

I changed the API around from the Regs and SubBlocks do to fit more with what O1 had, where pins are added through add_pin on the DUT instead of brought in from a separate file. I don't object to the latter, and its on the to-do list, but I went with the old O1 API to start.

I also moved the Proxy for pins behind a dut.__proxies__ class, allowing the use of pins and other methods as in O1 and hiding the proxy further from the user. The proxy is still available directly from dut.__proxies__['pins'].

Like I said, this is pretty basic at the moment. O2 is moving faster than I thought it would and since I diverged from Registers and SubBlocks I want to put this out as an alternative scheme. Some next steps for me in either a future PR, or just building on this one, more or less in order of priority:

  1. Error handling. I currently have no error handling. It'll just crash. Obviously this is bad and needs some work.
  2. Add pin alias support, like O1 had.
  3. Add pin group support with posturing and actions affecting the underlying pins.
  4. Add pin slicing support.
  5. Support for loaders, like what Regs and SubBlocks have, for pins, pin groups, and aliases.
  6. Support for pin group/pin metadata and pin roles (e.g., Power, Ground, etc.).

There's some stuff floating around for this already. For example, I started a PinCollection struct but haven't gotten around to integrating, so its just dead code at the moment. But, it shouldn't be for long. I also think that there's enough here to get started with vectors and pattern generation though. And 1) shouldn't be too much a problem since I think the vector/pattern generation side will be on the Rust side.

Thanks!

@coreyeng coreyeng requested a review from ginty December 10, 2019 17:22
@ginty
Copy link
Member

ginty commented Dec 10, 2019

Hi @coreyeng,

Looks good.

Enabling dut.add_pin first is the right thing to do. I realize that I have done it the wrong way around by adding the loader API for the regs first because you can't invoke that from a test (or anywhere else). So we should have the dut method and then call that from the loaders. I'll bring regs into line in due course.

A few changes for consideration:

I see you have a few ways of accessing individual pins, but I think we should firm up on one style that we will use across the board. I've just opened a ticket for that so we can discuss that one over there if required - #22

Where does the posturing term come from, do we need it? It seems to be just a data attribute (which gets combined with a state attribute to form the overall pin status), but the term is confusing to me as it makes it sound like it is more than that.

Can we move the pins into Model. Having it with a dedicated pin collection at the top will limit us to only having pins at the top-level. In the model will mean that we can support multi-chip modules better in future by having layers of pins, though for now we can just concentrate on having them defined in the top-level.
So it should be:

origen.dut.db.add_pin("", blah, blah) # First empty path arg means the top-level

Also _origen.model should be removed and all such APIs should go through origen.db + a path.

I think consistency is very important at this stage and the main thrust of this feedback is to keep everything heading in the same direction so that it feels like a cohesive framework rather than a set of individually designed components.

If you think the way you have done things here have advantages vs. the way I am advocating then let's definitely have that discussion.
For me the main thing is that we don't want alternative systems in co-existence.

Thanks!

@coreyeng
Copy link
Member Author

Thanks for the feedback @ginty! To answer the considerations:

I see you have a few ways of accessing individual pins, but I think we should firm up on one style that we will use across the board. I've just opened a ticket for that so we can discuss that one over there if required - #22

I updated it to work as I described in my comment there. I updated the tests so you can see how it looks there. I think it sticks pretty close to the dict API. We could update to get_pin too though.

Where does the posturing term come from, do we need it? It seems to be just a data attribute (which gets combined with a state attribute to form the overall pin status), but the term is confusing to me as it makes it sound like it is more than that.

This was my attempt at retaining the drive vs. drive! behavior from O1. I'd be okay with scrapping this and going with drive_high, drive_low, assert_high, and assert_low and changing actions to state. Then I guess you'd have to call cycle to update? So an O1 script of:

dut.pin('test').drive!

Would be written as

dut.pin('test').drive_high
tester.cycle()

We could even return something that can call cycle() itself and could rewrite as dut.pin('test').drive_high.cycle().

Can we move the pins into Model. Having it with a dedicated pin collection at the top will limit us to only having pins at the top-level. In the model will mean that we can support multi-chip modules better in future by having layers of pins, though for now we can just concentrate on having them defined in the top-level.
So it should be:

origen.dut.db.add_pin("", blah, blah) # First empty path arg means the top-level

Also _origen.model should be removed and all such APIs should go through origen.db + a path.

I added some tests to make sure the pins are still available on the models. I can shift the actual proxy calls back to dut.db, but is that where the add_pin should be? Are you meaning the 'developer side' should go through origen.db?

I think consistency is very important at this stage and the main thrust of this feedback is to keep everything heading in the same direction so that it feels like a cohesive framework rather than a set of individually designed components.

If you think the way you have done things here have advantages vs. the way I am advocating then let's definitely have that discussion.
For me the main thing is that we don't want alternative systems in co-existence.

Completely agree, and that's why I opened this a bit prematurely. Normally I'd of waited a bit to be more feature-rich and tested. If I'm understanding the origen.db as a developer-side thing, then I think that's what I was doing with the proxies class... just trying to move that out of the way. It sounds from #22 as well that you'd like dut.regs and dut.sub_blocks to return either a dictionary/dictionary-like type, not necessarily the proxy itself (unless we update the proxies to be dictionary-like, which we can do too).

@ginty ginty mentioned this pull request Dec 11, 2019
@ginty
Copy link
Member

ginty commented Dec 11, 2019

Hi @coreyeng,

Looks like we are agreed re. #22.

I've opened #23 to discuss how to handle application of state to the DUT, I like the chained idea you mention here.

Yes I mean that the call to Rust to add a pin should be through origen.dut.db and that the pins should be stored as part of that PyDUT struct (part of Model) rather than in their own collection.
One of the things that I can see is going to be difficult is how to make reference across different collections like DUT and your PinContainer - because from Rusts pov both of these live outside of its memory model (they are in memory owned by Python) and so its borrow checker goes nuts if you try to do even basic things.
So we can simplify that straight away by making the pins live in the same DUT data structure as everything else (and which makes logical sense anyway).

Then as long as a function has a handle on the DUT, then it can easily access both pins and regs, and specs, and timing and anything else it may need from it.
And the easiest way to get a handle on the DUT is to call a function on it, so that's why pretty much every call into Rust should be via origen.dut.db.....

I may as well also say that there is much more front end Python code here than I would really like or envisage, however I haven't been able to get an alternative system working yet, so I'm not going to block progress at this point.
However, I can outline what I am thinking (not least so that maybe you will be able to solve it!)...

So the sub-blocks that I implemented at first does have a fair amount of shadowing on the Python front end - a pure Python object (a controller) is instantiated for every Model that exists in the backend.
The reason for that is because the controller is where all the front end application code will go - think read_register methods, etc. So there has to be a pure Python framework for that and then a corresponding mirror of it in the backend.

However, its kind of a lot of work setting up that mirroring since it means you need to maintain a Python version and Rust version of the same APIs. e.g. I think you have concentrated here on making a Python API available, however someone writing the JTAG driver in Rust in the future is going to need a very similar pin API on the backend and so that will need to be added in Rust too.
Ideally, I don't really want the people writing higher level stuff like drivers in Rust to have to be fully aware of the low level details, rather it would be better for them to feel like they are just using the same Origen APIs that they are used to, but just with Rust syntax instead of Python.

I think one of the big features of this PYO3 library is that its system of marking up Structs with #[pyclass] and so on is that at the end of the day they are still Rust structs that can be normally instantiated and called by Rust code.
So I believe the direction we should really be heading is that the front end Python API methods are actually implemented back in Rust. That naturally means that you implement once - the driver dev working in Python calls the exact same functions as the driver dev working in Rust.

So when front end code calls dut.pins or dut.regs, etc. then that should really be translated into a call to origen.dut.db.regs which returns an instantiated Pyclass struct which provides the methods for working with the given collection.

I got stack overflow to help me with how to create such a function that returns a py class - https://stackoverflow.com/questions/59204849/how-to-implement-a-rust-function-that-returns-a-python-object-using-py03

Note that pyo3 supports definitions for all the magic methods that we might want to use to make such a class dict-like or to display nicely in the console - https://pyo3.rs/v0.8.3/class.html#class-customizations

What I'm currently struggling with though is how to have such a class contain references to the DUT objects they represent - e.g. to the pin objects stored in PyDUT.

I get the sense that this blog post holds the key, but its at a level of Rust-fu that is currently well above me: https://raphaelgomes.dev/blog/articles/2019-07-01-sharing-references-between-python-and-rust.html

Its feeling to me right now that that is something we won't really be able to properly tackle until we get a good 6 months or so of Rust under our belts - I think here we are pushing up against expert-level stuff within our first month of using Rust.
So I'm currently thinking of maybe just storing string references in such pyclass objects (similar to how I did with the path references to sub-blocks) and then when you call a function like drive(1) on that class, then it would just fetch the pin from the DUT (easy because it will have been called on origen.dut.db..) by supplying its string reference (easy, e.g. just the name of the pin stored as a String), and then manipulate the pin Struct as required.

That will be a bit slower than just de-referencing an existing pointer, but its likely to still be quick compared to what we are used to and it still has the same external interface we want to end up with. Then, when our Rust skills are up to it (or maybe yours are already!), we can replace the internals with a more efficient lookup mechanism in future.

So that's what I'm thinking, but I'm probably still a week or so from being able to show a proof of concept on the regs, but I would recommend that you think about experimenting along similar lines for the pin API in parallel.

@coreyeng
Copy link
Member Author

Hi @ginty,

Okay, I think shifting back my container thing into db calls shouldn't be difficult. To answer the second point, the reason I have so much Python code now was to cache the pins on the Python side, which I thought is what you were doing with _storage in the registers/subblocks (I probably should have called __cache__ _storage to make that clear). Actually, I wanted to ask about that too and see what the advantage was. I'm wasn't sure if it was speedier to cache references in Python vs. returning a casted-object from pyo3. If I throw out the cache then all that shifts back to pyo3 and the Rust core pretty easily.

I also encountered all the lifecycle and borrowing issues with pyo3... the silver lining is it makes some design decisions for us. I tried at first storing references but that only works if we're okay with static lifetimes (which I'd assume we aren't!).

@ginty
Copy link
Member

ginty commented Dec 11, 2019

Hi @coreyeng,

I don't think we should really be aiming to cache much on the Python side, I think setting and getting attributes from the DUT should involve a round trip to Rust in both cases, at least until performance tells us we need otherwise.

What's there for the regs right now is just me playing around, so sorry if I gave that impression.
And as I said above, sub_blocks are a special case I think to support the fact that the application will want to pile a lot of controller code into them.

@info-rchitect
Copy link
Member

info-rchitect commented Dec 11, 2019

WRT to this @ginty comment:

So we can simplify that straight away by making the pins live in the same DUT data structure as everything else (and which makes logical sense anyway).

Wouldn't this also make it easier to export the DUT model in any external format (if needed)?

@coreyeng
Copy link
Member Author

Hi @ginty,

Okay, sounds good! If I ax all the caching stuff, I think it'll naturally fall in line with what you want. And moving the backend access to origen.dut.db shouldn't be a problem either.

@ginty
Copy link
Member

ginty commented Dec 11, 2019

@info-rchitect, yes, absolutely

@info-rchitect
Copy link
Member

I am glad we are discussing this as I don't think the rest of us can contribute in a positive manner until the rust to python exchange pattern is created. Once this is done, wash-rinse-repeat will be much easier.

@coreyeng
Copy link
Member Author

Hi @ginty,

Question as I'm looking to move pins into the model: Is the origen.dut.db meant to be a single layer? So would a call to, say, origen.dut.sub_block('sb').add_pin('p') be translated to origen.dut.db.add_pin('dut.sb', 'p')?

@ginty
Copy link
Member

ginty commented Dec 11, 2019

@coreyeng, yes exactly like that

@ginty
Copy link
Member

ginty commented Dec 11, 2019

@coreyeng, though note that the path arg is relative to the DUT, so it should be "" to mean the DUT itself and your example would be:

origen.dut.db.add_pin('sb', 'p')

@coreyeng
Copy link
Member Author

Hi @ginty,

I think I've got enough here for some basic pin interactions. I've got a good start on pin groups as well, but I had some borrowing issues and had to shift everything back to the model space.

Most of the Python code is gone. The only remaining bits are added directly to origen.dut and I couldn't get the subclass option from pyo3 to work. It said it was experimental, but it was worth a try anyway.

I've got a lot of the dict-like structure for dut.pins and dut.pin_groups. I'm still working on them, but its getting closer.

I've also got everything going through dut.model.

I got rid of posture and went with set_data like we were discussing in #22.

I think that answered the main concerns we had before. I need to learn pytest some. Right now I've just got a bunch of stuff stacked up in a single test, but it shows the Python API I've got so far.

Thanks!

Copy link
Member

@ginty ginty left a comment

Choose a reason for hiding this comment

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

Looks good @coreyeng, just a couple of non-blocking comments.
Thanks!

example/tests/pin_api_test.py Outdated Show resolved Hide resolved

assert len(origen.dut.pin_groups) == 1
assert 'test_grp' in origen.dut.pin_groups
assert isinstance(origen.dut.pin_group('test_grp'), _origen.dut.pins.PinGroup)
Copy link
Member

Choose a reason for hiding this comment

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

I kinda agree with @pderouen, is making the user differentiate between pins and pin_groups the right thing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

My main reasons were, we could return a consistent class instead of sometimes a pin and sometimes a pin collection. Also, I don't think we had an easy way to get pin groups or all available names which, from an auto-doc perspective is something we'd need. It was also a way to differentiate between a physical pin and a more abstract grouping of them.

Based on that, it seemed like just making them distinct was the way to go. We could probably meet in the middle and do something like:

dut.add_pin('p1')
dut.add_pin('p2')
dut.group_pins('p', 'p1', 'p2')

dut.pin('p1') #=> PinGroup of size 1
dut.pin('p') #=> PinGroup of size 2

dut.pins #=> ['p1', 'p2',  'p']
dut.physical_pins #=> ['p1', 'p2']

This way, I can return a consistent class, pins/pin_groups/aliasing will all work at the pins level, and physical_pins can return the actual hardware pins. From the Rust perspective, I can keep the same scheme of the pin groups just operating on the underlying pin objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pderouen @ginty any comment here? I need to add some error handling before merging, but if the prevailing opinion is to merge pin and pin group retrieval, I need to do that first.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be handled similar to regs. For regs most of the API will be implemented by a BitCollection object which can contain one or more bits. That means from a user perspective there is no API difference between dealing with a single bit, a whole register or a partial register (and hopefully we can even extend that in O2 to be larger collections like multiple registers or even whole sections of memory).

Pins should probably be the same. Internally we should have a collection of all individual pin objects and a separate collection of pin groups - which don't contain the pins themselves but just the details of what pins are included in the given group and any other metadata associated with it.
Probably another collection would be a hashmap of aliases.

pin('blah') should look up individual pins, alias and pin groups for a match and wrap the resolved pin(s) in a PinCollection for returning to the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I think we're on the same page then. The only difference I have is I have a PinGroup as a named PinCollection.

… indexing classes from Py03. Have some basics working. Experimenting with cleaning up code duplication using macros. Added support for pin_collections as an anonymous pin group
@coreyeng coreyeng mentioned this pull request Dec 19, 2019
…Copmleted group/collection indexing. Added Python-level tets. Added a bunch of error handling.
@coreyeng
Copy link
Member Author

Hi,

This took a bit longer than expected... mostly because I kept going on tangents and kept adding 'just one more thing' about six or seven times. Anyway, the initial concern should be addressed. See the test suite for example usage.

I have three "pin-objects" now:

  • A pin: which is a physical pin, always of size 1. I made this (from the Python side) immutable: I removed all the setters that I had. Its available for querying as a 'lowest level' object, but all state changes should come from the associated pin group or pin collection.
  • A PinCollection, which behaves like the pin collections of O1. Using origen.dut.pin(...) or origen.dut.pins[...] will return a PinCollection, instead of a pin as before. Getting a pin just added through add_pin will give you a PinCollection of size 1. From this, all the setters and state changes are available.
  • A PinGroup, which is just a named PinCollection. I did this to indicate to the user that one is more permanent than the other. Other than that, the two are the same.

The physical pins can accessed using dut.origen.physical_pins. I've got a similar 'dict-like' structure for this, so it should be easy to pluck out the actual pin IDs, if needed. Because I provided this, I went against #22 and have iterators using all available names, meaning they'll be duplicates at the physical level. I think this dis-spells some of the magic behind pin/aliasing lookups, and further drives home the point that these are not the physical pins. And, this does allow for iterating through all available names easily.

Some other notes:

  • I got slicing to work. Not sure Regs WIP #39 includes this or not, but we should have an example here if not.
  • I think I hit all the main issues that would cause Rust to panic. Before, any unexpected input would cause Rust to either crash or hang. That should be fixed now and we should see a Python error.

Future Stuff:

  • I went a little "mutable-happy" on the Rust side, mostly because I kept having to change my getters in order to reuse as setters. But, that makes for unnecessary cloning abound. For most stuff its NBD, but taking into account possible scales like what @info-rchitect has,.. unnecessarily cloning a vector with 2096+ strings when its not needed is a waste. This is fixable, but all the changes will be behind the scenes.
  • Since this is still a WiP, I haven't done anything with pin groups of size > 1 yet (only creating them). I'm pretty sure nesting pin groups won't work as expected. I intend to work on that next, but the existing API should remain about the same.
  • Kind of along with the above, the length of a group or collection is the number of items, before any resolution. So a group consisting of two ports, both of size 8, will have a length of 2, not 16. I intend to add a width method to lookup the resolved bus size, which would correctly return 16.
  • I want to experiment with generics and traits for more code reuse. I tried a bit and got some things to work here and there, but not all the way. This has passing tests, so I'd like to merge this first before re-writing with generics.

… O1, instead of trying to be fancy and storing the IDs with which the group was created with. Added several tests to ensure adding groups of varying widths work.
@ginty
Copy link
Member

ginty commented Dec 28, 2019

This looks great @coreyeng, a lot of good stuff for me to learn from when doing regs.

A couple of pin-related features which don't exist yet (I think) but which some of the O1 users got good utility from are:

  • Changing pin attributes by function. In O1 these were like rich aliases - not only could you define a different name for a pin but you could define different attributes that were coupled to that name/function. Depending on which function name you used to refer to a pin defined what functionality you got back - https://origen-sdk.org/origen/guides/models/pins/#Pin_Functions

Not saying that the addition of these should be coupled to this PR, but have a think about them to make sure that none of the decisions being laid down here are going to make such things un-necessarily difficult in future.

@info-rchitect
Copy link
Member

This looks great @coreyeng, a lot of good stuff for me to learn from when doing regs.
A couple of pin-related features which don't exist yet (I think) but which some of the O1 users got good utility from are:

Changing pin availability (and attributes) by package. So you define which sub-set of all pins exist in a package, and then the pin availability changes when the package changes. See https://origen-sdk.org/origen/guides/models/pins/#Pin_Availability_by_Package

Changing pin attributes by function. In O1 these were like rich aliases - not only could you define a different name for a pin but you could define different attributes that were coupled to that name/function. Depending on which function name you used to refer to a pin defined what functionality you got back - https://origen-sdk.org/origen/guides/models/pins/#Pin_Functions

Not saying that the addition of these should be coupled to this PR, but have a think about them to make sure that none of the decisions being laid down here are going to make such things un-necessarily difficult in future.

@ginty Absolutely true, all of these O1 features would be a must have for all use-cases I have run into over the last 10 years. I do not think they should be coupled to this PR and think we can merge.

@coreyeng
Copy link
Member Author

Yes, I'll continue to add more pin features from O1. I still have some open items from #34 and I think I can get general metadata going, we can use that as a stop-gap then extract whatever ends up being useful there into their own thing. I expect pin roles, types, and functions to all be included at some point.

I thought from our last meeting we didn't know exactly what package support would look like. I don't use it personally, but wouldn't mind putting something together if its a regularly used.

@info-rchitect
Copy link
Member

@coreyeng I will start a new issue regarding package modeling and provide a lot of scenarios and how they can impact pin visibility and pin attributes.

@coreyeng
Copy link
Member Author

@info-rchitect, If what O1 has is sufficient, I can look into just moving that over here. I don't want you to have to add a bunch of details that's already present in the O1 docs. Its just not something I use. If there are changes we'd like to make a long the way, we can open an issue to discuss.

@info-rchitect
Copy link
Member

@coreyeng for sure start with O1 docs but there are many new needs wrt pin modeling such as multi chip modules where we need to figure out how Origen will communicate to identical pins on different chips. Another simple example is introducing things like two pins at wafer sort being aliased to a single pin with a new name for a particular package.

@info-rchitect info-rchitect merged commit b51ccbf into master Dec 31, 2019
@coreyeng coreyeng mentioned this pull request Dec 31, 2019
coreyeng pushed a commit that referenced this pull request Dec 31, 2019
coreyeng added a commit that referenced this pull request Dec 31, 2019
Merge pull request #21 from Origen-SDK/pins
@coreyeng coreyeng deleted the pins branch December 31, 2019 14:09
@coreyeng coreyeng mentioned this pull request Jan 2, 2020
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.

3 participants