simulation.assembly: implement pulls and pin inversion for jumpers by hcsch · Pull Request #1098 · GlasgowEmbedded/glasgow

hcsch · 2026-01-08T15:28:55Z

While attempting to test a 24x I²C EEPROM emulator applet, I wanted to
hook it up to the Memory24xInterface. Since the I²C bus requires a
pull-up somewhere on the open-drain SCL/SDA lines, the
Memory24xInterface configures pull-ups for the pins it is connected
to. The simulation assembly however did not support this so far.
This led to a non-functioning bus.

software/tests/gateware/test_i2c.py manually sets the values of the
simulation ports' output signals based on the oe signals. For a more
general implemention for Glasgow simulation assemblies, this change
follows a similar pattern, setting jumper signals to the desired
PullState's equivalent bit value, if all connected pins have their
output disabled.

While implementing this I noticed that pin inversion was being ignored
so far as well. This change also implements that, by passing the
requested inversion on to the created SimulationPorts.

With this change conflicting configuration requests for the same pin
lead to an exception. As do conflicting pulls on different pins
connected by a jumper.

whitequark · 2026-01-08T15:34:31Z

@Sazzach Do you think you could help me review this?

Sazzach · 2026-01-08T15:45:23Z

Yep, I'm happy to help review this! I'll take a look today.

hcsch · 2026-01-08T15:46:44Z

I'm fixing the lint issues just now (where applicable), I'll force-push in a bit. The test failures seem to be unrelated CI breakage (do correct me if not though)

Sazzach

Thanks for the pull request!

It looks good to me for the most part. My main concern is that in HardwareAssembly the behavior of set_pin_pull is to record the desired pull, and configure_ports performs the actual setting of the pulls. Here on SimulationAssembly, set_pin_pull both records the desired pulls and prevents changing them (in the SimulationPin.pull_state property).

I think that at some point it would be good to have configure_ports be able to be used to change the pulls during simulation, but currently I think only one applet does so.

Something along the lines of changing set_pin_pull to not restrict pull changes and changing configure_ports to emit a warning explaining that changing pull values during simulation is unsupported would resolve my concerns here, because currently in run the pull_state is fixed at elaboration time.

Sazzach · 2026-01-09T01:02:05Z

+        self._name = name
+        self._pin = pin
+        self._port = io.SimulationPort("io", 1, name=self._name, invert=pin.invert)
+        self._pull_state = None


It looks to me like the behavior of SimulationPin is the same when _pull_state = None and _pull_state = PullState.Float (aside from reading the property). Consider initializing _pull_state to PullState.Float instead of None. I think it would simplify things a little.

My original thought with this was to enable logging a change from unconfigured to a specific state, but I did not end up implementing something like that. I'll initialize with PullState.Float as you suggest

I've decided to keep this since I've now actually implemented logging that makes use of this. If you still want to drop the None, I'm fine with that as well.

Sazzach · 2026-01-09T01:09:49Z

+        if self._pull_state is None:
+            self._pull_state = new_state
+        else:
+            self._pull_state = self._pull_state.combine(new_state)


I don't think combine should be called here. Why should the new value be combined with the old value? It also means that pull_state cannot be changed from High or Low.

I think its fine to not support changing pull_state during simulation for now, but I think we should enforce this on configure_ports.

Here I thought along the lines of different parts of the same design requesting conflicting pulls being an issue (conflicting interests of which one gets fulfilled or direct path from Vcc to ground across two resistors in my head). Admittedly I kind of overlooked pull changes at runtime

I've changed this to just update the pull state, logging at debug level when the pull state is changed.

Sazzach · 2026-01-10T07:59:36Z

+                    m.d.comb += net.eq(pin.port.o)
            m.d.comb += Assert(
-                sum(Cat(pin.oe for pin in pins)) <= 1,
+                sum(Cat(pin.port.oe for pin in pins)) <= 1,


I think that this should be changed to check that the pins are at different levels too in order to support testing things such as I2C clock stretching.

I wasn't sure on that and was actually having an issue with that, but thought it to be worth my I2C EEPROM emulator setup.

I wanted to implement such a check for allowing multiple outputs, as long as they are at the same logic level, but was a bit uncertain whether the o signals are unaffected by a specific value already being asserted on the jumper signal, but since the jumper is set to the o signals and not the other way around, and there is no loop back from i to o (I think, though I vaguely recall seeing something like this for io ports or buffers) this is likely a non-problem after all.

I'll do some more source code reading and will fix this after ^^

Ah, I was thinking of https://github.com/amaranth-lang/amaranth/blob/3a3bf815dd17177c5245f6e489e125ad5e006972/amaranth/lib/io.py#L632-L636, but that's only from o to i, not the other way around.

I've adjusted the check for electrical contention in ca0f9ea to just check that no different values are output at the same time on a given jumper.

Do not fail the assertion just because of multiple simultaneously enable outputs. Only fail once two enabled outputs are set to different levels.

While attempting to test a 24x I²C EEPROM emulator applet, I wanted to hook it up to the `Memory24xInterface`. Since the I²C bus requires a pull-up somewhere on the open-drain SCL/SDA lines, the `Memory24xInterface` configures pull-ups for the pins it is connected to. The simulation assembly however did not support this so far. This led to a non-functioning bus. `software/tests/gateware/test_i2c.py` manually sets the values of the simulation ports' output signals based on the `oe` signals. For a more general implemention for Glasgow simulation assemblies, this change follows a similar pattern, setting jumper signals to the desired `PullState`'s equivalent bit value, if all connected pins have their output disabled. While implementing this I noticed that pin inversion was being ignored so far as well. This change also implements that, by passing the requested inversion on to the created `SimulationPort`s. With this change, setting conflicting pulls on different pins connected by a jumper leads to an exception.

This fixes conflicting pull detection across pins connected across multiple calls of `SimulationAssembly.connect_pins`.

hcsch · 2026-02-04T21:16:45Z

Sorry for taking a bit to get back to this PR. I've incorporated most of your feedback, especially that of not blocking changes to pull states before runtime and raising an exception when calling configure_ports at runtime for now.

If you have any further feedback on the new commits, I'd be happy to incorporate that as well.

Runtime changes to pulls would probably require adding a process that handles jumper values at runtime, right? How's the performance impact of that (if relevant and known)? Probably gonna leave that for a later follow-up PR though in case I end up needing that.

hcsch · 2026-02-04T21:44:56Z

Sorry for the noise, I just noticed that I made a mistake in adding the test for electrical contention on a jumper (I left an async in from when I was still using a decorator, which hid some more errors in the test body). I've fixed that now, though I still seem to have an UnusedElaboratable in test_jumper_non_conflicting_pulls...

hcsch · 2026-02-23T08:24:19Z

I've looked into the UnusedElaboratable warning and it actually comes from test_jumper_conflicting_pulls and only surfaces when multiple tests are run (presumably due to python running for longer and actually bothering to clean up objects, triggering the warning). This warning is expected for this test, but I'm not sure if I can catch it in the test implementation.

Sadly the single source line for the creation of the object provided by amaranth and python's tracemalloc were not enough to diagnose this. The Elaboratable in question is the top-level simulation Module. This appears in all tests and the delayed nature of the warning makes it hard to associate with a specific test when running multiple.
tracemalloc seems to also only give you a single source location and not a call trace as well (in this case the location is even inside amaranth's code (_unused.py) so tracemalloc effectively yields no usable information).

Is this stray warning acceptable, or should I drop that test / try to suppress this warning somehow?

whitequark · 2026-02-27T01:19:10Z

Is this stray warning acceptable, or should I drop that test / try to suppress this warning somehow?

There's a suppression for it, # amaranth: UnusedElaboratable=no at the start of file. Try adding it in the files you modify, one of them will probably do the trick

When a simulation is run with an erroneous setup (e.g. conflicting pulls) the `SimulationAssembly.run` can be exited via an exception before the simulation top-level module is elaborated. The user will have already gotten an exception, but an unused warning is still emitted later on, in a testbench possibly during the run of another test, making the warning hard to associate with the right test. Supress the `UnusedElaboratable` warning for this glasgow internal code to reduce this unnecessary noise.

hcsch requested a review from whitequark as a code owner January 8, 2026 15:28

hcsch force-pushed the feat/sim-jumpers-pulls-inversion branch from cee87fc to 1f4de38 Compare January 8, 2026 16:22

Sazzach reviewed Jan 10, 2026

View reviewed changes

simulation.assembly: loosen electrical contention check

ca0f9ea

Do not fail the assertion just because of multiple simultaneously enable outputs. Only fail once two enabled outputs are set to different levels.

hcsch force-pushed the feat/sim-jumpers-pulls-inversion branch from 1f4de38 to 3d82ec1 Compare February 4, 2026 21:02

hcsch added 2 commits February 4, 2026 22:06

simulation.assembly: merge jumped pin groups

dd9d21b

This fixes conflicting pull detection across pins connected across multiple calls of `SimulationAssembly.connect_pins`.

hcsch force-pushed the feat/sim-jumpers-pulls-inversion branch from 3d82ec1 to 0f0c225 Compare February 4, 2026 21:06

hcsch requested a review from Sazzach February 4, 2026 21:16

simulation.assembly: add tests for pull-ups, inversion, jumpers

0375e52

hcsch force-pushed the feat/sim-jumpers-pulls-inversion branch from 0f0c225 to 0375e52 Compare February 4, 2026 21:41

whitequark added waiting-on-author Status: Waiting on issue reporter or PR author waiting-on-review Status: Waiting for review and removed waiting-on-author Status: Waiting on issue reporter or PR author labels Feb 11, 2026

hcsch mentioned this pull request Feb 28, 2026

_unused.MustUse: add optional stack trace capturing amaranth-lang/amaranth#1655

Draft

Conversation

hcsch commented Jan 8, 2026

Uh oh!

whitequark commented Jan 8, 2026

Uh oh!

Sazzach commented Jan 8, 2026

Uh oh!

hcsch commented Jan 8, 2026

Uh oh!

Sazzach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hcsch commented Feb 4, 2026

Uh oh!

hcsch commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hcsch commented Feb 23, 2026

Uh oh!

whitequark commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hcsch commented Feb 4, 2026 •

edited

Loading