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 QR Code Parser Service #5134

Merged
merged 48 commits into from
Apr 26, 2022
Merged

Conversation

KrishnaIyer
Copy link
Member

@KrishnaIyer KrishnaIyer commented Jan 24, 2022

Summary

Implement QR Code Parser Service. Closes #4845 .

Changes

  • Refactor qrcode and qrcodegenerator package.
  • Add a new EntityOnboardingData message with EndDeviceOnboardingData as the first type.
  • Add a QRCodeParser service an implement the server.
  • Adapt DCS to dial QRG.
  • Adjust tests.

Testing

Unit tested.

Regressions

I don't expect any.

Notes for Reviewers

  1. The QRG will now be the single point of truth for QR code formats. So I've moved the qrcode package into qrcodegenerator, so that components don't call the functions separately but dial QRG instead.
  2. I don't see a use case where someone wants to parse a QR Code via the CLI; so I haven't added a command for this. We also don't claim using QR codes in the CLI. However, if we need a command for completeness, I can add that.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@KrishnaIyer KrishnaIyer added this to the v3.17.2 milestone Jan 24, 2022
@KrishnaIyer KrishnaIyer self-assigned this Jan 24, 2022
@github-actions github-actions bot added the compat/api This could affect API compatibility label Jan 24, 2022
@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch from 96106bc to 744d675 Compare January 24, 2022 12:36
@KrishnaIyer KrishnaIyer marked this pull request as ready for review January 24, 2022 13:23
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Reviewed API only for now

CHANGELOG.md Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
api/qrcodegenerator.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@nicholaspcr nicholaspcr left a comment

Choose a reason for hiding this comment

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

The DCS section LGTM.

@KrishnaIyer KrishnaIyer modified the milestones: v3.17.2, v3.18.0 Jan 27, 2022
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

Only looked at api. Not much to add on top of @johanstokking's comments at this point.

@KrishnaIyer KrishnaIyer changed the base branch from v3.17 to v3.18 February 7, 2022 14:05
@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch from 33e0f66 to e7979ef Compare February 7, 2022 14:05
@KrishnaIyer KrishnaIyer modified the milestones: v3.18.0, v3.18.1 Feb 15, 2022
@KrishnaIyer
Copy link
Member Author

@johanstokking Regarding the comment on EndDeviceTemplate.

Step 1 of the new onboarding flow says; #4847 (comment) (line numbers are mine)

a. Scan QR code: this contains at least the JoinEUI and DevEUI, potentially also the numeric Vendor and Profile ID and the Claim Authentication Code. Call the QRCodeParser.Parse() rpc (https://github.com/TheThingsNetwork/lorawan-stack/issues/4845)
b. Put the JoinEUI and DevEUI in the onboarding state
c. If the QR code contains a claim authentication code, put this in the onboarding state
d. If the QR code contains a non-zero Vendor ID and Profile ID, lookup the LoRaWAN device profile (https://github.com/TheThingsNetwork/lorawan-stack/issues/4842) and put this in the onboarding state. Otherwise, the LoRaWAN device profile is empty, but the activation mode can be preset to OTAA (ABP devices won't have QR codes)

So the QR Code is scanned and we only need the EUIs, CAC and the Vendor/Profile IDs. We then use this data in separate RPCs.

Theoretically, if we have the AppIDs and the above Vendor/Profile IDs, we can dial the Device Repository and get the End Device Template but this will be done further in the end device creation flow when the user chooses the region/firmware etc (Step 2 of onboarding flow).

a. So do we really need to use End Device Templates here? Because, if the QR Code does not have Vendor ID and Model ID, in that case wouldn't return a mostly empty End Device (Template) with just the EUIs and CAC?
b. I can update the fields of the EndDeviceOnboardingData to remove the TR005-only fields. But what do we do with those fields? Do we just discard them and only return what's needed for onboarding? Ex: Do we verify this checksum anywhere?

The way I understood the issue, we only need to parse the QR Code and return the data in a generic format and let the
caller call other RPCs with the returned data (For example use Vendor ID/ Model ID and the DR for the device profile).

@johanstokking
Copy link
Member

johanstokking commented Feb 23, 2022

a. So do we really need to use End Device Templates here? Because, if the QR Code does not have Vendor ID and Model ID, in that case wouldn't return a mostly empty End Device (Template) with just the EUIs and CAC?

There may be other QR codes in the future with additional fields. Also, as we don't have a formal serial number field currently, we probably want to put it in an attribute. End device templates are really designed for this.

b. I can update the fields of the EndDeviceOnboardingData to remove the TR005-only fields. But what do we do with those fields? Do we just discard them and only return what's needed for onboarding? Ex: Do we verify this checksum anywhere?

See TR005 1.0 spec:

210 The Checksum is used to validate the data integrity. Even though QR codes have their own
211 built-in integrity checks, this explicit checksum is useful when the content of the QR code is
212 presented as plain text. Checksum is generated using the CRC-16-MODBUS [CRC] of the
213 full QR content except the Checksum field itself, and presented in hexadecimal format
214 without the leading “0x”.

So the TR005 parser should verify it, and after that, it's not of any use anymore.

The way I understood the issue, we only need to parse the QR Code and return the data in a generic format and let the caller call other RPCs with the returned data (For example use Vendor ID/ Model ID and the DR for the device profile).

Yes. So the response would be a oneof with EndDeviceTemplate as one of the options.

@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch from e7979ef to abfb3b7 Compare March 2, 2022 09:53
@KrishnaIyer KrishnaIyer requested a review from pgalic96 as a code owner March 2, 2022 09:53
@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch from abfb3b7 to c0806b7 Compare March 2, 2022 14:15
@KrishnaIyer KrishnaIyer modified the milestones: v3.18.1, v3.18.2 Mar 3, 2022
@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch 3 times, most recently from d7eedfb to bb942c4 Compare March 3, 2022 12:04
@KrishnaIyer KrishnaIyer force-pushed the feature/4845-parse-qr-code branch from f06edc0 to 295c928 Compare April 26, 2022 09:46
@KrishnaIyer KrishnaIyer merged commit 96647b2 into v3.19 Apr 26, 2022
@KrishnaIyer KrishnaIyer deleted the feature/4845-parse-qr-code branch April 26, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/device claiming server compat/api This could affect API compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse QR code
6 participants