keypad4x4: fix the row scan, and let the caller set key values and polarity - #896
Open
zombieleet wants to merge 2 commits into
Open
zombieleet wants to merge 2 commits into
zombieleet wants to merge 2 commits into
Conversation
Two problems make GetIndices report the wrong row. The scan drives one row low, reads the columns, and raises the row again. When it finds a key it returns straight away, so the row it was testing stays low. The next scan then has two rows low at once, and a column that reads low no longer says which row it came from. The row is now raised before the return. The columns are also read on the instruction after the row goes low. The line does not fall that fast, because the wiring has some capacitance, so the read can still show the row before it. The scan now waits 50us after a row changes. Together these report a key as the row below it, now and then. Tested on an ESP32 with a 4x4 membrane keypad. Pressing the same key five times gives the same row every time, and pressing down one column gives rows 0, 1, 2 and 3 in order.
Two things the driver cannot guess, and had no way to be told. The value of a key. GetKey returns the position, 0 at the upper left end and 15 at the lower right end. A keypad prints something else on the keys, and only the caller knows what. The device already had a mapping field of the right shape, but nothing could write to it and Configure overwrote it on every call. Config.Mapping now fills it, Configure no longer touches it, and the default is still the position. The polarity of the scan. The columns rest high with a pull-up and each row is driven low. That fails when something on the board pulls a column down harder than the internal pull-up can hold it up. The column then rests low, the scan reads a key in that column on every row, and the driver latches and stops reporting anything at all. Config.Inverted turns the scan around, so the columns rest low and each row is driven high. Config.ColumnConfig gives the pin mode of the columns. Inverted wants an internal pull-down, but not every chip has one. AVR has pull-ups only, so naming PinInputPulldown here would break those targets. The caller passes it on a chip that has it. Nil means PinInputPullup, or PinInput when Inverted is set, which is enough when the board already holds the columns down. NewDevice and the Device interface do not change. NewDevice now calls NewDeviceWithConfig with an empty Config. Tested on an ESP32 with a 4x4 membrane keypad. The board pulls GPIO13 down, so the whole left column of the keypad never worked. With Inverted and a pull-down it reads 1, 4, 7 and * correctly, and the mapping gives those characters rather than 0, 4, 8 and 12.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
I wired a 4x4 membrane keypad to an ESP32 and ran the driver. The keys are
marked
1to9withAtoDdown the right side, as usual.Pressing the key marked
7reported8. Sometimes it reported12. Pressingthe key marked
1reported nothing at all. After I pressed4a few keysstarted 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:
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 instructionafter
rowPin.Low(). The line does not fall that fast, because the wiring hassome 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:
So
inputEnabledstays false and the driver reports nothing ever again. Thatis 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:
boards commonly fit a pull-down. That makes GPIO12 a poor column on a lot of
ESP32 boards, not only mine.
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:NewDeviceand theDeviceinterface do not change.NewDevicenow callsNewDeviceWithConfigwith an emptyConfig.Mappingalso fixes something that was already half there. The device has amappingfield of exactly the right shape, but nothing could write to it andConfigureoverwrote it on every call, soGetKeycould only ever return theposition. The default is still the position.
ColumnConfigis a pointer becausemachine.PinInputPulldowndoes not existon 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
PinModestarts atPinOutputon the ESP32. So the caller passes it:Nil gives
PinInputPullup, orPinInputwhenInvertedis set, which isenough 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:
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:
That whole column had never worked, and the keys come back as characters rather
than 0, 4, 8 and 12.
examples/keypad4x4builds for arduino at both commits, 7350 bytes and 7542bytes.
gofmtandmake fmt-checkpass.One thing worth mentioning separately:
smoketest.sh:112has the keypadexample 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.