Origen-SDK / o2

MIT License
4 stars 0 forks source link

Tester character mapping #106

Closed pderouen closed 3 years ago

pderouen commented 4 years ago

A simple way to allow the Renderer to determine the PinActions to char mapping.

I initially started down the path of giving StateTracker a way to call back into the TesterAPI, that was a bit of a mind pretzel for me. I also looked into passing StateTracker a closure that does the mapping. This led to several (endless) thread safety compile errors. In the end I think I prefer this implementation. It's simple and it has a feel similar to a yield in Ruby.

Have a look and let me know what you think.

ginty commented 4 years ago

Hi @pderouen, Thanks for the PR, it looks good as far as it goes, however as with almost all changes at this stage in the game it makes us think about related things and can easily take us off on tangents. So as discussed offline, I think these comments are not applicable to this PR specifically, but they are related, and this is as good a place as any to discuss them...

So when looking at this I'm a bit worried that state character handling is not headed in the right direction and is overly influenced by O1, which in turn was overly influenced by Teradyne.

On V93K and STIL (which is a good indication that this is how most other ATEs work), any character can be assigned to a pin on a given vector and this corresponds to a particular waveform defined by the current timing. For the simple drive(0/1) and read(0/1) cases it is conventional to use the characters 0/1/L/H but it is not necessary, and in fact another common convention is to use 'P' to mean drive 1 on a pin with clk waveform (a pulse).

The key point is that 0/1/L/H have no special meaning at all to the V93K and so character mapping being done at the ATE level does not make sense, I think it should be done by the application but with some syntax sugar for the common conventional cases.

It is a real goal of O2 that it can handle all the cases, not just the simple ones, and I think having pin actions defined like Drive, Verify, Capture, Don't Care are an oversimplification. Such an approach is fine for typical patterns and can get you very far (O1 being a testament to that) but ultimately it leaves you with no chance of handling the subset of all patterns which require more nuance. So when I look at the STIL spec I see that it defines 21 different pin events, and a character can map to a waveform that defines any combination of them. So you could have a pin which is driving on one half of the cycle and comparing on the other half and then how do we map that to something like the current system which has Drive and Compare as separate states?

So the approach I would like to see is something along the lines of the following:

An individual pin's notion of whether it is driving/comparing/etc should go away and instead it only really stores three pieces of information:

The baseline application API would be:

dut.pin("myclk").set_char('P')
dut.pin("pinA").set_char('0')
dut.pin("pinB").set_char('H')
dut.pin("pinB").set_char('L', 0)   # With optional data value
dut.pin("pinA").set_char('1', 1)
dut.pin("pinC").start_capture
dut.pin("pinC").stop_capture

We should have helpers for the common cases of driving and comparing data:

dut.pin("pinB").drive(1)
# Equivalent to:
dut.pin("pinB").set_char('1', 1)

dut.pin("pinB").verify(0)
# Equivalent to:
dut.pin("pinB").set_char('L', 0)

The above APIs should all be available on pin groups too and which would obviously map to the contained pins.

Finally, we should also give the application a way to define character mappings if they need to handle multiple ATE targets, for example a 'P' on a clk pin should be converted to a '1' when generating for a Teradyne target. Origen could also implement some common cases like that to save the user the trouble, but ultimately they could override via an API something like this:

# Re-map 'P' chars to '1' for Uflex
tester.character_map["myclk"]["P"] = "1"

For OrigenSim, I would really like to overhaul that too for O2 and make the timing handled by the pin drivers in Verilog. That should improve performance and also make it more capable of handling all potential waveforms. So on a timeset change we would download all the waveforms to the drivers (i.e. all the info about the wave to apply for each character), then it would be a simple case of just sending the current wave selection to the driver whenever a pin action occurs.

For the pin action nodes in the AST, I think they should be pretty simple:

PinStartCapture(pin_id)
PinStopCapture(pin_id)
PinSetChar(pin_id, wave_char, Option<data>)

@coreyeng, what's your thoughts on this? Sorry for not picking this up earlier but it is hard to pick out all the implications from a big PR and smaller changes like this which actually start to use infrastructure really help to make things clearer.

How much effort are we talking to change direction here do you think?

Hopefully it may simplify a lot of things, because it kind of removes the need for Origen to do any character mapping except for the application-defined ATE re-mappings, but that should be easy.

pderouen commented 4 years ago

On another only slightly tangential topic (capture mentioned above). There also needs to be a way to designate pins to drive overlay data and mark when the data should be updated. I had been planning to make that a node type that gets pushed to the AST, with the ATE Renderers turning it into either digsrc, label overlay, or subroutine overlay.

That was the next piece I had planned to attack after sorting out character mappings. On that topic, I'm not opposed to the proposed update. It would be good if we can provide default timing and character mappings to simplify things for IGXL apps. I typically don't define any Origen timing in my apps unless I'm compiling a sim snapshot.

ginty commented 4 years ago

@pderouen, yeah overlay should be handled separately, like capture, and probably you are best placed to make a proposal here. In this case I see Teradyne actually being the superset and V93K being the subset (at least in the way I use it) with overlay being more of a vector-level thing. I also think we want dedicated AST nodes to represent overlays as you suggest.

It would be good if we can provide default timing and character mappings to simplify things for IGXL apps. I typically don't define any Origen timing in my apps unless I'm compiling a sim snapshot.

Definitely agree on that. I think if you don't care about more complex waves then you would just naturally write your app with the pin.drive(1), pin.verify(1) APIs which would produce the common 1/0/L/H chars which would work on Teradyne. I didn't mention it above, but we should also keep pin.dont_care which would be equivalent to pin.set_char('x') which would naturally work on Teradyne and would be a sane default for V93K.

Things like the JTAG driver should also use the Teradyne friendly default chars, but give the user the option to configure for different chars if they want to like we talked about.

coreyeng commented 4 years ago

Hi @ginty,

The way I did pins and timesets was heavily influenced by O1 - I won't deny it. Unfortunately, I only have experience with IGXL and simplified V93K stuff. I know more advanced stuff is out there, but I have no idea how or when to use it, let alone apply it to something like Origen. I also have no clue how other platforms outside of those two work and not much of an idea on even the differences between Smartest 7 vs. Smartest 8 and the capabilities therein. Anyway, there's my reasoning for what I did,for what its worth. 😛

It will take some work to redo the pin modeling. Possible, but won't be a quick fix. Your argument though makes it clear that its the right the right thing to do, especially since its still in the early stages and will just get more and more difficult to transition the longer we wait. Off-topic, I also wanted to add interior mutability to pins and timesets, so may be a good opportunity for that too.

I'm making good progress on the docs - I got frustrated integrating some javascript stuff and when Sunday rolled up I didn't have the motivation so I finished up #107 , which was 80% done from March anyway. @pderouen, not sure if you're interested on taking on @ginty's proposal. I agree with it though and I can make it the next item up for me after the doc system gets rolled out.

pderouen commented 4 years ago

@pderouen, not sure if you're interested on taking on @ginty's proposal.

Let's talk about it in the meeting tomorrow. I'm not opposed to doing it. But, I suspect it will be a long slog for me. My initial thought is that if it's going to be a while before anybody else would get to it, then I'd be willing to take a run at it.

ginty commented 4 years ago

@pderouen, I'm wondering if capture and overlay should be more tester-level APIs rather than pin-level. e.g. tester.start_capture(dut.pin("pinC")) rather than dut.pin("pinC").start_capture

Ultimately, resolving to AST nodes like this:

Capture([pin_id, pin_id, pin_id])

Overlay(String, [pin_id, pin_id, pin_id])
pderouen commented 4 years ago

Yeah, the tester and pins are inter-related when it comes to capture and overlay. Historically in o1 it has been done by telling the tester which pins to overlay/capture. I didn't intend to suggest that the pins should handle the request. But, now that I'm thinking about it, the only advantage I see in going that route would be a slightly less verbose syntax to request it:

dut.pin(:blah).capture

instead of

tester.capture dut.pin(:blah)

I don't think it makes much difference either way. I definitely agree that in the end the request gets pushed to the AST for the Renderer to process as appropriate.

ginty commented 4 years ago

I think the advantage of going the tester route is that it doesn't require a pin reference where applicable. e.g. a subroutine type of overlay and an STV on the J750 both don't require a pin arg.

pderouen commented 4 years ago

For overlay and capture, if you want the option to Render for subr or dig src/cap, you need the drivers to specify the pin(s). The Renderer will have to decide later if whole vectors get removed or instrument op codes inserted with special pin characters.

In any case, I'm not advocating to move the handling to the pins.

pderouen commented 4 years ago

Based on all of the discussions, the changes in this PR are no longer needed. The Renderer can handle updating pins to use special capture or source characters by pass through the AST looking for capture/overlay nodes (or characters requested for remap) and modifying the pin nodes with the correct characters for the next pass through the AST (like the other preprocessors).

We can close this PR whenever we're done with the conversation.

ginty commented 3 years ago

Guess we can close this now, superceded by #121