keypad4x4: fix the row scan, and let the caller set key values and polarity

[zombieleet]

The problem

I wired a 4x4 membrane keypad to an ESP32 and ran the driver. The keys are
marked 1 to 9 with A to D down the right side, as usual.

Pressing the key marked 7 reported 8. Sometimes it reported 12. Pressing
the key marked 1 reported nothing at all. After I pressed 4 a few keys
started working, and then the driver went silent and stayed silent.

Three separate causes, and only the last one is about my board.

The row is left driven when a key is found. The scan drives one row low,
reads the columns, and raises the row again at the end of the loop. Finding a
key returns before that:

for rowIndex, rowPin := range keypad.rows {
	rowPin.Low()
	for columnIndex := range keypad.columns {
		if !columnPin.Get() && keypad.inputEnabled {
			...
			return keypad.lastRow, keypad.lastColumn   // rowPin.High() skipped
		}
	}
	rowPin.High()      // only reached when nothing was pressed
}

Every hit leaves that row low. The next scan then has two rows low at once, and
a column reading low no longer says which row it came from.

The columns are read too early. columnPin.Get() runs on the instruction
after rowPin.Low(). The line does not fall that fast, because the wiring has
some capacitance, so the read can still show the row before it. This is what
reports a key as the row below it.

A column that cannot rest high. The columns rest high with a pull-up and
each row is driven low. On my board something pulls GPIO13 down with about
10k, and the internal pull-up is about 45k, so the pull-down wins. GPIO13 rests
low with no key pressed, the scan sees a key in column 0 on every row, and then
this never becomes true again:

if columnPin.Get() &&                    // the column must go high to re-arm
	columnIndex == keypad.lastColumn &&
	rowIndex == keypad.lastRow &&
	!keypad.inputEnabled {
	keypad.inputEnabled = true
}

So inputEnabled stays false and the driver reports nothing ever again. That
is the "press 1, nothing, press 4, a few work, then silence" part. Every value
I saw was a multiple of 4, which is column 0 on each row.

Why the third one is worth an option

A pull-up scan is the usual convention and it is right for most boards. But a
column cannot rest high wherever the pin already carries an external pull-down,
and that is not rare:

• Strapping pins. GPIO12 on the ESP32 is MTDI and has to be low at boot, so
  boards commonly fit a pull-down. That makes GPIO12 a poor column on a lot of
  ESP32 boards, not only mine.
• Pins shared with something on the board. Cameras, SD slots, displays.
• Keypad modules that bring their own resistors. Those break a pull-up scan
  on any board, including a bare one.

There is no way to tell the driver about any of this.

What this changes

Two commits. The first fixes the scan and touches nothing else, so it can be
read on its own.

The second adds Config:

type Config struct {
	Mapping      [4][4]uint8         // the value of each key
	Inverted     bool                // columns rest low, rows are driven high
	ColumnConfig *machine.PinConfig  // the pin mode of the columns
}

func NewDeviceWithConfig(config Config, r4, r3, r2, r1, c4, c3, c2, c1 machine.Pin) Device

NewDevice and the Device interface do not change. NewDevice now calls
NewDeviceWithConfig with an empty Config.

Mapping also fixes something that was already half there. The device has a
mapping field of exactly the right shape, but nothing could write to it and
Configure overwrote it on every call, so GetKey could only ever return the
position. The default is still the position.

ColumnConfig is a pointer because machine.PinInputPulldown does not exist
on every chip. AVR has pull-ups only, and naming that constant in the driver
breaks the arduino build. A zero value cannot mean "unset" either, because
PinMode starts at PinOutput on the ESP32. So the caller passes it:

pulldown := machine.PinConfig{Mode: machine.PinInputPulldown}
cfg := keypad4x4.Config{Inverted: true, ColumnConfig: &pulldown}

Nil gives PinInputPullup, or PinInput when Inverted is set, which is
enough when the board already holds the columns down.

Testing

ESP32-WROVER with a 4x4 membrane keypad, on the pins from the Freenove lesson:
rows GPIO14, 27, 26, 25 and columns GPIO13, 21, 22, 23.

Default path, on the three columns that work on this board. The same key five
times, then down one column:

index 9   row 2  col 1     index 9   row 2  col 1
index 9   row 2  col 1     index 1   row 0  col 1
index 9   row 2  col 1     index 5   row 1  col 1
index 9   row 2  col 1     index 9   row 2  col 1
index 9   row 2  col 1     index 13  row 3  col 1

Same row every time, and rows 0 to 3 in order down the column.

Inverted, with a pull-down and a mapping, on all four columns including GPIO13:

1 : key 1
2 : key 4
3 : key 7
4 : key *

That whole column had never worked, and the keys come back as characters rather
than 0, 4, 8 and 12.

examples/keypad4x4 builds for arduino at both commits, 7350 bytes and 7542
bytes. gofmt and make fmt-check pass.

One thing worth mentioning separately: smoketest.sh:112 has the keypad
example commented out, and it was already commented before the 2023 change that
moved the smoke tests into that file. It builds today. Happy to enable it here
or leave it alone.

[deadprogram]

Thanks for the detailed write-up and the testing on real hardware. Both scan fixes look right, and the new Config keeps NewDevice backward compatible.

A few requests:

1. Please shorten the comments. This repo keeps code comments to 1 or 2 lines. The Config field docs and the comment in GetIndices can be cut down a lot. The comment in NewDeviceWithConfig about what Configure used to do can go. The PR description and commit messages already cover that history.

2. About the settle delay: the selected row is driven low, so it falls fast. The slow edge is the column rising back through the pull-up after the previous row is released. The delay still helps, but 50us is more than needed. Something like 10us should be enough. On some targets time.Sleep with short durations can also sleep much longer than asked, and it runs 4 times per scan.

3. Please add a line to the Mapping doc saying that a key mapped to 255 can't be told apart from NoKeyPressed.

4. Yes, please enable the keypad example in smoketest.sh.

[zombieleet]

Thanks for the comment @deadprogram. Made the changes.