Replace quirc and BC-UR libraries with lighter pure-C implementations#303
Replace quirc and BC-UR libraries with lighter pure-C implementations#303odudex wants to merge 5 commits into
Conversation
|
Hi @odudex awesome stuff! I have been irked by bc-ur for a long time (we also use it in gdk) - .the c++ impl is inefficient and unwieldy to work with, and the upstream maintainers are resistant to any kind of changes. Its long been a dream of mine to have time to rewrite it as clean C, so its great to see you've done exactly that :) The code size reduction and performance improvements are also very welcome, so I'm very interested in merging these changes. Swapping from our current impl would require us to fully review/test both libraries, which will take a while, and we'd almost certainly do this a library at a time, starting with bc-ur. So please bear with us as we review cUR first and then we will look at migrating to it in due course. |
|
Note I've cherry-picked the libjade change into master so you don't have to keep carrying it. |
|
Great to hear there's a demand for cUR! Your review and feedback are really welcome, and you can count on me to make adjustments if needed. |
|
I'd say for the purposes of keeping this more easily rebase-able, it would be a good idea to structure it as the following:
I'd leave the removal of the existing components out for now since nuking them after the migration is trivial, and rebasing with them deleted will be painful if there are changes there while this is under review. |
|
Commits structured as requested. |
| static bool ur_decoder_is_failure(ur_decoder_t* decoder) | ||
| { | ||
| return ur_decoder_is_complete(decoder) && !ur_decoder_is_success(decoder); | ||
| } |
There was a problem hiding this comment.
(Note this is a design nit from the reference impl and not a bug)
The reference API is pretty confusing I think, and could be made better in your impl. Given the following:
- is complete is either true or false, but an error can occur in either state, and if the error is fetched when incomplete it will return UR_DECODER_OK (even if the decoder is null, despite having an error code available for that)
- is success is either true or false but can only be true when is complete is true and if its false before then it doesn't mean the decode was not successful. Also no error when null as above.
- error code (get_last_error) handles null, but can also return OK when neither is complete or is success are true AFAICS.
I think a better design is to replace ur_decoder_is_complete/is_success/get_last_error with ur_decode_get_state(). The states being:
- OK: finished without error and a result is available (caller can call get_result() and expect a non-null return)
- PROCESSING: not finished, and no error yet encountered. Keep feeding the decoder more data.
- NO_RESULT: finished without decoding error but there is no result content (caller decides if this is an error condition or not)
- an error (i.e the already existing error codes)
The goal of such an API change would be that the caller is less likely to have logic errors (it would simplify the Jade code for example)
There was a problem hiding this comment.
The reference API is pretty confusing I think, and could be made better in your impl.
Thank you! I focused on performance optimizations but forgot to refine API from early sketches. Now is the time to make it more consistent and intuitive. I'll refactor it and let you know when I have something better!
There was a problem hiding this comment.
On the subject of performance, for esp32 float operations are much faster/smaller than double (float is cpu native while some double operations are emulated and require linking a support library). The same is probably not true on modern desktop/laptop processors however. It would be nice if the double type could be chosen at compile time (conditionally defining a ur_float_t typedef, for example).
I'm not immediately sure if this would change the resulting encoded data however. TBH I have no idea why this spec includes a floating point based sampler at all, it seems like completely useless overkill.
There was a problem hiding this comment.
On the subject of performance, for esp32
floatoperations are much faster/smaller thandouble
Right! I'll evaluate all number variables, and simplify to float or integer math wherever possible.
Add cUR (https://github.com/odudex/cUR), a pure-C implementation of BCR-2020-005 UR encoding, pinned at the head of its main branch. No other changes - the source migration follows in the next commit.
Move the UR transport layer from the C++20 bc-ur library (and its C shim with placement-new sizing constants) to cUR, a pure-C implementation of BCR-2020-005, used as the UR transport envelope only - all payload CBOR remains TinyCBOR in bcur.c. - main/bcur.c and main/selfcheck.c moved to the cUR API: heap encoder/decoder handles, results borrowed from the decoder, fragments freed with free(). Encoder output is always uppercase. - collect_any_bcur() now replaces the decoder via qr_data->ctx on hard failure, and bcur_scan_qr() re-reads it after scanning. - selfcheck: a duplicate part is deduped before being counted, so the 'processed parts' expectation differs when the same part is presented twice. - Build with UR_ENVELOPE_ONLY to skip cUR's payload-type codecs; libjade links the same component's host static lib (bundled SHA-256, no mbedcrypto/wally dependencies). The esp32_bc-ur component is left in place (now unused) so it can be removed separately once the migration has settled.
Drive the animated-QR progress bar with ur_decoder_estimated_percent_complete_weighted() instead of the pure received/expected fragment counts. The weighted estimate also credits mixed fountain parts that have not yet resolved a pure fragment, so progress keeps moving where the count-based bar would stall at 'almost done' until the final reconstruction. Capped at 99% until the decode actually completes.
Add k_quirc (https://github.com/odudex/k_quirc), a rewritten ESP32-optimized QR decoder (bilinear/adaptive thresholding, span-based flood fill, SPIRAM-aware allocation), pinned at the head of its master branch. No other changes - the source migration follows in the next commit.
Rewrite the thin wrapper in main/qrscan.c to the k_quirc_* API, folding quirc_extract + quirc_decode into a single k_quirc_decode() and dropping the caller-allocated datastream scratch buffer (now managed internally). k_quirc_end() is called with find_inverted=false to preserve current behavior. Downstream callers use the qr_data_t / jade_camera_scan_qr / scan_qr wrapper API and are unaffected. Also update the three build references: main/CMakeLists.txt PRIV_REQUIRES, libjade include dirs, and the libjade.c amalgamated .c includes. The vendored esp32-quirc component is left in place (now unused) so it can be removed separately once the migration has settled.
|
Done: cUR:
Jade:
|
2 participants
This PR proposes, for your evaluation, replacing two libraries in the QR scan/display path:
components/esp32-quirc→ k_quirc submodule — QR decodingcomponents/esp32_bc-ur(C++) → cUR (C) submodule — BC-UR transport (bytewords, fountain codes, multi-part assembly)Both were originally developed for Kern and Krux projects. cUR is used envelope-only: all payload CBOR stays in TinyCBOR in
main/bcur.c, andmain/bcur.his unchanged, so no callers needed modification. A third small commit makes the libjadeESP_LOGmocks accept non-literal tags (k_quirc logs with aTAGvariable).Measured impact (esp32s3 dev config, vs master)
jade.binlibstdc++qr_qvga_*camera fixtures (desktop benchmark feeding the same 220×220 scan input to both decoders at-O2; per-image speedups 1.9–3.2×, decode parity 13/13).Compatibility and verification
UR:CRYPTO-PSBTparts, wire-format parity with bc-ur. Fountain fragments are additionally cross-validated in cUR's own test suite against foundation-ur-py output.processed_partsstays at 1 when the same part is presented twice (bc-ur counted 2).test_jade.pypasses via libjade, including the multi-partUR:CRYPTO-PSBTcamera-scan fixture.--psram --unamalgamated).Caveats
Motivation
If all goes well, give back from DIY community to the Jade project and at the same time get reviews to help harden these tools.