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

Implement a QRCode payload generator #46

Merged
merged 7 commits into from
Mar 17, 2020

Conversation

sagar-apple
Copy link
Contributor

Pulling in @bhaskar-apple's work on a QRCode Setup Payload generator

This PR includes code to generate a base45 encoded string given a setup payload.

Here is a brief description of some of the classes / functions -

  1. SetupPayload - A class representing a payload that gets encoded in a QR code.
  2. SetupPayloadGenerator - A class that generates a base45 encoded string given a SetupPayload object
  3. SetupCodeUtils - Utility functions to encode a base45 string.

The encoding of the binary data to a base45 string is done as follows -
Every 2 bytes (16 bits) of binary source data are encoded to 3 characters of the Base-45 alphabet.
If an odd number of bytes are to be encoded, the remaining single byte is encoded to 2 characters of the Base-45 alphabet.

@sagar-apple
Copy link
Contributor Author

@pan-apple @rwalker-apple

Copy link
Contributor Author

@sagar-apple sagar-apple left a comment

Choose a reason for hiding this comment

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

This PR generates code covers stats for a bunch of headers in /usr/include, not sure why. I'm not sure if it's expected.

src/QRCode/SetupPayloadGenerator.cpp Outdated Show resolved Hide resolved
@sagar-apple
Copy link
Contributor Author

Not intentional to generate code cover stats for headers in /usr/include.
My apologies if I somehow managed to do that

Haha no no don't worry :)

src/Makefile Outdated Show resolved Hide resolved
@sagar-apple sagar-apple force-pushed the qrcode branch 2 times, most recently from efc466f to 29026a7 Compare March 12, 2020 19:02

string base45EncodedString(uint64_t input, size_t minLength)
{
static std::map<int, char> BS45_CHARS = base45CharacterMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

do all of our target environments support C++ static initializers?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is the QR generation code. I would expect it to be running as a CL tool used by vendors on desktop class machines and not in the accessory themselves. So I would expect those to support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Along with what Bhaskar said this might be consumed by the commissioner, so I don't necessarily see an issue there either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Devices with displays will likely also run this code. Generally, our policy with respect to OpenWeave is that C++ is fine but STL is generally prohibited unless it is strictly tooling or desktop, mobile, or server-class code only.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is meant to run only on tooling / desktop class code.
I see the use case where the device may have to show the code on a UI. But since the payload is static, I would assume the better approach is to burn the code into the device vs. having logic in the device to generate it from the payload.

@woody-apple woody-apple merged commit 57e0b9f into project-chip:master Mar 17, 2020
fkjagodzinski pushed a commit to fkjagodzinski/connectedhomeip that referenced this pull request Feb 16, 2021
Build Mbed port with Mbed version of mbedtls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants