From e1cd6bc09be4615c856eb204d73b1783893327a7 Mon Sep 17 00:00:00 2001
From: hap <happppp@users.noreply.github.com>
Date: Thu, 6 Jul 2023 15:16:20 +0200
Subject: k005289,k051649: pre instead of postincrement voice counter to
 prevent confusion, k051649: fix possible array overflow when reading voice
 counter, msx scc: fix test register offset

---
 src/devices/bus/msx/cart/konami.cpp |  4 ++--
 src/devices/sound/k005289.cpp       |  6 +++---
 src/devices/sound/k005289.h         | 12 +++++++-----
 src/devices/sound/k051649.cpp       | 35 +++++++++++++++++++++--------------
 src/devices/sound/k051649.h         |  3 ++-
 5 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/src/devices/bus/msx/cart/konami.cpp b/src/devices/bus/msx/cart/konami.cpp
index 6675d37a876..58518dd4ef5 100644
--- a/src/devices/bus/msx/cart/konami.cpp
+++ b/src/devices/bus/msx/cart/konami.cpp
@@ -139,8 +139,8 @@ std::error_condition msx_cart_konami_scc_device::initialize_cartridge(std::strin
 	m_scc_view[1].install_write_handler(0x9880, 0x9889, 0, 0x0710, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_frequency_w)));
 	m_scc_view[1].install_write_handler(0x988a, 0x988e, 0, 0x0710, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_volume_w)));
 	m_scc_view[1].install_write_handler(0x988f, 0x988f, 0, 0x0710, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_keyonoff_w)));
-	m_scc_view[1].install_read_handler(0x98c0, 0x98c0, 0, 0x071f, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_test_r)));
-	m_scc_view[1].install_write_handler(0x98c0, 0x98c0, 0, 0x071f, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_test_w)));
+	m_scc_view[1].install_read_handler(0x98e0, 0x98e0, 0, 0x071f, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_test_r)));
+	m_scc_view[1].install_write_handler(0x98e0, 0x98e0, 0, 0x071f, 0, emu::rw_delegate(m_k051649, FUNC(k051649_device::k051649_test_w)));
 	page(2)->install_read_bank(0x8000, 0x9fff, m_rombank[2]);
 	page(2)->install_write_handler(0x9000, 0x97ff, emu::rw_delegate(*this, FUNC(msx_cart_konami_scc_device::bank_w<2>)));
 	page(2)->install_read_bank(0xa000, 0xbfff, m_rombank[3]);
diff --git a/src/devices/sound/k005289.cpp b/src/devices/sound/k005289.cpp
index f88f12c43f1..4202ce8207c 100644
--- a/src/devices/sound/k005289.cpp
+++ b/src/devices/sound/k005289.cpp
@@ -68,9 +68,9 @@ void k005289_device::device_start()
 	m_stream = stream_alloc(0, 1, clock());
 
 	/* reset all the voices */
-	for (auto & elem : m_voice)
+	for (auto & voice : m_voice)
 	{
-		elem.reset();
+		voice.reset();
 	}
 
 	save_item(STRUCT_MEMBER(m_voice, counter));
@@ -93,7 +93,7 @@ void k005289_device::sound_stream_update(sound_stream &stream, std::vector<read_
 		for (int i = 0; i < 2; i++)
 		{
 			voice_t &v = m_voice[i];
-			if ((v.counter--) <= 0)
+			if (--v.counter < 0)
 			{
 				v.waveform = (v.waveform & ~0x1f) | ((v.waveform + 1) & 0x1f);
 				v.counter = v.frequency;
diff --git a/src/devices/sound/k005289.h b/src/devices/sound/k005289.h
index cacf1d98f52..618c8dd7ac7 100644
--- a/src/devices/sound/k005289.h
+++ b/src/devices/sound/k005289.h
@@ -34,6 +34,8 @@ private:
 
 	struct voice_t
 	{
+		voice_t() { reset(); }
+
 		void reset()
 		{
 			counter = 0;
@@ -43,11 +45,11 @@ private:
 			volume = 0;
 		}
 
-		s16 counter = 0;
-		u16 frequency = 0;
-		u16 pitch = 0;
-		u16 waveform = 0;
-		u8 volume = 0;
+		s16 counter;
+		u16 frequency;
+		u16 pitch;
+		u16 waveform;
+		u8 volume;
 	};
 	voice_t m_voice[2];
 };
diff --git a/src/devices/sound/k051649.cpp b/src/devices/sound/k051649.cpp
index 5ddffc5f36f..e98ad04d6d9 100644
--- a/src/devices/sound/k051649.cpp
+++ b/src/devices/sound/k051649.cpp
@@ -22,6 +22,10 @@
     K052539 is more or less equivalent to this chip except channel 5
     does not share waveram with channel 4.
 
+    TODO:
+    - make K052539 a subdevice
+    - test register bits 0-4, not used in any software
+
 ***************************************************************************/
 
 #include "emu.h"
@@ -136,7 +140,7 @@ void k051649_device::sound_stream_update(sound_stream &stream, std::vector<read_
 			// channel is halted for freq < 9
 			if (voice.frequency > 8)
 			{
-				if ((voice.clock--) <= 0)
+				if (--voice.clock < 0)
 				{
 					voice.counter = (voice.counter + 1) & 0x1f;
 					voice.clock = voice.frequency;
@@ -145,6 +149,8 @@ void k051649_device::sound_stream_update(sound_stream &stream, std::vector<read_
 				if (voice.key)
 					outputs[0].add_int(i, (voice.waveram[voice.counter] * voice.volume) >> 4, 1024);
 			}
+			else if (voice.clock > 0)
+				voice.clock--;
 		}
 	}
 }
@@ -174,17 +180,19 @@ void k051649_device::k051649_waveform_w(offs_t offset, u8 data)
 
 u8 k051649_device::k051649_waveform_r(offs_t offset)
 {
-	// test-register bits 6/7 expose the internal counter
+	u8 counter = 0;
+
+	// test register bits 6/7 expose the internal counter
 	if (m_test & 0xc0)
 	{
 		m_stream->update();
 
-		if (offset >= 0x60)
-			offset += m_channel_list[3 + (m_test >> 6 & 1)].counter;
+		if (offset >= 0x60 && (m_test & 0xc0) != 0xc0)
+			counter = m_channel_list[3 + (m_test >> 6 & 1)].counter;
 		else if (m_test & 0x40)
-			offset += m_channel_list[offset >> 5].counter;
+			counter = m_channel_list[offset >> 5].counter;
 	}
-	return m_channel_list[offset >> 5].waveram[offset & 0x1f];
+	return m_channel_list[offset >> 5].waveram[(offset + counter) & 0x1f];
 }
 
 
@@ -201,13 +209,15 @@ void k051649_device::k052539_waveform_w(offs_t offset, u8 data)
 
 u8 k051649_device::k052539_waveform_r(offs_t offset)
 {
-	// test-register bit 6 exposes the internal counter
+	u8 counter = 0;
+
+	// test register bit 6 exposes the internal counter
 	if (m_test & 0x40)
 	{
 		m_stream->update();
-		offset += m_channel_list[offset >> 5].counter;
+		counter = m_channel_list[offset >> 5].counter;
 	}
-	return m_channel_list[offset >> 5].waveram[offset & 0x1f];
+	return m_channel_list[offset >> 5].waveram[(offset + counter) & 0x1f];
 }
 
 
@@ -225,15 +235,12 @@ void k051649_device::k051649_frequency_w(offs_t offset, u8 data)
 
 	m_stream->update();
 
-	// test-register bit 5 resets the internal counter
+	// test register bit 5 resets the internal counter
 	if (m_test & 0x20)
 	{
-		m_channel_list[offset].counter = 0;
+		m_channel_list[offset].counter = 0x1f;
 		m_channel_list[offset].clock = 0;
 	}
-	// TODO: correct?
-	else if (m_channel_list[offset].frequency < 9)
-		m_channel_list[offset].clock = 0;
 
 	// update frequency
 	if (freq_hi)
diff --git a/src/devices/sound/k051649.h b/src/devices/sound/k051649.h
index d9174e00dc9..1613cdfcfd8 100644
--- a/src/devices/sound/k051649.h
+++ b/src/devices/sound/k051649.h
@@ -30,6 +30,7 @@ public:
 	u8   k052539_waveform_r(offs_t offset);
 
 	void scc_map(address_map &map);
+
 protected:
 	// device-level overrides
 	virtual void device_start() override;
@@ -55,7 +56,7 @@ private:
 		}
 
 		u8 counter;     // address counter for wavetable
-		u32 clock;      // internal clock
+		s16 clock;      // internal clock
 		u16 frequency;  // frequency; result: (input clock / (32 * (frequency + 1)))
 		int volume;     // volume
 		bool key;       // keyon/off
-- 
cgit v1.2.3