Added instructions on how to get fingerprint reader to work.#3
Added instructions on how to get fingerprint reader to work.#3jadegamesuk wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR integrates microarray fingerprint driver support into libfprint by adding comprehensive build configuration (meson.build), updating documentation with hardware compatibility and build/test instructions, and refining enrollment slot-selection logic to enforce a 0–9 limit. ChangesMicroarray Driver Integration with Build System and Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/microarray.c (1)
453-458:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDestructive template clearing when slots are full is a data-loss risk.
When slots 0–9 are fully occupied, the code unconditionally erases all stored templates (line 456–457) without warning the user or offering alternatives. This means attempting to enroll an 11th fingerprint silently destroys all 10 existing enrollments, which is unexpected and likely unacceptable for users.
Consider one of these alternatives:
- Return an error indicating storage is full, allowing the caller to decide next steps
- Prompt the user to select which slot to overwrite
- Implement a delete operation separate from enrollment, so users can explicitly free slots before enrolling new prints
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/microarray.c` around lines 453 - 458, The fallback in microarray.c currently nukes all templates when slots 0–9 are full by setting self->fid=0 and sending MA_CMD_EMPTY with ma_submit_cmd, which causes silent data loss; change this behavior to return an explicit error to the caller instead of performing MA_CMD_EMPTY: detect the full condition and return a meaningful error code/enum (e.g., MA_ERR_STORAGE_FULL) from the enclosing function and remove or gate the MA_CMD_EMPTY path; alternatively, if interactive flows exist, replace the unconditional erase with a call to a new confirmation or slot-selection routine so the caller can prompt/choose before calling ma_submit_cmd with MA_CMD_EMPTY (referencing self->fid, MA_CMD_EMPTY, ma_submit_cmd, ssm, device).
🧹 Nitpick comments (4)
src/microarray.c (1)
428-444: ⚡ Quick winClarify loop termination when slot limit is reached.
The
breakon line 434 only exits the inner loop, so the outer loop continues iterating through remaining bytes even after reaching the slot 9 cap. While functionally safe (sincecandidate_slotwill always exceed 9 for subsequent bytes, preventingself->fidfrom being set incorrectly), this results in unnecessary iterations and confusing control flow.♻️ Proposed fix to cleanly exit both loops
Replace the inner
breakwith a jump to the outer loop's exit:if (resp[0] == 0x00) { + gboolean done = FALSE; - for (int byte = 0; byte < 4 && self->fid < 0; byte++) { + for (int byte = 0; byte < 4 && self->fid < 0 && !done; byte++) { for (int bit = 0; bit < 8; bit++) { int candidate_slot = byte * 8 + bit; /* STRICT CAP: Your chip only supports up to slot 9 (10 fingers total) */ if (candidate_slot > 9) { - break; + done = TRUE; + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/microarray.c` around lines 428 - 444, The inner-loop break when candidate_slot > 9 only exits the bit loop and lets the outer byte loop continue unnecessarily; modify the logic to exit both loops as soon as the slot cap is reached by adding a clear control transfer (e.g., set a boolean flag like reached_slot_cap or use a goto to a labeled exit) and check it in the outer loop condition so that once candidate_slot > 9 you immediately stop iterating; adjust around the for loops that compute candidate_slot and set self->fid (using resp and self->fid) to use this single immediate exit approach.README.md (2)
88-99: ⚡ Quick winCaution: pinned
meson.buildcopied over upstream HEAD may drift.Step 6 overwrites the freshly-cloned upstream
libfprint/meson.buildwith this repo's pinned copy. Because step 3 always clones the latest upstream, upstream changes to its build graph (added sources/drivers) will be lost and the build can break. The note on Line 134 partially covers this, but consider pinning a known-good upstream tag/commit in step 3 to keep the copiedmeson.buildin sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 88 - 99, The README currently copies the repo's pinned meson.build over the freshly-cloned upstream libfprint which can drift; update the instructions so that the upstream clone in Step 3 is pinned to a known-good tag or commit instead of always cloning HEAD and losing upstream build changes—modify the Step 3 clone command to checkout a specific tag/commit (referencing the cloned libfprint repo and the meson.build overwrite in Step 6) and add a short note explaining to update the pinned upstream tag when adapting the pinned meson.build.
98-99: ⚡ Quick winHardcoded soname and multiarch path are brittle.
libfprint-2.so.2.0.0and/usr/lib/x86_64-linux-gnuare both hardcoded. Since step 3 clones the latest upstreamlibfprint, a future soversion/version bump will change the built filename (e.g.2.x.y), making thesecpcommands silently copy a stale file or fail. The path also assumes an x86_64 Debian/Ubuntu multiarch layout. Consider deriving the produced filename and target dir dynamically.♻️ Derive the built library and target path
LIB=$(ls libfprint/libfprint-2.so.2.* | head -n1) DEST=$(dirname "$(ldconfig -p | grep -m1 libfprint-2.so.2 | awk '{print $NF}')") sudo cp "$LIB" "$DEST/libfprint-2.so.2" sudo ldconfig🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 98 - 99, Replace the hardcoded soname "libfprint-2.so.2.0.0" and hardcoded multiarch path "/usr/lib/x86_64-linux-gnu" with a dynamic lookup that: finds the produced library filename under libfprint (matching libfprint-2.so.2.*), determines the system's current destination directory for libfprint-2.so.2 via ldconfig/ldconfig -p, and then copies the discovered file to the discovered destination and runs ldconfig; update the README lines that reference libfprint/libfprint-2.so.2.0.0 and /usr/lib/x86_64-linux-gnu accordingly so future version bumps and non-x86_64 multiarch layouts are handled automatically.meson.build (1)
217-224: ⚡ Quick winGuard
supported_drivers += 'microarray'against pre-existing entries.This repo’s
meson.buildonly appends'microarray'tosupported_drivers(no other in-repo mutations), so duplicates won’t occur from within this file. Duplicatefpi_device_microarray_get_type()entries in the generatedfpi-drivers.care only possible ifsupported_driversis provided/populated externally (e.g., by a parent Meson project) and already contains'microarray'.🛡️ Guard the append
-supported_drivers += 'microarray' +if not supported_drivers.contains('microarray') + supported_drivers += 'microarray' +endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meson.build` around lines 217 - 224, Prevent duplicate entries by checking supported_drivers before appending 'microarray': replace the unconditional "supported_drivers += 'microarray'" with a guard that only appends if 'microarray' is not already present in supported_drivers (so you avoid duplicate fpi_device_microarray_get_type() entries in the generated code); reference the supported_drivers variable and the generated fpi_device_microarray_get_type symbol when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/microarray.c`:
- Around line 427-444: The ReadIndex response parsing currently treats any
non-zero resp[0] as "all slots full" and later falls through to
template-clearing; instead, detect resp[0] != 0x00 as a command failure and bail
out explicitly: when parsing the ReadIndex response (using resp, candidate_slot,
bit/byte loops and self->fid), do not leave self->fid as -1 to signal "all full"
on error—either return an error code from the function or set a distinct error
flag/state on self (e.g., fid_error) and log/report the ReadIndex failure so the
later template-clearing fallback is skipped unless you truly know the bitmap
indicates no free slots. Ensure the new branch handles resp[0] != 0x00 before
any logic that assumes a valid bitmap.
---
Outside diff comments:
In `@src/microarray.c`:
- Around line 453-458: The fallback in microarray.c currently nukes all
templates when slots 0–9 are full by setting self->fid=0 and sending
MA_CMD_EMPTY with ma_submit_cmd, which causes silent data loss; change this
behavior to return an explicit error to the caller instead of performing
MA_CMD_EMPTY: detect the full condition and return a meaningful error code/enum
(e.g., MA_ERR_STORAGE_FULL) from the enclosing function and remove or gate the
MA_CMD_EMPTY path; alternatively, if interactive flows exist, replace the
unconditional erase with a call to a new confirmation or slot-selection routine
so the caller can prompt/choose before calling ma_submit_cmd with MA_CMD_EMPTY
(referencing self->fid, MA_CMD_EMPTY, ma_submit_cmd, ssm, device).
---
Nitpick comments:
In `@meson.build`:
- Around line 217-224: Prevent duplicate entries by checking supported_drivers
before appending 'microarray': replace the unconditional "supported_drivers +=
'microarray'" with a guard that only appends if 'microarray' is not already
present in supported_drivers (so you avoid duplicate
fpi_device_microarray_get_type() entries in the generated code); reference the
supported_drivers variable and the generated fpi_device_microarray_get_type
symbol when implementing the check.
In `@README.md`:
- Around line 88-99: The README currently copies the repo's pinned meson.build
over the freshly-cloned upstream libfprint which can drift; update the
instructions so that the upstream clone in Step 3 is pinned to a known-good tag
or commit instead of always cloning HEAD and losing upstream build
changes—modify the Step 3 clone command to checkout a specific tag/commit
(referencing the cloned libfprint repo and the meson.build overwrite in Step 6)
and add a short note explaining to update the pinned upstream tag when adapting
the pinned meson.build.
- Around line 98-99: Replace the hardcoded soname "libfprint-2.so.2.0.0" and
hardcoded multiarch path "/usr/lib/x86_64-linux-gnu" with a dynamic lookup that:
finds the produced library filename under libfprint (matching
libfprint-2.so.2.*), determines the system's current destination directory for
libfprint-2.so.2 via ldconfig/ldconfig -p, and then copies the discovered file
to the discovered destination and runs ldconfig; update the README lines that
reference libfprint/libfprint-2.so.2.0.0 and /usr/lib/x86_64-linux-gnu
accordingly so future version bumps and non-x86_64 multiarch layouts are handled
automatically.
In `@src/microarray.c`:
- Around line 428-444: The inner-loop break when candidate_slot > 9 only exits
the bit loop and lets the outer byte loop continue unnecessarily; modify the
logic to exit both loops as soon as the slot cap is reached by adding a clear
control transfer (e.g., set a boolean flag like reached_slot_cap or use a goto
to a labeled exit) and check it in the outer loop condition so that once
candidate_slot > 9 you immediately stop iterating; adjust around the for loops
that compute candidate_slot and set self->fid (using resp and self->fid) to use
this single immediate exit approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6268dda3-139f-405c-9d6d-2bbab19cd96f
📒 Files selected for processing (3)
README.mdmeson.buildsrc/microarray.c
| if (resp[0] == 0x00) { | ||
| for (int byte = 0; byte < 4 && self->fid < 0; byte++) { | ||
| for (int bit = 0; bit < 8; bit++) { | ||
| int candidate_slot = byte * 8 + bit; | ||
|
|
||
| /* STRICT CAP: Your chip only supports up to slot 9 (10 fingers total) */ | ||
| if (candidate_slot > 9) { | ||
| break; | ||
| } | ||
|
|
||
| /* If this bit is 0, the slot is empty! Let's claim it. */ | ||
| if (!(resp[1 + byte] & (1 << bit))) { | ||
| self->fid = byte * 8 + bit; | ||
| self->fid = candidate_slot; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling for ReadIndex command failure.
If resp[0] != 0x00 (indicating the ReadIndex command failed), the code skips bitmap parsing, leaving self->fid = -1. It then falls through to the template-clearing fallback (lines 453–458), treating a command failure the same as "all slots full." This conflation could trigger an unintended data wipe when the real problem is a transient device error.
🛡️ Proposed fix to handle command failure explicitly
if (resp[0] == 0x00) {
for (int byte = 0; byte < 4 && self->fid < 0; byte++) {
for (int bit = 0; bit < 8; bit++) {
int candidate_slot = byte * 8 + bit;
/* STRICT CAP: Your chip only supports up to slot 9 (10 fingers total) */
if (candidate_slot > 9) {
break;
}
/* If this bit is 0, the slot is empty! Let's claim it. */
if (!(resp[1 + byte] & (1 << bit))) {
self->fid = candidate_slot;
break;
}
}
}
+ } else {
+ /* ReadIndex command failed */
+ fp_warn ("ReadIndex command failed with status 0x%02x", resp[0]);
+ fpi_ssm_mark_failed (ssm,
+ fpi_device_error_new_msg (FP_DEVICE_ERROR_PROTO,
+ "Failed to read enrollment index: 0x%02x",
+ resp[0]));
+ return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/microarray.c` around lines 427 - 444, The ReadIndex response parsing
currently treats any non-zero resp[0] as "all slots full" and later falls
through to template-clearing; instead, detect resp[0] != 0x00 as a command
failure and bail out explicitly: when parsing the ReadIndex response (using
resp, candidate_slot, bit/byte loops and self->fid), do not leave self->fid as
-1 to signal "all full" on error—either return an error code from the function
or set a distinct error flag/state on self (e.g., fid_error) and log/report the
ReadIndex failure so the later template-clearing fallback is skipped unless you
truly know the bitmap indicates no free slots. Ensure the new branch handles
resp[0] != 0x00 before any logic that assumes a valid bitmap.
1 participant
Made quite a few changes to ensure that libfprint works with this USB fingerprint as well as multiple fingerprints as well.
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes