Timing issues (#561)

* INT8 counter should count Interrupt activations and not PIT counter (which runs backward)

* INT8 counter should count Interrupt activations and not PIT counter (which runs backward). Remove timer hack to call vga card.

* Change screen refresh from infinite loop to 60hz timer

---------

Co-authored-by: JvE <joris.vaneijden@gmail.com>

src/Spice86.Core/Emulator/Devices/Timer/Counter.cs

@@ -138,7 +138,7 @@ private byte Policy3(ushort value) {
         return Lsb(value);
     }
 
-    private void UpdateDesiredFreqency(long desiredFrequency) {
+    public void UpdateDesiredFreqency(long desiredFrequency) {
         Activator.Frequency = desiredFrequency;
         if (_loggerService.IsEnabled(Serilog.Events.LogEventLevel.Verbose)) {
             _loggerService.Verbose("Updating counter {Index} frequency to {DesiredFrequency}", Index, desiredFrequency);

src/Spice86.Core/Emulator/Devices/Timer/Timer.cs

@@ -33,22 +33,13 @@ public class Timer : DefaultIOPortHandler, ITimeMultiplier {
     private readonly Counter[] _counters = new Counter[3];
     private readonly DualPic _dualPic;
 
-    private readonly IVideoCard? _vgaCard;
-
-    // screen refresh
-    private readonly Counter _vgaScreenRefreshCounter;
-
-    public Timer(State state, ILoggerService loggerService, DualPic dualPic, IVideoCard? vgaCard, CounterConfigurator counterConfigurator, bool failOnUnhandledPort) : base(state, failOnUnhandledPort, loggerService) {
+    public Timer(State state, ILoggerService loggerService, DualPic dualPic,CounterConfigurator counterConfigurator, bool failOnUnhandledPort) : base(state, failOnUnhandledPort, loggerService) {
         _dualPic = dualPic;
-        _vgaCard = vgaCard;
         for (int i = 0; i < _counters.Length; i++) {
             _counters[i] = new Counter(state,
                 _loggerService,
                 i, counterConfigurator.InstanciateCounterActivator(state));
         }
-        // screen refresh is 60hz regardless of the configuration
-        _vgaScreenRefreshCounter = new Counter(state, _loggerService, 4, new TimeCounterActivator(1));
-        _vgaScreenRefreshCounter.SetValue((int)(Counter.HardwareFrequency / 60));
     }
 
     /// <inheritdoc cref="ITimeMultiplier" /> 
@@ -113,14 +104,11 @@ public override void WriteByte(int port, byte value) {
         base.WriteByte(port, value);
     }
 
+
     public void Tick() {
         if (_counters[0].ProcessActivation()) {
             _dualPic.ProcessInterruptRequest(0);
         }
-
-        if (_vgaScreenRefreshCounter.ProcessActivation()) {
-            _vgaCard?.UpdateScreen();
-        }
     }
 
     private static bool IsCounterRegisterPort(int port) => port is >= CounterRegisterZero and <= CounterRegisterTwo;

src/Spice86.Core/Emulator/Devices/Video/VgaCard.cs

@@ -6,16 +6,15 @@ namespace Spice86.Core.Emulator.Devices.Video;
 using Spice86.Shared.Emulator.Video;
 using Spice86.Shared.Interfaces;
 
+using System.Diagnostics;
+
 /// <summary>
 ///     Thin interface between renderer and gui.
 /// </summary>
-public class VgaCard : IVideoCard {
+public class VgaCard : IDebuggableComponent {
     private readonly IGui? _gui;
     private readonly ILoggerService _logger;
     private readonly IVgaRenderer _renderer;
-    private int _renderHeight;
-    private int _renderWidth;
-    private int _requiredBufferSize;
 
     /// <summary>
     ///     Create a new VGA card.
@@ -24,30 +23,36 @@ public VgaCard(IGui? gui, IVgaRenderer renderer, ILoggerService loggerService) {
         _gui = gui;
         _logger = loggerService;
         _renderer = renderer;
-        _renderWidth = renderer.Width;
-        _renderHeight = renderer.Height;
-        _requiredBufferSize = _renderWidth * _renderHeight;
-        if (gui is not null) {
-            gui.RenderScreen += (_, e) => Render(e);
+        if (_gui is not null) {
+            _gui.RenderScreen += (_, e) => Render(e);
+            // Init bitmaps, needed for GUI to start calling Render function
+            _gui.SetResolution(_renderer.Width, _renderer.Height);
         }
     }
 
-    public void UpdateScreen() {
-        if (_renderer.Width != _renderWidth || _renderer.Height != _renderHeight) {
-            _gui?.SetResolution(_renderer.Width, _renderer.Height);
-            _renderWidth = _renderer.Width;
-            _renderHeight = _renderer.Height;
-            _requiredBufferSize = _renderWidth * _renderHeight;
+    private bool EnsureGuiResolutionMatchesHardware() {
+        if (_renderer.Width == _gui?.Width && _renderer.Height == _gui?.Height) {
+            // Resolution is matching, nothing to do.
+            return true;
         }
-        _gui?.UpdateScreen();
+        _gui?.SetResolution(_renderer.Width, _renderer.Height);
+        // Wait for it to be applied
+        while (_renderer.Width != _gui?.Width || _renderer.Height != _gui?.Height);
+        // Report that resolution did not match
+        return false;
     }
+
 
-    /// <inheritdoc />
     private unsafe void Render(UIRenderEventArgs uiRenderEventArgs) {
+        if (!EnsureGuiResolutionMatchesHardware()) {
+            // Gui resolution had to be changed, UI event is not valid anymore.
+            return;
+        }
         var buffer = new Span<uint>((void*)uiRenderEventArgs.Address, uiRenderEventArgs.Length);
-        if (buffer.Length < _requiredBufferSize && _logger.IsEnabled(LogEventLevel.Warning)) {
+        int requiredBufferSize = _renderer.Width * _renderer.Height;
+        if (buffer.Length < requiredBufferSize && _logger.IsEnabled(LogEventLevel.Warning)) {
             _logger.Warning("Buffer size {BufferLength} is too small for the required buffer size {RequiredBufferSize} for render resolution {RenderWidth} x {RenderHeight}",
-                buffer.Length, _requiredBufferSize, _renderWidth, _renderHeight);
+                buffer.Length, requiredBufferSize, _renderer.Width, _renderer.Height);
             return;
         }
         _renderer.Render(buffer);

src/Spice86.Core/Emulator/Gdb/GdbCustomCommandsHandler.cs

@@ -176,7 +176,6 @@ private string ExecuteCustomCommand(ExecutionFlowRecorder executionFlowRecorder,
             "dumpall" => DumpAll(executionFlowRecorder, functionHandler),
             "breakcycles" => BreakCycles(args),
             "breakcsip" => BreakCsIp(args),
-            "vbuffer" => Vbuffer(args),
             _ => InvalidCommand(originalCommand),
         };
     }
@@ -257,11 +256,6 @@ private string Help(string additionnalMessage) {
  - peekRet<optional type>: displays the return address of the current function as stored in the stack in RAM.If a parameter is provided, dump the return on the stack as if the return was one of the provided type. Valid values are: {GetValidRetValues()}
  - state: displays the state of the machine
  - ramx: displays the content of ram at the specified segmented address with x being the number of bits to extract. Example: ram8 DS:1234 => Will display the byte at address DS:1234
- - vbuffer: family of commands to control video bufers:
-   - vbuffer refresh: refreshes the screen
-   - vbuffer add<address> <resolution> <scale?>: Example vbuffer add 0x1234 320x200 1.5 -> Add an additional buffer displaying what is at address 0x1234, with resolution 320x200 and scale 1.5
-   - vbuffer remove <address>: Deletes the buffer at address
-   - vbuffer list: Lists the buffers currently displayed
 ");
     }
 
@@ -301,21 +295,4 @@ private string State() {
         string state = _state.ToString();
         return _gdbIo.GenerateMessageToDisplayResponse(state);
     }
-
-    private string Vbuffer(string[] args) {
-        try {
-            string action = ExtractAction(args);
-
-            // Actions for 1 parameter
-            if ("refresh".Equals(action)) {
-                _gui?.UpdateScreen();
-                return _gdbIo.GenerateResponse("");
-            } else {
-                return _gdbIo.GenerateMessageToDisplayResponse($"Could not understand action {action}");
-            }
-        } catch (ArgumentException e) {
-            e.Demystify();
-            return _gdbIo.GenerateMessageToDisplayResponse(e.Message);
-        }
-    }
 }

src/Spice86.Core/Emulator/InterruptHandlers/Timer/TimerInt8Handler.cs

@@ -36,8 +36,7 @@ public TimerInt8Handler(IMemory memory, Cpu cpu, DualPic dualPic, Timer timer, B
 
     /// <inheritdoc />
     public override void Run() {
-        long numberOfTicks = _timer.NumberOfTicks;
-        TickCounterValue = (uint)numberOfTicks;
+        TickCounterValue++;
         _dualPic.AcknowledgeInterrupt(0);
     }
 

src/Spice86.Core/Emulator/VM/Machine.cs

@@ -157,7 +157,7 @@ public sealed class Machine : IDisposable, IDebuggableComponent {
     /// <summary>
     /// The VGA Card.
     /// </summary>
-    public IVideoCard VgaCard { get; }
+    public VgaCard VgaCard { get; }
 
     /// <summary>
     /// The VGA Registers
@@ -233,7 +233,7 @@ public Machine(IGui? gui, State cpuState, IOPortDispatcher ioPortDispatcher, ILo
         VgaRenderer = new Renderer(VgaRegisters, vgaMemory);
         VgaCard = new VgaCard(gui, VgaRenderer, loggerService);
 
-        Timer = new Timer(CpuState, loggerService, DualPic, VgaCard, counterConfigurator, configuration.FailOnUnhandledPort);
+        Timer = new Timer(CpuState, loggerService, DualPic, counterConfigurator, configuration.FailOnUnhandledPort);
         if (gui is not null) {
             gui.ProgrammableIntervalTimer = Timer;
         }

src/Spice86.Shared/Interfaces/IGui.cs

@@ -43,10 +43,9 @@ public interface IGui {
     /// </summary>
     double MouseY { get; set; }
 
-    /// <summary>
-    /// Refresh the display with the content of the video ram.
-    /// </summary>
-    void UpdateScreen();
+    public int Width { get; }
+
+    public int Height { get; }
 
     /// <summary>
     /// On video mode change: Set Resolution of the video source for the GUI to display

src/Spice86/ViewModels/MainWindowViewModel.cs

@@ -36,8 +36,11 @@
 using Spice86.Models.Debugging;
 using Spice86.Shared.Emulator.Video;
 
+using Timer = System.Timers.Timer;
+
 /// <inheritdoc cref="Spice86.Shared.Interfaces.IGui" />
 public sealed partial class MainWindowViewModel : ViewModelBase, IPauseStatus, IGui, IDisposable {
+    private const double ScreenRefreshHz = 60;
     private readonly ILoggerService _loggerService;
     private readonly IUIDispatcherTimer _uiDispatcherTimer;
     private readonly IHostStorageProvider _hostStorageProvider;
@@ -58,10 +61,9 @@ public sealed partial class MainWindowViewModel : ViewModelBase, IPauseStatus, I
     private bool _isAppClosing;
 
     private static Action? _uiUpdateMethod;
-    private bool _exitDrawThread;
     private Action? _drawAction;
-    private Thread? _drawThread;
-    private SemaphoreSlim? _drawingSemaphoreSlim;
+    private readonly Timer _drawTimer = new(1000.0 / ScreenRefreshHz);
+    private readonly SemaphoreSlim? _drawingSemaphoreSlim = new(1, 1);
 
     public event EventHandler<KeyboardEventArgs>? KeyUp;
     public event EventHandler<KeyboardEventArgs>? KeyDown;
@@ -138,12 +140,6 @@ public double Scale {
 
     private bool _isDrawThreadInitialized;
 
-    private void DrawThreadMethod() {
-        while (!_exitDrawThread) {
-            _drawAction?.Invoke();
-        }
-    }
-
     [ObservableProperty]
     private Cursor? _cursor = Cursor.Default;
 
@@ -338,32 +334,25 @@ public void ShowColorPalette() {
     [RelayCommand]
     public void ResetTimeMultiplier() => TimeMultiplier = Configuration.TimeMultiplier;
 
-    public void UpdateScreen() {
-        if (_disposed || _isSettingResolution || _isAppClosing || _uiUpdateMethod is null || Bitmap is null || RenderScreen is null) {
+    private void InitializeRenderingThread() {
+        if (_isDrawThreadInitialized || _disposed || _isSettingResolution || _isAppClosing || _uiUpdateMethod is null || Bitmap is null || RenderScreen is null) {
             return;
         }
-        if (!_isDrawThreadInitialized) {
-            _drawThread = new Thread(DrawThreadMethod) {
-                Name = "UIRenderThread"
-            };
-            _drawingSemaphoreSlim = new(1, 1);
-            _drawThread.Start();
-            _isDrawThreadInitialized = true;
-        }
-
-        _drawAction ??= () => {
-            unsafe {
-                _drawingSemaphoreSlim?.Wait();
-                try {
-                    using ILockedFramebuffer pixels = Bitmap.Lock();
-                    var uiRenderEventArgs = new UIRenderEventArgs(pixels.Address, pixels.RowBytes * pixels.Size.Height / 4);
-                    RenderScreen.Invoke(this, uiRenderEventArgs);
-                } finally {
-                    _drawingSemaphoreSlim?.Release();
-                }
-                _uiDispatcher.Post(static () => _uiUpdateMethod.Invoke(), DispatcherPriority.Render);
+        _drawAction = () => {
+            _drawingSemaphoreSlim?.Wait();
+            try {
+                using ILockedFramebuffer pixels = Bitmap.Lock();
+                var uiRenderEventArgs = new UIRenderEventArgs(pixels.Address, pixels.RowBytes * pixels.Size.Height / 4);
+                RenderScreen.Invoke(this, uiRenderEventArgs);
+            } finally {
+                _drawingSemaphoreSlim?.Release();
             }
+            _uiDispatcher.Post(static () => _uiUpdateMethod.Invoke(), DispatcherPriority.Render);
         };
+
+        _drawTimer.Elapsed += (_, _) => _drawAction.Invoke();
+        _drawTimer.Start();
+        _isDrawThreadInitialized = true;
     }
 
     public double MouseX { get; set; }
@@ -453,6 +442,7 @@ public void SetResolution(int width, int height) => _uiDispatcher.Post(() => {
             }
         }
         _isSettingResolution = false;
+        InitializeRenderingThread();
     }, DispatcherPriority.MaxValue);
 
     public event EventHandler<UIRenderEventArgs>? RenderScreen;
@@ -486,10 +476,8 @@ private void Dispose(bool disposing) {
 
     private void DisposeDrawThread() {
         _drawAction = null;
-        _exitDrawThread = true;
-        if (_drawThread?.IsAlive == true) {
-            _drawThread.Join();
-        }
+        _drawTimer.Stop();
+        _drawTimer.Dispose();
         _isDrawThreadInitialized = false;
     }