Pin makeover

[coreyeng]

Hello,

This is the "Pin Makeover" PR, geared towards resolving the pin API on the DUT with the PinCollection-based pin API introduced by #121.

Pins was the first major piece I did in Rust and, obviously, my Rust skills were very elementary. The current way that the Pins interact over the PyAPI-> Origen boundary is pretty convoluted and riddled with limitations. I say "limitations" over "bugs" because I knew the issues where present, but didn't really know how to best fix at the time. These are things like, only handling 32-bit data, only supporting pins on the dut (many places were hard-coded to model 0, I mean), and requiring a mutable DUT in order to do much of anything, even though this isn't really required (at the time I didn't see a problem with needing a mutable DUT over a non-mutable one).

To review what #121 introduced:

• PinCollection: a struct which stores references to actual Pins and requires an (immutable) DUT lock to instantiate. All of the driver methods, plus some other utility stuff is here. Should follow the same idea as BitCollections.
• PinStore: Stores just pin IDs, without needing to any lifecycle considerations. Given the DUT, can be cast to a PinCollection to access driver functions like, drive_high. The only danger here is the same one that would plague everything else: if the DUT and whatever is holding the PinStore gets out of sync, the IDs will become invalid.
• PinGroup: pretty much just a named PinStore, stored on the DUT.

Anyway, concerning the Rust API from #121, there's really not much to be said. This is primarily a rewrite of the PyAPI <-> Origen interface to get a single means of handling pins and pin operations.

I'm sure they'll be some other smaller items coming in future PRs (I already see a few minor things), but this is the big one, sort of speak. This the main re-arch with lots of changes whereas future items should be much smaller in scope. To avoid massive PRs like we've had, I think this one is in a good place to start review.

@ginty, I have a few specific items I'd like your feedback on:

• I removed pins holding any kind of data. This is likely to return in some form when I get to adding analog pins, but, at least for digital stuff, I don't see why pins need data anymore when the symbol should contain the data by definition. For example, there is no more generic drive option, only a drive 1 or drive 0.
  You had mentioned wanting data for driving a clock or something like that, like we had in O1, but isn't this kind of a contradiction to your previous requests? Wouldn't the correct way to implement this be to remap 1 to a clock pulse in the timeset, and 0 to just flat-lining it for any pins that act as a clock? Or even splitting it up further for different clock frequencies?
  I can see the want to add some shorthand/helper methods for this, and I agree. I can look at that, especially as I get closer to starting OrigenSim stuff, but I'd think that should be just helper stuff and there shouldn't be any internal dependency on pin data just to start/stop a clock. If we're in agreement here, I can start to look at such shorthand/helper functions.
• PinAction is still around, but in a form that I hope is closer to your liking. I still believe thinking in terms of pins, especially when writing drivers, is convenient. If I'm recalling correctly, I think your biggest complaint about the previous implementation was it forced you to always think of pins in these terms and that, by the time you got to the timeset, you were still dealing with PinActions, and since this was an enum, there wasn't really anyway way around it.
  What I've done this time around is undid the enum-like behavior and replaced it with just a wrapper around a String. I think this gives us a good middle ground. Drivers and the PyAPI pin stuff can still think in "PinAction" terms, which, again, I think is the most convenient and straightforward, but now, at any point, and can essentially say screw your PinActions and just give me the symbol. With it now just being a wrapper around a String, there should be no hoops to jump through to get this, and I have functions to do this.
  This could have just as easily been written as a bunch of public functions in the Pins or a Utility module somewhere that takes a String representing a tester symbol, but I think a struct is the best, at least for organizational and clarity (in the drivers and PyAPI pins, specifically), and I hope that the ability to "unwrap" it to just the raw symbol as safely and simply as it is should give us a solid compromise.
  The Timeset stuff still uses PinActions, but this is just because it works. I expect to revisit this when I return to some timeset features that are still planned. But, the existing PinAction usage is on-par with #121, and even master prior to #121, and this PR will enable further clean up here in the future (even if the cleanup isn't actually present yet).

Thanks!

[coreyeng]

Hello,

Providing some requested details here to trace through how a "pin action" goes from the user API to the final rendering in the pattern. The Rust API is similar, it just obviously jumps the PyAPI portion.

The general sequences for, say, driving a pin to 0, will go as follows:

1. Starting with dut.pins["reset"].drive(0) (just running in the python_app), will hit the function on the PyAPI side. This goes through some basic "get the pin", then calls update. During this, I morph the data into a transaction, which I've extended quite a bit for this, that provides a struct to handle width, masking, etc.
2. This hits the method-of-the-same-name in the PinGroup on the Rust side, which cast itself to a PinCollection and updates the pins based on the transaction details.
3. There's a couple layered methods here that I provided specifically in case drivers need to post-process nodes prior to adding to the AST. But, the general method jumps to this which boils down to writing the given PinActions (as strings) into the AST.
   At this point, the AST will look like this:
   
   In retrospect, this is probably overkill, but if a PinGroup is given, I put that into the AST along with the actual Pins. The second element is the PinAction, now represented as a plain String. This is whatever the character given happens to be.
   This will go on under the rendering phase. At this point, everything is the "dummy timing". 0s, 1, Hs, Ls, etc, but you can also throw in any character you want:
   
   The first element is the ID of the pin group or the pin. The None, is a Metadata table that's a placeholder for future stuff. I'm thinking I'll need this when I get into PinRoles, analog pins, etc.
   The "dummy stuff" is located here. This is the "basic", "dummy", "alias", "common", or whatever else you want to call it. Its just a place for it to live so that its universally available.
4. The timing so far hasn't been involved, and I don't think it should be. In the end, this is just throwing symbols into the AST, and as we've said, a record of what its doing. To me, the timeset will interpret what the AST wants to do, into the final rendered pattern. With supporting multiple testers, I don't see how this can be done any other way outside of having multiple ASTs.
   But, once rendering commences, the timeset owns the show. At least for what I have in the vector-based base trait, this will instantiate a StateTracker, which also assists with compression, pattern headers, etc, but also asks the current timeset what it should actually do.
   As an aside, the Timeset was already written in terms of PinActions and it was easier to keep it that way for now. I'm not saying it should stay that way, but I'd rather this gets done in more bit-size chunks so I drew the current line there. But, if we agree that the PinAction is a benign struct that just carries around a String with convenient functions, then I'd also look at putting this into the AST instead of essentially 'tearing it down to build it back up'.
   
5. Each timeset will have a Resolver for each target that's instantiated. Each can start with a default (which, not-coincidentally, looks like the V93K) but can then be edited as needed.
   This is ultimately what will be put in the rendered pattern.

Hope this helps demystify what's going on behind the scenes!

[ginty]

Hi @coreyeng, thanks so much for that last comment. Such detail is not always required, but it has really helped to understand the big picture in this case, very much appreciate you taking the time to do that.

1) Looks good. The only thing I'm missing here is where is the API method to set an atrbirary string value on a pin group - i.e. I can see the convenience methods for the common drive/verify/etc. actions, but what about dut.pins["reset"].set_wave_char("Y")?

2 & 3 and the AST looks good.

4) Yes, we are in full agreement here.

5) This looks fine as way of re-mapping the standard action characters to handle ATE differences, and I think the example you have here as a capture being implemented as a 'C' on V93K and an "X" + stv on Teradyne is probably one of the main use cases for this. I think we also need another level of re-mapping by pin and tester. The canonical example of this is where it is quite common on ADV to use a 'P' on clock pins which would just be a '1' on Teradyne.

I think this is not any big deal, just a hashmap that can be updated by an application and which can specify the intention that anytime you see a '1' on PinX when generating for testerY, then substitute it for a 'P' . That can then be applied easily in a tester-specific AST processing pass.

Don't think we need to add that before going ahead here, but please keep this in mind in your future work.

Also as I think the capture action should throw some indication of it in the AST, just a simple capture node should be fine. That would be used by the Teradynes to inject a 'stv' or whatever on the relavent vector.

[coreyeng]

Thanks for the feedback!

For item 1, is the example with the "A" not what you're looking for here?

I still called it "actions", but it accepts arbitrary string values. I purposefully tried to stay away from using "char" since I'm allowing for multichar strings per group (see the Python specs for how this looks (also here for the pin action class itself using multi-char symbols)

For item 5, I wanted to do this by looking into the timeset's waveforms and plucking out anything from there. I haven't gotten to this point yet. There's still waveform inheritance and block usage in the STIL spec that's not implemented but I was waiting for the dust to settle a bit more before going back to that. Just for transparency - that's not stuff I'll use much (I'm much concerned with pattern generation and app development in my day-to-day) - not that I don't want to do it, but was going to work on that in parallel with some more pattern-oriented stuff (as I've already been doing :p)

I agree with needing a capture node as well. Currently though, it does work in J750 with this and I've got an example pattern, so I've not worried much about this yet. This works because it uses the "aliased/common/what-have-you" actions, so its looking for a "C", not a "X", as this is prior to the timeset resolution. As long as our "common" actions to step on each other, this should be fine.

edit - "fine" as in generate correctly. Not that we shouldn't add capture nodes.

[ginty]

Thanks, yeah I could see the 'A' but not the API to get it there.

Could I request that we make the Python (user-facing) API <pin collection>.set_char() instead of set_actions(), at least as an alias and that's the one we would document in the guides?

I feel my mental map of this wants to coverge on actions referring to our common use case methods (drive, verify, dont_care, etc) and those then become just a dumb wave chars the closer they get to the final pattern rendering. I don't think that's what we quite have yet, but it's a possible evolutionary path from here and making that method called set_char signifies that it's bypassing that action API. Plus (and the main reason) is that wave char is the more commonly understood term for this, rather than making up our own term (action), so I think it will be clearer to users.

Thanks!

[coreyeng]

I'm not 100% following "bypassing that action API". So would you want "set_char" to even bypass timeset look up? For example, if a timeset maps "A" to "B", set_actions("A") would end up with B in the rendered pattern. Would you want set_chars("A") to place A in the rendered pattern? I'd think it too should go through the timeset and get B in the rendered output.

I've got no problem adding set_chars as an alias to "set_actions", but that would still get resolved by the timeset.

[ginty]

Yeah, sorry I find the language around this difficult.

Here is an attempt to outline how I would like to organize and think about this stuff...

I don't really see it as the job of the timeset to re-map characters, that's something else. I envisage the timing being "the DUT timing" rather than there being anything tester specific about it. So it would be basically like a pattern where you express it once in Origen and then it can be rendered to different ATE formats.

Many (most?) times, people will generate patterns without defining timing at all.

Re-mapping mainly exists for the cases where timing for a given tester exists outside of Origen and the generated patterns need a little bit of fudging to align with the external conventions. Generally speaking, if timing is defined in Origen and Origen generates both the pattern and the ATE timing files, then no re-mapping should be required at all.

I think it is maybe going down a wrong/confusing path to implement the X/C difference for capture as a re-mapping. I don't really see that as a re-mapping and more like a tester-specific rendering. I think what should really happen in that case is that we have a dedicated capture node in the AST, which optionally contains a list of pins, and then in late stage processing a Teradyne processor would force the char to X in that case while V93K would use a C.

Both tester-specific rendering and re-mapping should happen very late in the process, so the initial AST should reflect the character generated by the application code and re-mapping would occur in later AST processing.

Here are some walkthoughs of how some typical cases would be handled...

Basic Example

pin("clk").drive(1)

Would put this in the AST:

PinAction(<pin_id>, "1", Some(1)) # <pin_id>, char, data value

The above is not subject to any-remapping, the drive method itself implements the convention that a drive 1 corresponds to the character "1".

Using the set char API is just a more direct way of pushing a value in to the AST:

pin("clk").set_char("P")

Would put this in the AST:

PinAction(<pin_id>, "P", None) # <pin_id>, char, data value

Capture Example

Capture is a special case I think, it is not strictly about a conventional character re-mapping as it also has a vector-level and tester instrumentation component to it.

So I think by default our capture method should really do this:

pin("tdo").capture()

AST:

PinCapture(Some([<pin_id>]))

i.e. I don't think it really needs to have a character component to it, the Teradyne and ADV renderers can later implement the X/C convention.

If an application requires something more elaborate - e.g. specify a specific (non-std) timing waveform for the capture, then they could do:

pin("tdo").capture()
pin("tdo").set_char("Y")

and we could add the convenience API:

pin("tdo").capture("Y")

In both cases, creating the AST entries:

`PinCapture(Some([<pin_id>]))`
`PinAction(<pin_id>, "Y", None)          # <pin_id>, char, data value`

A Teradyne renderer would say "well, I see you've specified both a capture and a char on this vector, but as that makes no sense on this platform I'm going to force the char to an 'X' anyway". Whereas the ADV renderer would allow and implement it as requested.

There may also be tester-specific APIs around capture, those will be implemented via the tester specific blocks that I showed recently for prog gen.

Re-mapping Example

So for re-mapping, I think it should be really simple. Something like this on the app side populates some lookup table in Rust-land:

pin("clk").remap("1", "P", "v93Ksmt7")     # Re-map a 1 to a 'P' on the clk pin when generating for V93K SMT7

Late in the day once the AST has gone down the V93K path, a processor will run to substitute any "1"s on the clk pin to "P".

Note, if it wasn't clear before, re-mapping like this does not mean changing timing waves, it is just an ASCII re-mapping to align to the conventions imposed by an external timing definition.

Origen Sim

Here you would have to have a timing model defined in Origen (or use a default that Origen will provide).

At the start of the simulation Origen will download the timing to the improved O2 simulation pin drivers. These will implement the timing waves in RTL.

The <pin>.drive(1) and similar methods should be implemented as calling "set_char" internally, i.e. that should be the single point that generates AST entries rather than each method doing its own. When simulation is enabled the "set_char" method would be one of the main intercept points and it would just directly download to the appropriate pin driver to say "this is your new character".

When cycle is called and/or time is otherwise advanced, the pin drivers would each apply the waveforms corresponding to the current character.

No-remapping should be involved here, it is the app's responsibility to specify characters that correspond to its timing (or to specify a timing that corresponds to the characters that it uses if you prefer to think of it that way round).

In Closing

Sorry, that may have ended up being a long way of saying that maybe we want to make a few tweaks here after all.

My feeling is that I would like to see this frontend part of the pin API being really simple, it is just about stuffing things into the AST and capturing re-map requests somewhere. I think these latest changes are moving closer in that direction, but maybe it can go further.

Ultimately, there should be zero re-mapping, peaking into timing, or anything like that on the frontend and the AST just corresponds directly to the methods that were called.

[coreyeng]

Thanks for the response! I understand what you're saying and was the direction I was planning to go anyway, outside of relying on the tester remapping.

As far as stuffing symbols into the AST, that is as you described above. I read your previous comments as wanting to bypass the remapping, which would be a different node type (or option, I guess) to signal that the node was immune to remapping. That doesn't sound like what you want, so just a straight alias method in a future PR should get the nomenclature you're looking for. The remapping requests I think is fine, its just only available at the timeset-level currently (but doesn't have to be used). Actually, the Teradyne remapping was more to ensure it worked than to be the proper way to handle it. The "capture" is already parsed as you want I think (that is, look for "common" waveform character of "C" to denote its a capture and handle it appropriately. Could just as easily hard-code an X here instead of remapping, though remapping offers the, probably superfluous freedom to change that character on the Python side). Remapping per pin is planned, but Timeset needs a bit more work before that'll be available.

However, I'm now second guessing having a capture node. Isn't this getting back to what I had before with enumerated pin actions? Why do we draw the line at capturing? Seems that following that logic, we could also put a node to drive high, drive low, etc.... and those enumerated values is what I had before. Recall that I still did allow for arbitrary wave chars to be added in that scheme as well, it was just of a PinAction::Other type, instead of a PinAction::Capture, for example. I thought you wanted to get away from enumerating was a pin could do and keeping it straight to STIL-like symbols. Seems we're doing a 180 on that.

[ginty]

Yeah, I thought about sniffing the character for "C" to mean capture, but it seemed a bit 'dirty' to me.

I think capture is quite unique and distinctly different from drive/compare/most (all?) other pin states, so it is OK to handle differently from my perspective - depending on the ATE it has vector-level and instrumentation concerns and is not just about defining a wave form (like drive and compare is for example). I see it as a close relative of overlay which is also handled differently rather than just a straight character mapping.

On the V93K it should be perfectly legal to capture on a wave character other than "C", so for that reason I do think it is worth having a capture node + character, and that would also make life easier for Teradyne.

[coreyeng]

I think you could conceivably make that argument for any of the symbols though, not just capture. You could always write test method, or some weird platform comes around, that HighZ's differently, for example. Also, like you've said with a timing that "drives half a cycle and highz's half a cycle", for example, I'd think you could also have something that captures for only half a cycle. I don't really know when this would be used, but I don't see why you couldn't. I don't like the idea of allowing capture to have some dedicated behavior but not supporting it for the other actions.

Regarding capturing on non-C, that would be the timeset that remaps it, but the render itself has access to both the pre-and-post re-mapped symbols. Sniffing a C for capture should always be safe and the remapping occurs afterwards.

Overlays I was planning to handle with metadata either on the PinAction itself, or on the Cycle node. But I think that's a different enough since that's not a "action". By that, I mean, the pin will still do whatever it was going to do regardless of its being overlayed or not. I was not planning to implement overlaying as an action.

[coreyeng]

Also, I undid the formatting. Its gonna be hell to merge that with all the name changes I have. I'll run format once this gets settled.

[ginty]

I'd think you could also have something that captures for only half a cycle. I don't really know when this would be used, but I don't see why you couldn't.

yes you could, that's why I said it would be (fully) defined by a capture node + a character, such an operation might be requested by a given application saying pin.set_char("Y").

To render this properly to a pattern you will need to know if its a capture or not, but that's not generally true for other operations like drive/expect/don't care/whatever - in those case you can just render the character and not really care what it does. Capture is definitely different to most other pin states.

To handle this capture case properly either you need to ask the timing model if "Y" contains any capture time, or you ask the user/app to give more info in this case, i.e.:

tester.capture()
pin.set_char("Y")

I prefer the latter, but the former would be OK too.

[ginty]

Also, I undid the formatting. Its gonna be hell to merge that with all the name changes I have. I'll run format once this gets settled.

Should not have been a big deal, all you had to do was run fmt on your branch and the diff/merge would have made sense.

[coreyeng]

How would pin.set_char("Y") know that its a capture though? That just looks like a setting an arbitrary value to me. I'd rather attach some metadata or something to it to indicate a capture. But then, I'm not sure how we'd expect to handle something like "capture half a cycle" in a generic way. It seems like it would need circumvent the render's default capture handling and just place the symbol.

but that's not generally true for other operations

I think this is mostly what I have a problem with. It seems like if we're going to go out of our way to make capture node, we should do so for all the "standard" actions. I'd think we'd want consistency here and even though it may never come up (like the capture half a cycle thing), I'd still rather make it all look the same. But, this gets back to what I had before with enumerate states.

Should not have been a big deal, all you had to do was run fmt on your branch and the diff/merge would have made sense.

In theory, yes, but in practice that's almost never gone according to plan when I'm moving lots of stuff around, like I am here. It tends to smash function prototypes together and makes a mess.

[ginty]

How would pin.set_char("Y") know that its a capture though?

Well yes, that's the crux of it. In such a case the externally/3rd-party defined timing file (for the V93K) for this application would be telling the ATE that it is a capture. In fact, such an application would work fine just by Origen rendering the "Y", but they would have trouble if they tried to generate an equivalent pattern for the Uflex as Origen would not know to insert the stv or whatever. So to be able to re-target properly we need more data on the Origen side to indicate that this is a capture.

but that's not generally true for other operations

I think this is mostly what I have a problem with. It seems like if we're going to go out of our way to make capture node, we should do so for all the "standard" actions. I'd think we'd want consistency here and even though it may never come up (like the capture half a cycle thing), I'd still rather make it all look the same. But, this gets back to what I had before with enumerate states.

You know what, the problem here is because ADV have made things muddy by treating capture like a timing attribute, I think Teradyne do it much better by treating the intention to capture on a vector as a clearly separate concern from the waveform timing. Basically the timing defines when to read and a separate flag states capture on/off. So I've just had a look at the STIL section 18.2, waveform event definitions (upon which Origen's timing model will be based), and it doesn't have capture as a possible wave attribute either - though of course is does have drive, expect, don't-care, high-Z, etc.

So Origen is aligned with Teradyne/STIL by treating capture specially and not like a std waveform timing attribute, hopefully that will make you feel better about it.

Should not have been a big deal, all you had to do was run fmt on your branch and the diff/merge would have made sense.

In theory, yes, but in practice that's almost never gone according to plan when I'm moving lots of stuff around, like I am here. It tends to smash function prototypes together and makes a mess.

Yeah Ok, but you're pushing similar merge issues onto me when I try to merge the now un-formatted master into my formatted branches. I'm not sure what's going to happen now that you've reverted a commit which I've already included in a merge on the prog_gen branch, but I guess that will come out in the wash in due course. Anyway, lesson learned is let's not have unformatted code merged into master in future.

[coreyeng]

You know what, the problem here is because ADV have made things muddy by treating capture like a timing attribute,

This very well I think is the problem. I'm looking at capture as any other "pin action", which I suppose the V93K has oversimplified for me.

So Origen is aligned with Teradyne/STIL by treating capture specially and not like a std waveform timing attribute, hopefully that will make you feel better about it.

Yes, it does actually. I have been reference STIL as its been brought up but didn't notice the absence of a capture wave char. Actually, this does it for me. I can see this as being kind of a "meta" thing, like overlays, where the V93K just happens to make run-of-the-mill vector out of it.

Yeah Ok, but you're pushing similar merge issues onto me when I try to merge the now un-formatted master into my formatted branches.

Shouldn't. I reverted it, so the history is there. Unless you made changes to those files between your format and my revert, its should go back to as it was before.

Anyway, lesson learned is let's not have unformatted code merged into master in future.

Agree, but I don't think mid-PR is the right time to apply that! Its much easier to revert now and reformat after merge. Assuming...

Okay, I'll stop thinking of capture in the same terms as a drive high/low, etc., which thinking about the J750, I can see that. I still think this is a step in the right direction. My thoughts now are: let's merge this and I'll take the action to add capture nodes and update the J750, V93K, and test cases accordingly. I don't see how adding this would alter much of what's been done here. This PR was mostly to get the PyAPI and "Rust API" to agree. This same issue was present prior to this PR, just in a slightly different form (everything was enumerated, instead of nothing).

[ginty]

Sounds good to me.

Btw, I have since been able to push an update to the prog_gen branch without it complaining that I've diverged or anything (I think before you reverted the revert), so I think you can probably re-revert it (!) if that does make it easier for you. Then we can fmt master and I can re-merge that before merging prog_gen.

[coreyeng]

My understanding is the revert checks in a patch that undoes a previous commits. I think since its the "history" is preserved, merging future goes as before. I don't think there's any "magic" to it and it should be transparent to you.

My problem with format after moving stuff around is Git is prone to getting confused and re-adding functions I've deleted, adding two method headers, etc. It was just easiest to revert the format and should be the same net result. Which is also akin to reverting the revert, then merging this on top of it.

I just ran the format here. I'll wait for the specs to pass to be sure, but end result should be this PR + "origen fmt".

[coreyeng]

Travis is being excessively slow today. The push passed though so this should be fine.