Verilog: Regression: player 2 keys control player 1 instead of player 2

[ewenmcneill]

In the Verilog simulation, it appears the keys for player 1 (arrow keys, space/shift) and the keys for player 2 (a, d, w, s, z, x) both actually control player 1, at least experimentally. Ie, both seem to only affect the horizontal vars output (ie, based on vertical position). (I think this is an accidental regression; see comments at the end.)

8bitworkshop/presets/verilog/switches.v

Lines 7 to 8 in 03af8c2

	Player 1 Keys: arrow keys + space + shift
	Player 2 Keys: A/D/W/S + Z + X

8bitworkshop/presets/verilog/switches.v

Lines 33 to 40 in 03af8c2

	// select p1 bit based on vertical position
	wire p1gfx = switches_p1[vpos[7:5]];
	// select p2 bit based on horizontal position
	wire p2gfx = switches_p2[hpos[7:5]];
	assign rgb = {1'b0,
	display_on && p1gfx,
	display_on && p2gfx};

This also appears to be true when using the sprite rotation example, but fortunately that one only relies on the player 1 controls (but can be controlled with both "player 1" and "player 2" controls equivalently).

8bitworkshop/presets/verilog/sprite_rotation.v

Lines 400 to 402 in 03af8c2

	.switch_left(switches_p1[0]),
	.switch_right(switches_p1[1]),
	.switch_up(switches_p1[2])

As best I can tell the problem is in platform/verilog.ts in the table passed to makeKeycodeMap():

8bitworkshop/src/platform/verilog.ts

Lines 43 to 61 in 03af8c2

	var VERILOG_KEYCODE_MAP = makeKeycodeMap([
	[Keys.LEFT, 0, 0x1],
	[Keys.RIGHT, 0, 0x2],
	[Keys.UP, 0, 0x4],
	[Keys.DOWN, 0, 0x8],
	[Keys.A, 0, 0x10],
	[Keys.B, 0, 0x20],
	[Keys.P2_LEFT, 0, 0x1],
	[Keys.P2_RIGHT, 0, 0x2],
	[Keys.P2_UP, 0, 0x4],
	[Keys.P2_DOWN, 0, 0x8],
	[Keys.P2_A, 0, 0x10],
	[Keys.P2_B, 0, 0x20],
	[Keys.START, 2, 0x1],
	[Keys.P2_START, 2, 0x2],
	[Keys.SELECT, 2, 0x4],
	[Keys.P2_SELECT, 2, 0x8],
	[Keys.VK_7, 2, 0x10],
	]);

It seems like the second parameter in each row should be the player number or similar, but in platform/verilog.ts both the player 1 keys and the player 2 keys are set to 0 (ie, player 1).

Compare with, eg, platforms/nes.ts which matches the player 1 keys with a 1 and the player 2 keys with a 2:

8bitworkshop/src/platform/nes.ts

Lines 49 to 67 in 03af8c2

	const JSNES_KEYCODE_MAP = makeKeycodeMap([
	[Keys.A, 0, 0],
	[Keys.B, 0, 1],
	[Keys.SELECT, 0, 2],
	[Keys.START, 0, 3],
	[Keys.UP, 0, 4],
	[Keys.DOWN, 0, 5],
	[Keys.LEFT, 0, 6],
	[Keys.RIGHT, 0, 7],
	[Keys.P2_A, 1, 0],
	[Keys.P2_B, 1, 1],
	[Keys.P2_SELECT, 1, 2],
	[Keys.P2_START, 1, 3],
	[Keys.P2_UP, 1, 4],
	[Keys.P2_DOWN, 1, 5],
	[Keys.P2_LEFT, 1, 6],
	[Keys.P2_RIGHT, 1, 7],
	]);

as does Galaxian:

8bitworkshop/src/platform/galaxian.ts

Lines 12 to 22 in 03af8c2

	const GALAXIAN_KEYCODE_MAP = makeKeycodeMap([
	[Keys.A, 0, 0x10], // P1
	[Keys.LEFT, 0, 0x4],
	[Keys.RIGHT, 0, 0x8],
	[Keys.P2_A, 1, 0x10], // P2
	[Keys.P2_LEFT, 1, 0x4],
	[Keys.P2_RIGHT, 1, 0x8],
	[Keys.SELECT, 0, 0x1],
	[Keys.START, 1, 0x1],
	[Keys.VK_2, 1, 0x2],
	]);

From the definition of makeKeycodeMap(), I think it confirms the second item in each row is the index (eg, into the switches array), so that'd confirm that player 1 should be 0 and player 2 should be 1:

8bitworkshop/src/common/emu.ts

Lines 510 to 523 in 03af8c2

	export function makeKeycodeMap(table : [KeyDef,number,number][]) : KeyCodeMap {
	var map = new Map<number,KeyMapEntry>();
	for (var i=0; i<table.length; i++) {
	var entry = table[i];
	var val : KeyMapEntry = {index:entry[1], mask:entry[2], def:entry[0]};
	map[entry[0].c] = val;
	}
	return map;
	}
	const DEFAULT_CONTROLLER_KEYS : KeyDef[] = [
	Keys.UP, Keys.DOWN, Keys.LEFT, Keys.RIGHT, Keys.A, Keys.B, Keys.SELECT, Keys.START,
	Keys.P2_UP, Keys.P2_DOWN, Keys.P2_LEFT, Keys.P2_RIGHT, Keys.P2_A, Keys.P2_B, Keys.P2_SELECT, Keys.P2_START,
	];

AFAICT the fix would just be to change the P2_* keys rows from index 0 to index 1, ie in these lines:

8bitworkshop/src/platform/verilog.ts

Lines 50 to 55 in 03af8c2

	[Keys.P2_LEFT, 0, 0x1],
	[Keys.P2_RIGHT, 0, 0x2],
	[Keys.P2_UP, 0, 0x4],
	[Keys.P2_DOWN, 0, 0x8],
	[Keys.P2_A, 0, 0x10],
	[Keys.P2_B, 0, 0x20],

And that appears to be the way that it was before the most recent change (to "standardize keys"), so my guess is that this is a cut'n'paste regression that was just overlooked in testing that change.

Ewen

PS: Also for anyone finding this, you have to click on the output video screen to ensure the input focus goes there for the keys to work; that was non-obvious initially. (Otherwise the input focus is, eg, the source editor, and pressing keys in that doesn't control the running program :-) )