Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matrix keyboard module #3631

Open
wants to merge 6 commits into
base: dev-esp32
Choose a base branch
from
Open

Matrix keyboard module #3631

wants to merge 6 commits into from

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Feb 4, 2024

Fixes #3630

  • This PR is for the dev branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

This adds the matrix module which supports simple matrix keypads. Simple interface -- it just calls a specified callback on key press (and/or key release).

@pjsg pjsg requested a review from jmattsson April 27, 2024 22:02
int character = -1;

// We are either waiting for a negative edge (keypress) or a positive edge
// (keyrelease)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is confusing me - we just disabled row interrupts, so why would we be waiting for an edge?

QUEUE_IT(d, character);
if (!d->task_queued) {
if (task_post_medium(tasknumber, (task_param_t)d)) {
d->task_queued = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe d->task_queued needs to be mutex protected, as it gets written by both the GPIO ISR/task as well as the NodeMCU task.

if (*cb_ptr != LUA_NOREF) {
luaL_unref(L, LUA_REGISTRYINDEX, *cb_ptr);
*cb_ptr = LUA_NOREF;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be neater with luaL_unref2() instead.


typedef struct {
int32_t character; // 1 + character for press, -1 - character for release
uint32_t time_us;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field never appears to be filled in, but is used in the callback.


if (!HAS_QUEUED_DATA(d)) {
luaL_unref(L, LUA_REGISTRYINDEX, d->self_ref);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to change this to a task_post so we only have a single place where we release the self_ref, for easier reasoning.

callback_free(L, d, MATRIX_ALL);

if (matrix_close( d )) {
return luaL_error( L, "Unable to close switch." );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch?

@@ -0,0 +1,77 @@
# matrix Module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a better name be matrixkdb even? There are many types of matrices, I mean.

@jmattsson
Copy link
Member

Ah, one other review comment - the documentation needs to be linked in mkdocs.yml too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants