search

Origen-SDK / o2

MIT License
4 stars 0 forks source link

Timing updates #121

Closed coreyeng closed 3 years ago

coreyeng commented 3 years ago

Hi,

This isn't quite "merge quality" yet so I'm trying out the Git draft PR thing. I'm working on getting this in better shape, but we can start talking about what this PR was supposed to be. I ended up trialing some of the updates and I'm actually glad I did as I found them insufficient in some places (as opposed to just a theoretical view). At this point though, it'll be much more work on remove the superfluous bits than to just start reviewing and leave some development stuff in there.

I'll be writing some docs for the pins and timing, but in the meantime I have the specs for timing and pin updates. The main update here is that you assign and "drive" any arbitrary states I have some default which should be sufficient for the V93K and J750, but the Uflex can update as shown here and we can tie in some booting for this so that the target can automatically its pin states. I haven't gotten to that yet though. Will do that soon.

As you'll obviously see, I got distracted AF while working on this. I trialed the actual vector generation, that went well, so I added SWD, that went well, so I added Arm Debug, that went poorly and ended up messing around with the registers and bit collections a bit (nothing major), but also updated the PyO3 version to 0.11+ (@ginty, a note on this - BigUint doesn't seem supported anymore so I accepted the input from Python as a u128 and convert to a BigUint. I meant to open an issue on Pyo3 for this but wanted to look into it a bit more first - haven't gotten to that though).

A couple more points though:

Right now, I have both macros and functions floating around on the Rust side. I got it in my head that I should be using macros but after doing some research, found that that was the wrong path and am in the process of fixing.

There's also a lot of build warnings currently, which I'm also looking at. Most of this are on purpose though so I don't forget to either add support or make a note to myself (like overlays, which I added placeholders for but haven't implemented yet).

Anyway, look at this much or as little as you want. Some of this will be subject to change, but we can start to review some of the more polished spots and you can see the direction I'm going with Arm Debug.

ginty commented 3 years ago

Hi @coreyeng,

Good to see this starting to firm up, you've obviously done a lot of work on it!

As far as the timing goes, upon an initial look at this I'm a bit concerned that it doesn't implement the approach we previously discussed and which is captured in this comment thread - https://github.com/Origen-SDK/o2/pull/106#issuecomment-630733303

The desire was to move away from static actions in the AST (and pin model) like 'drive 1' and 'expect 0' and instead move to dumb characters instead - '1', 'L', etc. which don't carry any semantic meaning (except perhaps by convention) about what kind of pin action they resolve to.

Many operations, e.g. rendering a vector pattern, won't care what these characters actually mean and can simply use them verbatim. For operations that do care about what a waveform character actually means would obtain that information by looking up the current timeset.

It looks like this PR is still doing much the same as it originally did and is storing things like PinActions::VerifyHigh in the AST rather than something like SetPinChar(<pin_id>, 'H').

Really what I expected to see here was the drivers doing things like:

dut.pin("tdi").set_char(tdi_drive0_char);
dut.pin("tdi").set_char(tdi_drive1_char);
dut.pin("tdo").set_char(tdo_expect0_char);
dut.pin("tdo").set_char(tdo_dont_care_char);

Where tdi_drive0_char would default to '0' but would provide various ways for the app to override that - e.g. when instantiating the driver, via a global re-mapping, etc.

coreyeng commented 3 years ago

I did keep the top-most interface very similar to what it was before. The addition is that arbitrary symbols are now allowed, which although still gives you a PinAction in the AST, can correspond to anything. It'll be whatever the set_action told it to be. I did it this way to support multiple testers from the same AST, as if we do something like dut.pin("tdi").set_char(tdi_drive0_char) which may default to 0 on one platform, could default to something else on another. The AST becomes ambiguous per tester then as it just sees a 0 without any context into what that means. Instead, I set it up as putting what the desired action is into the AST then letting the tester render, in conjunction with the timeset at that time, determine what character ultimately gets into the output. Even though the PinAction in the AST could be arbitrary, that arbitrary action should have an entry in the timeset and, if not, its up to the render to figure out what to about it (ignore it? fail the rendering? idk).

I also think this helps keep the interface simple if you don't need this kind of flexibility. Based on based personal experience, most patterns would still like to just use dut.pin.drive(0) and expect to do a DriveLow operation - whatever that actually entails is up to the renderer to decide later, in conjunction with what the timeset tells it.

I did go a different direction with the API while implementing, but I think flexibility desired what added. In this any case you can see that couldn't be represented with this scheme?

ginty commented 3 years ago

Well the goal of this change was to ensure that at the lowest level we are representing pin states as references to timing-defined waveforms. That means that we will be able to handle any and all weird waveforms in OrigenSim, pattern conversions, etc.

I think what we have here now is similar to what we have in O1 where for the most part timing is a second class citizen, usually it doesn't even need to be defined at all, and (simple) waveforms are represented in the pin model (O1) and the AST (O2) via state concepts like 'Drive' and 'Compare'. You can have a go at hacking together something to handle the more complex cases by being able to assign an arbitrary character, but ultimately the end result is going to be hacky due to the inconsistency between how waveform information is represented.

What I really want to see here is the timeset taking full responsibility for what a wave character means in all cases, I expect this to make the low level representation much simpler and that is going to be a much better platform for building timing-aware components on top of. I don't think the user experience should be any different from what we have now, but the underlying representation needs to be much cleaner.

On some of your points:

I did it this way to support multiple testers from the same AST, as if we do something like dut.pin("tdi").set_char(tdi_drive0_char) which may default to 0 on one platform, could default to something else on another. The AST becomes ambiguous per tester then as it just sees a 0 without any context into what that means.

The value in the AST represents a baseline character, really the character that the author had in mind when writing their application for their first target platform. I don't expect that this would ever really deviate from the '0', '1', 'L', 'H' convention in the vast majority of cases. If that aligns with the character to be used on a given ATE then everything just works.

If they want to change it for a specific ATE then we should have a re-mapping API that can be invoked per tester, something like:

# Re-map '1' chars to 'P' for the clk pin on V93K
tester.v93k.character_map["myclk"]["1"] = "P"

So when you want to get a waveform for a given character the lookup path (internally managed by Origen) would be:

get_char (from AST) -> ATE re-map -> Current timeset -> Default Timing (if no timeset defined then provide sane defaults for 1/0/L/H/X)

This was all previously discussed in #106.

patterns would still like to just use dut.pin.drive(0) and expect to do a DriveLow operation - whatever that actually entails is up to the renderer to decide later, in conjunction with what the timeset tells it.

You would still get that per the previously discussed proposal. We would still have API methods like pin.drive(1) but these would be purely syntax sugar that would simply map to a set_char.

Fundamentally, the problem with the current direction here is that the low-level representation is too influenced by the high level API. I think we are not in any disagreement about the interface we want to present to the user, however under the hood I want to see an ultimately much simpler architecture which clearly has the timeset in charge of what a pin state actually means. From my experience writing OrigenSim and some V93K corner cases I think we need to get this right on O2 otherwise Origen will remain a good tool for simple timing use cases, but incapable of handling the more complex cases like scan.

coreyeng commented 3 years ago

Hopefully getting closer. Just as an update, here's what the AST looks like from this pattern:

Test("timeset_workout")
    PatternHeader
        Text("******************************************************************************************")
        Text("Generated")
        Text("  Time: 2020-09-23 23:33:14.024254300 -05:00")
        Text("  By: nxa13790")
        Text("  Command: origen generate \\\\?\\C:\\Users\\nxa13790\\Documents\\origen\\o2\\test_apps\\python_app\\example\\patterns\\timeset_workout.py")
        Text("******************************************************************************************")
        Text("Workspace")
        Text("  Environment")
        Text("    OS: Windows Unknown")
        Text("    Mode: development")
        Text("    Targets")
        Text("        C:\\Users\\nxa13790\\Documents\\origen\\o2\\test_apps\\python_app\\targets\\dut\\eagle.py")
        Text("        C:\\Users\\nxa13790\\Documents\\origen\\o2\\test_apps\\python_app\\targets\\tester\\v93k_smt7.py")
        Text("        C:\\Users\\nxa13790\\Documents\\origen\\o2\\test_apps\\python_app\\targets\\tester\\j750.py")
        Text("  Application")
        Text("    Local Path: C:\\Users\\nxa13790\\Documents\\origen\\o2\\test_apps\\python_app")
        Text("  Origen Core")
        Text("    Version: 2.0.0-pre2")
        Text("    Executable Path: C:\\Users\\nxa13790\\AppData\\Local\\Programs\\Python\\Python38\\python.exe")
        Text("******************************************************************************************")
        Text("Header Comments")
        Text("  From the Application")
        Text("    Hello pattern from the application!")
        Text("******************************************************************************************")
    SetTimeset(0)
    SetPinHeader(2)
    Cycle(100, true)
    Comment(1, "Toggle \'clk\' for a few pulses with the \'simple\' timeset")
    SetTimeset(0)
    PinGroupAction(11, ["1"], None) -> (clk)
        PinAction(8, "1", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["0"], None) -> (clk)
        PinAction(8, "0", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["1"], None) -> (clk)
        PinAction(8, "1", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["0"], None) -> (clk)
        PinAction(8, "0", None) -> (clk)
    Cycle(1, true)
    Comment(1, "Toggle \'clk\' for a few pulses with the \'backwards\' timeset")
    SetTimeset(2)
    PinGroupAction(11, ["1"], None) -> (clk)
        PinAction(8, "1", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["0"], None) -> (clk)
        PinAction(8, "0", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["1"], None) -> (clk)
        PinAction(8, "1", None) -> (clk)
    Cycle(1, true)
    PinGroupAction(11, ["0"], None) -> (clk)
        PinAction(8, "0", None) -> (clk)
    Cycle(1, true)
    SetTimeset(0)
    Comment(1, "Set the clk to an arbitrary symbol")
    PinAction(8, "9", None) -> (clk)
    Cycle(1, true)
    Comment(1, "Set porta to various symbols")
    PinAction(0, "a", None) -> (porta0)
    PinAction(1, "b", None) -> (porta1)
    Cycle(11, true)
    PatternEnd
coreyeng commented 3 years ago

I think I'm making more progress here and it's at least getting to where we can take a genuine look at it. The most recent changes are mostly rework of Rust-side API for writing drivers.

I went with a new struct, a PinBus, which is derived from a PinGroup (so far, could be made derive-able from whatever we need). Due to aliasing of the pins, groups, and such, building and checking for a valid PinBus is actually one of the more expensive tasks, so I wanted to minimize that, but the only way I could get a direct reference to a Pin or PinGroup in the Services was to tie it to the static lifetime which, and maybe @ginty has some insight here, I couldn't tell if it would be unloaded when we call dut.services() and services.change() or not. From my reading, it looked like it could, but not necessarily would. So, I went with just storing pin ids. If it doesn't unload, then that'd essentially be a memory leak that may get problematic if we need to reload the DUT and Services many times, which O1 does in some places (like web generation). I guess we could manually write some tear-down code, but this seemed more involved than I was willing to go for now. But maybe its worth it?

This PinBus has the Rust-side API for drivers on it. Best way is just to show some examples in [SWD}(https://github.com/Origen-SDK/o2/blob/timing_updates/rust/origen/src/services/swd/driver.rs). It should look pretty similar to the Python side. Essentially, given an existing PinGroup you can derive a PinBus which will allow you to push state changes on those pins to the AST.

Here's the driver code to switch to SWD in ArmDebug and here's driving the header of an SWD transaction using this API.

It also seemed like a waste to update the pin states in the pins themselves in the middle of the driver. Nothing outside of the driver will be able to influence it as its running, so it isn't necessary to keep jumping into actual pins to update their states. The pins now use a RWLock to allow access the current state so we don't have to pass a mutable DUT around, but this comes at the cost of borrow-checking at runtime, which I think we want to minimize where possible.

Instead, it’s the driver's responsibility to basically say "I'm done, go update the DUT pins now". The PinBus has a function for this, which will backtrack through the AST, find its most recent state, and update the pin in the DUT appropriately. For SWD, I have this wrapped with its two PinBuses, which are easily called from itself or ArmDebug, wherever needed.

A few notes:

And few other things:

ginty commented 3 years ago

Hi @coreyeng,

Thanks for the updates. This is just some quick feedback based on an intial reading of your update comments...

I think it would be best for pins and regs to closely align in how they work, so they should map roughly as follows:

bit -> pin reg -> pin_group bitcollection -> pin bus (though I would rather you call it pin collection!)

As such the pincollection should be storing real refererences to pins, and that can be achieved with a lifetime on the (immutable) DUT and shouldn't require the use of a static lifetime.

e.g. here is a reg to bitcollection method - https://github.com/Origen-SDK/o2/blob/master/rust/origen/src/core/model/registers/register.rs#L674 So you will need to pass the DUT around anytime you want a PinCollection but that should be no big deal and passing the DUT around as the final argument should be considered pretty standard in Rust APIs.

When we get to the pyapi I would be tempted to also do what the bitcollection does and have a lite pincollection (like your current bus which stores ids) and a richer/real one that can be materialized when detail about the pin is needed.

I am totally happy to lose the pin state variable completely and make pins totally immutable, as you suggest drivers can keep track of pin states themselves if they need it.

I don't think introspection of the AST (basically using it as a runtime variable store) is the right approach. We should think of the AST as something you just push updates into in 99.99% of cases.

So what you are doing here still seems too complicated to me, we should just have a pin/pin_group/pincollection.set_char(...) method where all it needs to do is push the data into the AST. If the application code does something like:

pin("pinA").set_char("0")
pin("pinA").set_char("1")
cycle

Then the AST should look like:

set_char(10, "0")
set_char(10, "1")
cycle

There is no need to optimize that redundant state change at generation time, that would just all come out on the wash at render time. i.e. either we process it to specifically remove such redundancies at the end, or more likely just let the drivers execute the redundant update. The final value in the vector will be correct.

I do like the get_with_decendants function and it will no doubt come in useful during AST processing, but I don't think we should be using it at generation time like this.

I haven't really digested the few other things at the end, but they all sound good.

ginty commented 3 years ago

Having just wrote that, actually I can see that maybe we don't need a rich pincollection, maybe just having an id's based version like you currently have is fine. The difference being that for regs the bits do provide runtime data/state storage, but the pins do not. So maybe all we need here is to rename pinbus to collection (please) and then simplify the AST interaction.

coreyeng commented 3 years ago

Thanks for the feedback @ginty! If I was doing this from scratch, I would've named PinBus PinCollection, but PinCollection is currently used as an anonymous PinGroup on the PyAPI side and I wanted a clean break between these two while I develop the Rust side. Longer term, I'd like to clean up the PinGroup and PinCollection but I don't know how quick of a fix that will be - like the PinActions, its pretty ingrained and would require a fairly large rewrite.

I also would like to have PinBus store actual Pins, and that was my original intent. But, as stated before, the most expensive pin-based operation is trying to resolve the physical pins from pin groups/collections, as the aliasing allowed requires a couple loops. I want to minimize this and storing the IDs themselves was the only way that didn't introduce a static lifetime.

I'm talking about storing, what amounts to, pin references in the Services, such as here in the SWD. Ideally, this would be the Pin itself, not the IDs, but this requires the DUT and Services lifecycle to be tied together and, since they are both, lazy-static implemented, static was the only way I could do this in the current architecture.

We could revisit the discussion of polymorphed models, or have a place to store resolved PinIDs in the DUT and then the SWD setup would store the ID whatever that structure ends up being. As I'm typing this though, I'm thinking the SWD could actually store the resolved PinIDs, then I can have add a function to quickly create a PinBus from the already-known Pin IDs for use in the driver. That should skip repeatedly resolving any groups which for small stuff this will be negligible but large packages of 1000s of pins may see some slowdown if it happens over and over and over again.

There is no need to optimize that redundant state change at generation time, that would just all come out on the wash at render time.

I disagree with this as I think a fair assumption is that any pins the driver wants to update should be towards the back of the AST so a reverse lookup shouldn't take very long. One optimization I need to make is to tie SWDCLK and SWDIO together in a single PinBus so only a single reverse-lookup though the AST is necessary, instead of the two it does now.

ginty commented 3 years ago

You can have pincollection in both origen and pyapi though and if you ever need to access both in the same module then you can rename on import.

Storing IDs in the service and anywhere else is totally fine, that's how its supposed to work. Avoiding tricky lifetime scenarios in long-lived data structures is good (not just for us but for future contributors who will be new to Rust) and fetching a pin when you need it from its ID will be plenty fast enough.

As I'm typing this though, I'm thinking the SWD could actually store the resolved PinIDs, then I can have add a function to quickly create a PinBus from the already-known Pin IDs for use in the driver.

Yes, exactly like that.

On the last point, I think you would do well to stop worrying about optimization of the AST, it is supposed to just be a record of what the application did and if it did a few sub-optimal things then that's what the AST should reflect. It is much easier for everyone to treat interaction with the AST really simply at generation time, as soon as the app does something important record it in the AST and forget about it. Optimization and clean up is much better handled downstream with a dedicated processor pass, these are cheap and maintainable and adding inline complication to avoid those should only be done in very exceptional cases, i.e. to fix a known and significant performance issue.

coreyeng commented 3 years ago

I'd have to rename the PinCollection on the rust/origen side. Though, it is much smaller there, so that could be done pretty easily I think, but it'll be confusing if you jump back to PyAPI and see PinCollection is totally different. Though, that can be a future clean-up item.

I'll try some stuff and we can re-review. The AST interaction updates shouldn't be too bad either.

coreyeng commented 3 years ago

These updates weren't so bad, as I already prototyped the PinBus as containing Pin references, so it was mostly a matter of resurrecting that.

There's really only two key changes:

The implication of this from the driver side is that:

  1. You always need a DUT to perform pin actions (not a big deal, as this was implied anyway as you needed it to update the states. There's just no way around it now).
  2. Services can store IDs to Pins or PinGroups, like here in SWD, and then resolve into a PinCollection when pin actions are needed. All other aspects stay the same.

Per @ginty's recommendation to quit worrying so much about small performance increases at the expense of simplicity (which, to be fair, are negligible in the vast majority of cases), I just left the PinGroup resolution occurring each time. If it turns into a issue, we can optimize it later.

ginty commented 3 years ago

Hi @coreyeng, thought I'd try to help this morning by quickly merging my SupportedTester update in here, however, I ran out of time to finish it, sorry. I don't think it should be a big deal to you to finish it, looks like some functions just need to be switched over to using the new type, if you run pytest you will see what's left.

coreyeng commented 3 years ago

Thanks! I'll complete the merge. I don't see it as explicitly "approved" though. Was there anything else that needed discussion or is it okay to merge this with the latest master, fix the tests, and merge into master?