From 9e6fce427ded3e8ff16ab52725cc3342ac0b4a7a Mon Sep 17 00:00:00 2001 From: Shubham Padkonde Date: Mon, 21 Sep 2026 08:36:52 +0530 Subject: [PATCH] Reject clock periods SimpleClockGenerator cannot generate SimpleClockGenerator toggles the clock every `clockPeriod ~/ 2` time units, so it silently mishandles periods it cannot halve: - a period of 0 or 1 gives a half period of 0, which schedules the next toggle at the current time. The simulator then spins on time 0 forever without advancing, and because the loop never yields the Dart event loop cannot even fire a timeout. - an odd period is rounded down, so a generator asked for 3 produces a clock with a period of 2, and one asked for 5 produces a period of 4. - a negative period schedules an action in the past, which the Simulator rejects with a less obvious error. Validate the period in the constructor instead and throw an IllegalConfigurationException naming the offending value, and document the requirement on `clockPeriod`. Two existing tests passed such periods incidentally: changed_test used 17 for a clock whose only purpose is to add simulator events, and unassignable_test used 1 for a generator that is never simulated. Both now use an even period. Fixes #567 Co-Authored-By: Claude Opus 5 --- lib/src/modules/clkgen.dart | 17 ++++++++- test/changed_test.dart | 2 +- test/clkgen_test.dart | 74 +++++++++++++++++++++++++++++++++++++ test/unassignable_test.dart | 2 +- 4 files changed, 92 insertions(+), 3 deletions(-) create mode 100755 test/clkgen_test.dart diff --git a/lib/src/modules/clkgen.dart b/lib/src/modules/clkgen.dart index d3c18cf3b5..fe4bb1957f 100644 --- a/lib/src/modules/clkgen.dart +++ b/lib/src/modules/clkgen.dart @@ -16,6 +16,9 @@ class SimpleClockGenerator extends Module with SystemVerilog { /// /// For example, if the [clockPeriod] is 10, then the frequency is 1/10, /// and the time between positive edges of the generated clock is 10. + /// + /// The clock toggles once every half period, so [clockPeriod] must be an + /// even number of time units greater than or equal to 2. final int clockPeriod; /// The generated clock. @@ -24,8 +27,20 @@ class SimpleClockGenerator extends Module with SystemVerilog { /// Constructs a very simple clock generator. Generates a non-synthesizable /// SystemVerilog representation. /// - /// Set the frequency via [clockPeriod]. + /// Set the frequency via [clockPeriod], which must be an even number of + /// time units greater than or equal to 2. Other values throw an + /// [IllegalConfigurationException]. SimpleClockGenerator(this.clockPeriod, {super.name = 'clkgen'}) { + if (clockPeriod < 2 || clockPeriod.isOdd) { + throw IllegalConfigurationException( + 'The clockPeriod must be an even number of time units greater than' + ' or equal to 2, but got $clockPeriod. The clock toggles once every' + ' half period, so a period below 2 would schedule both edges within' + ' the same time unit and prevent the simulation from advancing, and' + ' an odd period would silently generate a clock whose period is' + ' ${clockPeriod - 1} instead.'); + } + addOutput('clk'); clk.makeUnassignable( diff --git a/test/changed_test.dart b/test/changed_test.dart index 167cbb7169..c949a2ea55 100644 --- a/test/changed_test.dart +++ b/test/changed_test.dart @@ -371,7 +371,7 @@ void main() { final clk = SimpleClockGenerator(200).clk; // faster clk just to add more events to the Simulator - SimpleClockGenerator(17).clk; + SimpleClockGenerator(16).clk; final posedgeChangingSignal = Logic()..put(0); final negedgeChangingSignal = Logic()..put(0); diff --git a/test/clkgen_test.dart b/test/clkgen_test.dart new file mode 100755 index 0000000000..fdf5d90d7a --- /dev/null +++ b/test/clkgen_test.dart @@ -0,0 +1,74 @@ +// Copyright (C) 2026 Intel Corporation +// SPDX-License-Identifier: BSD-3-Clause +// +// clkgen_test.dart +// Tests for the simple clock generator +// +// 2026 September 21 +// Author: Shubham Padkonde + +import 'dart:async'; + +import 'package:rohd/rohd.dart'; +import 'package:test/test.dart'; + +/// Collects the times of the first [count] positive edges of a clock with +/// [clockPeriod]. +Future> posedgeTimes(int clockPeriod, {int count = 4}) async { + await Simulator.reset(); + + final clk = SimpleClockGenerator(clockPeriod).clk; + final times = []; + + clk.posedge.listen((_) { + times.add(Simulator.time); + if (times.length == count) { + unawaited(Simulator.endSimulation()); + } + }); + + Simulator.setMaxSimTime(clockPeriod * (count + 2)); + await Simulator.run(); + + return times; +} + +void main() { + tearDown(() async { + await Simulator.reset(); + }); + + group('generates the requested period', () { + for (final clockPeriod in [2, 4, 10]) { + test('of $clockPeriod', () async { + final times = await posedgeTimes(clockPeriod); + + expect(times.length, 4); + for (var i = 1; i < times.length; i++) { + expect(times[i] - times[i - 1], clockPeriod, + reason: 'positive edges should be $clockPeriod apart'); + } + }); + } + }); + + group('rejects a period it cannot generate', () { + // A period below 2 has a half period of 0, which schedules both edges in + // the same time unit and hangs the simulation. + for (final clockPeriod in [-2, -1, 0, 1]) { + test('of $clockPeriod', () { + expect(() => SimpleClockGenerator(clockPeriod), + throwsA(isA())); + }); + } + + // An odd period is rounded down by the half period, so the generated + // clock would silently run at a different frequency than requested. + for (final clockPeriod in [3, 5, 11]) { + test('of $clockPeriod', () { + expect(() => SimpleClockGenerator(clockPeriod), + throwsA(isA())); + }); + } + }); +} diff --git a/test/unassignable_test.dart b/test/unassignable_test.dart index dcbe039619..897bc0943c 100644 --- a/test/unassignable_test.dart +++ b/test/unassignable_test.dart @@ -40,7 +40,7 @@ void main() { test('SimpleClockGenerator outputs cannot be assigned', () { try { - SimpleClockGenerator(1).clk <= Logic(); + SimpleClockGenerator(2).clk <= Logic(); fail('Should have thrown an exception'); } on UnassignableException catch (e) { expect(e.toString(), contains('SimpleClockGenerator'));