Fix bugs in LogicStructure instrumentation calls to packed and changed issues across Simulator.reset (#458)

Author: mkorbel1

lib/src/signals/logic_structure.dart

@@ -281,10 +281,12 @@ class LogicStructure implements Logic {
   Logic? get srcConnection => null;
 
   @override
-  LogicValue get value => packed.value;
+  LogicValue get value =>
+      elements.map((e) => e.value).toList(growable: false).rswizzle();
 
   @override
-  LogicValue? get previousValue => packed.previousValue;
+  LogicValue? get previousValue =>
+      elements.map((e) => e.value).toList(growable: false).rswizzle();
 
   @override
   late final int width = elements.map((e) => e.width).sum;
@@ -396,10 +398,32 @@ class LogicStructure implements Logic {
   @Deprecated('Use `value` instead.'
       '  Check `width` separately to confirm single-bit.')
   @override
+  // Can rely on `packed` here because it must be 1 bit.
   LogicValue get bit => packed.bit;
 
   @override
-  Stream<LogicValueChanged> get changed => packed.changed;
+  late final Stream<LogicValueChanged> changed = _internalPacked.changed;
+
+  /// An internal version of [packed] for instrumentation operations on this
+  /// [LogicStructure].
+  late final _internalPacked = _generateInternalPacked();
+
+  /// Generates and subscribes to be stored lazily into [_internalPacked].
+  Logic _generateInternalPacked() {
+    final internalPacked = Logic(width: width)..put(value);
+
+    void updateInternalPacked() {
+      internalPacked.put(value);
+    }
+
+    for (final element in elements) {
+      element.glitch.listen((args) {
+        updateInternalPacked();
+      });
+    }
+
+    return internalPacked;
+  }
 
   @override
   Logic eq(dynamic other) => packed.eq(other);
@@ -408,7 +432,8 @@ class LogicStructure implements Logic {
   Logic neq(dynamic other) => packed.neq(other);
 
   @override
-  SynchronousEmitter<LogicValueChanged> get glitch => packed.glitch;
+  late final SynchronousEmitter<LogicValueChanged> glitch =
+      _internalPacked.glitch;
 
   @override
   Logic gt(dynamic other) => packed.gt(other);
@@ -424,28 +449,32 @@ class LogicStructure implements Logic {
 
   @Deprecated('Use value.isValid instead.')
   @override
-  bool hasValidValue() => packed.hasValidValue();
+  bool hasValidValue() => value.isValid;
 
   @Deprecated('Use value.isFloating instead.')
   @override
-  bool isFloating() => packed.isFloating();
+  bool isFloating() => value.isFloating;
 
   @override
   Logic isIn(List<dynamic> list) => packed.isIn(list);
 
   @override
+  // Can rely on `packed` here because it must be 1 bit.
   Stream<LogicValueChanged> get negedge => packed.negedge;
 
   @override
+  // Can rely on `packed` here because it must be 1 bit.
   Stream<LogicValueChanged> get posedge => packed.posedge;
 
   @override
-  Future<LogicValueChanged> get nextChanged => packed.nextChanged;
+  Future<LogicValueChanged> get nextChanged => changed.first;
 
   @override
+  // Can rely on `packed` here because it must be 1 bit.
   Future<LogicValueChanged> get nextNegedge => packed.nextNegedge;
 
   @override
+  // Can rely on `packed` here because it must be 1 bit.
   Future<LogicValueChanged> get nextPosedge => packed.nextPosedge;
 
   @override
@@ -460,13 +489,14 @@ class LogicStructure implements Logic {
   @override
   Logic zeroExtend(int newWidth) => packed.zeroExtend(newWidth);
 
+  @Deprecated('Use `value` instead.'
+      '  Check `width` separately to confirm single-bit.')
   @override
-  // ignore: deprecated_member_use_from_same_package
-  BigInt get valueBigInt => packed.valueBigInt;
+  BigInt get valueBigInt => value.toBigInt();
 
+  @Deprecated('Use value.toInt() instead.')
   @override
-  // ignore: deprecated_member_use_from_same_package
-  int get valueInt => packed.valueInt;
+  int get valueInt => value.toInt();
 
   @override
   Logic? get _srcConnection => throw UnsupportedError('Delegated to elements');

lib/src/signals/wire.dart

@@ -55,11 +55,26 @@ class _Wire {
       // them! saves performance!
       _changedBeingWatched = true;
 
-      _postTickSubscription ??= Simulator.postTick.listen((event) {
-        if (value != _preTickValue && _preTickValue != null) {
-          _changedController.add(LogicValueChanged(value, _preTickValue!));
-        }
-      });
+      StreamSubscription<void> subscribe() {
+        unawaited(Simulator.resetRequested.then((_) async {
+          final oldPostTickSubscription = _postTickSubscription;
+
+          _postTickSubscription = subscribe();
+
+          unawaited(oldPostTickSubscription?.cancel());
+        }));
+
+        return Simulator.postTick.listen((event) {
+          if (value != _preTickValue && _preTickValue != null) {
+            _changedController.add(LogicValueChanged(value, _preTickValue!));
+          }
+        });
+      }
+
+      assert(_postTickSubscription == null,
+          'Should not be creating a new subscription if one exists.');
+
+      _postTickSubscription = subscribe();
     }
     return _changedController.stream;
   }
@@ -68,9 +83,22 @@ class _Wire {
   ///
   /// If one already exists, it will not create a new one.
   void _setupPreTickListener() {
-    _preTickSubscription ??= Simulator.preTick.listen((event) {
+    _preTickSubscription = Simulator.preTick.listen((event) {
       _preTickValue = value;
     });
+
+    unawaited(Simulator.resetRequested.then((_) async {
+      assert(_preTickSubscription != null,
+          'Should not be null if we are setting up a new one.');
+
+      _preTickValue = value;
+
+      final oldPreTickSubscription = _preTickSubscription;
+
+      _setupPreTickListener();
+
+      unawaited(oldPreTickSubscription?.cancel());
+    }));
   }
 
   /// The [value] of this signal before the most recent [Simulator.tick] had

lib/src/simulator.dart

@@ -89,25 +89,29 @@ abstract class Simulator {
 
   /// Emits an event before any other actions take place on the tick.
   static Stream<void> get preTick => _preTickController.stream;
+  static StreamController<void> _preTickController =
+      StreamController.broadcast(sync: true);
 
   /// Emits an event at the start of actions within a tick.
   static Stream<void> get startTick => _startTickController.stream;
+  static StreamController<void> _startTickController =
+      StreamController.broadcast(sync: true);
 
   /// Emits an event when most events are complete, and clocks are stable.
   static Stream<void> get clkStable => _clkStableController.stream;
+  static StreamController<void> _clkStableController =
+      StreamController.broadcast(sync: true);
 
   /// Emits an event after all events are completed.
   static Stream<void> get postTick => _postTickController.stream;
-
-  static StreamController<void> _preTickController =
-      StreamController.broadcast(sync: true);
-  static StreamController<void> _startTickController =
-      StreamController.broadcast(sync: true);
-  static StreamController<void> _clkStableController =
-      StreamController.broadcast(sync: true);
   static StreamController<void> _postTickController =
       StreamController.broadcast(sync: true);
 
+  /// Completes when [reset] is called after the [Simulator] has completed any
+  /// actions it needs to perform to prepare for the next simulation.
+  static Future<void> get resetRequested => _resetCompleter.future;
+  static Completer<void> _resetCompleter = Completer<void>();
+
   /// Completes when the simulation has completed.
   static Future<void> get simulationEnded => _simulationEndedCompleter.future;
   static Completer<void> _simulationEndedCompleter = Completer<void>();
@@ -156,6 +160,13 @@ abstract class Simulator {
     _pendingTimestamps.clear();
     _phase = SimulatorPhase.outOfTick;
     _injectedActions.clear();
+
+    // make sure we've already passed the new completer so that listeners can
+    // get the latest
+    final oldResetCompleter = _resetCompleter;
+    _resetCompleter = Completer();
+    oldResetCompleter.complete();
+    await oldResetCompleter.future;
   }
 
   /// Sets a time, after which, the [Simulator] will halt processing of new

test/changed_test.dart

@@ -39,6 +39,79 @@ void main() {
     expect(numChangesDetected, equals(1));
   });
 
+  group('across simulator reset', () {
+    Future<void> testAcrossSimulatorResets(Logic a) async {
+      var numChangesDetected = 0;
+
+      a.changed.listen((event) {
+        numChangesDetected++;
+        expect(event.newValue, event.previousValue + 1);
+      });
+
+      a.glitch.listen((args) {});
+
+      Simulator.registerAction(10, () => a.put(1));
+
+      await Simulator.run();
+
+      expect(numChangesDetected, 1);
+
+      await Simulator.reset();
+
+      expect(a.value.toInt(), 1);
+
+      // should *not* trigger a change, didn't change!
+      Simulator.registerAction(20, () => a.put(1));
+
+      await Simulator.run();
+
+      expect(numChangesDetected, 1);
+
+      await Simulator.reset();
+
+      expect(a.value.toInt(), 1);
+
+      // *should* trigger a change
+      Simulator.registerAction(30, () => a.put(2));
+      Simulator.registerAction(40, () => a.put(3));
+
+      await Simulator.run();
+
+      expect(a.value.toInt(), 3);
+
+      expect(numChangesDetected, 3);
+    }
+
+    test('normal logic', () async {
+      final a = Logic(width: 8)..put(0);
+
+      await testAcrossSimulatorResets(a);
+    });
+
+    test('logic structure', () async {
+      final a = LogicStructure([Logic(), Logic()])..put(0);
+
+      await testAcrossSimulatorResets(a);
+    });
+  });
+
+  test('changed does not trigger first time if no change', () async {
+    final a = Logic()..put(1);
+
+    var numChangesDetected = 0;
+
+    a.changed.listen((event) {
+      numChangesDetected++;
+    });
+
+    Simulator.registerAction(10, () => a.put(1));
+    Simulator.registerAction(20, () => a.put(1));
+
+    await Simulator.run();
+
+    expect(numChangesDetected, 0);
+  });
+
   test('clk edge counter', () async {
     final clk = SimpleClockGenerator(10).clk;
     final b = Logic();

test/logic_structure_test.dart

@@ -7,6 +7,8 @@
 // 2023 May 5
 // Author: Max Korbel <[email protected]>
 
+import 'dart:async';
+
 import 'package:rohd/rohd.dart';
 import 'package:rohd/src/utilities/simcompare.dart';
 import 'package:test/test.dart';
@@ -70,11 +72,81 @@ class FancyStructInverter extends Module {
   }
 }
 
+class StructModuleWithInstrumentation extends Module {
+  StructModuleWithInstrumentation(Logic a) {
+    a = addInput('a', a, width: 2);
+
+    MyStruct()
+      ..gets(a)
+      ..value
+      ..previousValue
+      ..width
+      ..srcConnection
+      ..dstConnections
+      ..parentModule
+      ..parentStructure
+      ..naming
+      ..arrayIndex
+      ..isArrayMember
+      ..leafElements
+      ..isInput
+      ..isOutput
+      ..changed
+      ..glitch
+      ..nextChanged
+      // ignore: deprecated_member_use_from_same_package
+      ..hasValidValue()
+      // ignore: deprecated_member_use_from_same_package
+      ..isFloating()
+      // ignore: deprecated_member_use_from_same_package
+      ..valueBigInt
+      // ignore: deprecated_member_use_from_same_package
+      ..valueInt;
+  }
+}
+
 void main() {
   tearDown(() async {
     await Simulator.reset();
   });
 
+  test('late previousValue', () async {
+    final s = MyStruct();
+    final clk = SimpleClockGenerator(10).clk;
+    Simulator.setMaxSimTime(200);
+
+    var i = 0;
+    clk.posedge.listen((_) => s.inject(i++));
+
+    unawaited(Simulator.run());
+
+    await clk.nextPosedge;
+    await clk.nextPosedge;
+    await clk.nextPosedge;
+
+    expect(s.previousValue, isNotNull);
+    expect(s.previousValue!.toInt(), 1);
+
+    s.packed;
+    await clk.nextPosedge;
+    await clk.nextPosedge;
+    expect(s.packed.previousValue, s.previousValue);
+    await clk.nextPosedge;
+    expect(s.packed.previousValue, s.previousValue);
+
+    await Simulator.endSimulation();
+  });
+
+  test('instrumentation on struct does not make hardware', () async {
+    final mod = StructModuleWithInstrumentation(Const(0, width: 2));
+    await mod.build();
+
+    final sv = mod.generateSynth();
+
+    expect(sv.contains('swizzle'), isFalse,
+        reason: 'Should not pack from instrumentation!');
+  });
+
   group('LogicStructure construction', () {
     test('simple construction', () {
       final s = LogicStructure([