Add OLED connector to ulx3s.py

[GuzTech]

This adds the OLED connector pins to the ULX3S platform. The constraints were taken from https://github.com/emard/ulx3s-misc/blob/master/constraints/ulx3s_v20.lpf

[awygle]

Please make this resource consistent with the version found in genesys2.py, which splits out the SPI signals from the control signals and groups the latter as Subsignals under a unified Resource:

        SPIResource(0,  # OLED, SSD1306, 128 x 32
                    cs="dummy-cs0", clk="AF17", copi="Y15",
                    cipo="dummy-cipo0", reset="AB17",
                    attrs=Attrs(IOSTANDARD="LVCMOS18")),
        Resource("oled", 0,  # OLED, UG-2832HSWEG04
                 Subsignal("dc", Pins("AC17", dir="o")),
                 Subsignal("vdd_en", PinsN("AG17", dir="o")),
                 Subsignal("vbat_en", PinsN("AB22", dir="o"),
                           Attrs(IOSTANDARD="LVCMOS33")),
                 Attrs(IOSTANDARD="LVCMOS18")),

Ideally we want the nmigen-boards platforms to be as consistent as possible about things like this.

[GuzTech]

I've tried this, but I'm running into a problem. SPIResource expects a cipo pin, but the connector does not have one. If I set the cipo parameter to None then I get an error that it cannot be empty, and any other string that I provide gives me an error that no pin with that name exists.

[awygle]

Hm. I think this is sufficiently weird to warrant further discussion - I'll bring it up with whitequark at Monday's meeting when she's back from vacation.

[whitequark]

I've tried this, but I'm running into a problem. SPIResource expects a cipo pin, but the connector does not have one. If I set the cipo parameter to None then I get an error that it cannot be empty, and any other string that I provide gives me an error that no pin with that name exists.

It is now possible to specify either cipo=None or copi=None when creating an SPIResource. Please update the PR to use it.

[GuzTech]

I just update nmigen and nmigen-boards, and I still get the same error that the cipo parameter must be a whitespace-separated string, not None.

This is what I have right now:

# OLED connector
SPIResource(0,
    cs="N2", clk="P4", copi="P3", cipo=None, reset="P2",
    attrs=Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),
Resource("oled", 0,
    Subsignal("dc", Pins("P1", dir="o"),
        Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP"))),

For clarity, nmigen-boards is 0.1.dev158 and nmigen is 0.3.dev227.

[whitequark]

I just update nmigen and nmigen-boards

Sorry, I forgot to push. Try now.

[GuzTech]

I modified it, but the test code didn't work. Turns out I had to invert the cs signal for the OLED display to work.

In the old code I just directly assigned to the cs pin which I called csn, but when using SPIResource, I have to manually invert the cs pin myself. Does nMigen place inverters based on the name or something?

[whitequark]

Does nMigen place inverters based on the name or something?

Nope; nMigen places inverters if you use PinsN or (in case of resource factories) pass invert=True, but never on its own. Could it be a bug somewhere?

[GuzTech]

That's strange then. I would expect these two to be equivalent:

SPIResource(0,
            cs="N2", clk="P4", copi="P3", cipo=None, reset="P2",
            attrs=Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),

spi = platform.request("spi")
oled_csn = spi.cs

and

oled_resource = [
    Resource("oled_clk",  0, Pins("P4", dir="o"), Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),
    Resource("oled_mosi", 0, Pins("P3", dir="o"), Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),
    Resource("oled_resn", 0, Pins("P2", dir="o"), Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),
    Resource("oled_csn",  0, Pins("N2", dir="o"), Attrs(IO_TYPE="LVCMOS33", DRIVE="4", PULLMODE="UP")),
]

platform.add_resources(oled_resource)
oled_csn  = platform.request("oled_csn")

But in the former case I have to use ~oled_csn in order for it work work.

[whitequark]

No, you're actually right. SPIResource does indeed invert CS. I think this highlights a flaw in our current tooling and conventions.

We should develop a tool that prints an overview of the platform's connectivity: which resources were actually used, what polarities do the pins have, what kind of IO buffer are they configured with, and what connectors do they go through.

We should also consider changing the naming convention so that pins with fixed inversion reflect that in the argument name. This really should've been cs_n="N2"...

[GuzTech]

I agree. Especially because I just assign to pins of the platform and I don't really think about any of the IO buffers the might get added by nMigen/synthesis tools. I happened to look at the generated code and saw that one version had an inverter and the other didn't.

Another problem is that cs is just a name. If I set cs to 1, I expect the pin to go high. If I set cs_n to 1, I expect the pin to also go high, but I can see other people maybe thinking that it will get inverted so the pin will actually go low.

So yes, I think an overview with resources, polarities, IO buffer types, etc. would definitely help to clear thing up.

[whitequark]

Another problem is that cs is just a name. If I set cs to 1, I expect the pin to go high. If I set cs_n to 1, I expect the pin to also go high, but I can see other people maybe thinking that it will get inverted so the pin will actually go low.

The logic here is:

• Inside of an idiomatic nMigen design, all signals are active high. Then, if you ever see with m.If(cs):, you know it will trigger when cs becomes active.
• At the boundary of the design, the signals are conditioned, buffered, and inverted as appropriate. So if you see Pins, that's active high, and PinsN, active low.

Although this might not be common in contemporary HDL, it is, in my view, self-consistent and clear.

But, unfortunately, the way we defined resource factories does not follow that principle, as you have clearly demonstrated.

[whitequark]

Filed #129 to track the pin naming issue.

[GuzTech]

Yes, and that's exactly how I expect it to work as well. I just looked at the SPIResource again and saw that cs is indeed a PinsN, so active low. But yeah, that's not clear from just the name itself.

Would it be a breaking change to either rename cs to cs_n or something, or to change it from PinsN to Pins? I could go through all the board files, change them, and create a PR if you want.

[whitequark]

Would it be a breaking change to either rename cs to cs_n or something

It would (specifically, it would break any external platforms that rely on resource definitions from this repo), but that's fine as this is a really severe issue.

Changing cs to cs_n is the right choice here.

I could go through all the board files, change them, and create a PR if you want.

That would be most welcome! It's more than just cs though, we have quite a few pins with fixed inverters.

[GuzTech]

In interface.py I only see that the cs signal in SPIResource is inconsistent with what its name implies. I could start with those first, and if there are other names that need to be changed, I could create a separate PR for them.

EDIT: I just realized that this has to be modified in nMigen too, or else cs_n would be referring to a function parameter that doesn't exist.

[whitequark]

EDIT: I just realized that this has to be modified in nMigen too, or else cs_n would be referring to a function parameter that doesn't exist.

Nope, the resources are all defined in nmigen-boards, specifically to handle this case.

[whitequark]

In interface.py I only see that the cs signal in SPIResource is inconsistent with what its name implies.

There are a lot more in nmigen_boards.resources.memory.

[whitequark]

Filed amaranth-lang/amaranth#551 to track report generation.

[GuzTech]

Ah yes, I see it now. I'll go through the entire nmigen-boards repo then.

[GuzTech]

Ok so now that the function names are properly changed, this PR should be able to get merged... except that I had to remove my fork of the repo for the previous PR. I don't really do much with PRs so I screwed this one up as well :S

I'll create a new PR with the suggestions from this PR, and if/when it gets merged, we can close this one.