Reject clock periods SimpleClockGenerator cannot generate

[Shubham-Padkonde]

Description & Motivation

SimpleClockGenerator toggles its clock every clockPeriod ~/ 2 time units. That integer division means the module silently mishandles every period it cannot halve, which is what #567 ran into.

Measured on main at d22fe80, driving each generator and recording the first positive edges:

clockPeriod	actual spacing between positive edges
10	10
4	4
2	2
5	4
3	2
1	never advances
0	never advances
-2	SimulatorException: Cannot add timestamp "-1" in the past

Three distinct problems:

• A period below 2 has a half period of 0, so the toggle is registered at the current simulator time. The simulator then spins on time 0 forever. Worth noting that this is not an ordinary hang: the loop never yields, so a Future.timeout() around Simulator.run() cannot fire either, and the reproducer has to be killed.
• An odd period is rounded down, so a generator asked for 3 quietly produces a clock whose period is 2, and one asked for 5 produces 4. Nothing reports this; the testbench just runs at the wrong frequency.
• A negative period schedules into the past, producing an error that doesn't point at the clock generator.

Following the direction in #567 — decide explicitly what to do for 1, odd numbers, and illegal numbers, and throw for anything unsupported — this validates clockPeriod in the constructor and throws IllegalConfigurationException naming the offending value and explaining why it cannot be generated. The requirement is documented on clockPeriod and on the constructor.

Related Issue(s)

Fixes #567

Testing

New test/clkgen_test.dart:

• generates the requested period for 2, 4 and 10 asserts the spacing between consecutive positive edges equals the requested period — this is what actually pinned down that 3 and 5 were wrong.
• rejects a period it cannot generate for -2, -1, 0, 1, 3, 5 and 11 asserts IllegalConfigurationException.

All 7 rejection tests fail on the unpatched tree (+3 -7), and all 10 pass with the change.

00:00 +10: All tests passed!

Full suite, dart test -j 3, clean main versus this branch:

04:34 +959 ~51 -772: Some tests failed.    <- main
04:32 +969 ~51 -772: Some tests failed.    <- this branch

So 969 passing and 51 skipped, the +10 being exactly the new tests in this PR, with the failure count unchanged. Every one of the 772 failures is Icarus Verilog missing from my machine, not a behaviour change: 743 report ProcessException ... Command: iverilog -g2012 ... directly, and the remaining 29 are name_test.dart:186 catching that same exception and reporting Expected to pass!. No other exception type appears anywhere in the log. CONTRIBUTING notes iverilog is required for the complete suite; CI has it, so these will run there.

dart format reports no changes and dart analyze is clean on the touched files.

Backwards-compatibility

Yes, this is a breaking change for anyone passing a period below 2 or an odd period — they now get an exception at construction instead of a silently wrong or hanging simulation. That is the intent of #567, but it is worth calling out explicitly.

Two tests in this repo were relying on such values incidentally, and are updated here:

• changed_test.dart used SimpleClockGenerator(17) for a clock whose stated purpose is "just to add more events to the Simulator" — now 16.
• unassignable_test.dart used SimpleClockGenerator(1) for a generator that is never simulated, only checked for an assignment error — now 2.

Every other construction in the repo (96 of them) already passes an even period of 10 or 200. I grepped for non-literal arguments; the only ones are in the new test file, so no other call site is affected.

If you would rather keep odd periods working — rounding down as today, perhaps with a warning — I'm happy to narrow this to just clockPeriod < 2 and drop the two test updates. Say the word and I'll push that instead.

Documentation

Yes, and included: the clockPeriod field doc and the constructor doc both now state that the period must be an even number of time units greater than or equal to 2, and that other values throw. No user-guide page documents this constructor, so nothing outside the API docs needed updating.

---

Disclosure: this change was written by Claude Code (Claude Opus 5) working as my agent, at my direction. The measurements and test output quoted above come from real runs in my local environment; I am accountable for the content of this PR and happy to iterate on review feedback.

🤖 Generated with Claude Code

[Shubham-Padkonde]

Correction to the Testing section above (I've edited the body): I originally wrote "1209 passing" by adding a 240-test run to the 969 from the full suite. That was wrong — those 240 were a re-run of a subset of the same suite, not additional tests, so I double-counted them.

The correct figures, from running clean main and this branch back to back:

04:34 +959 ~51 -772: Some tests failed.    <- main
04:32 +969 ~51 -772: Some tests failed.    <- this branch

969 passing, 51 skipped, with the +10 being exactly the new tests in this PR and the failure count unchanged. The substance is unaffected — all 772 failures are still Icarus Verilog missing locally — but the number I quoted was inflated and I'd rather flag that than quietly fix it.