Base45 to a Set up payload #146

bhaskar-apple · 2020-03-29T19:18:09Z

Problem

QR code base45 string parser #145

Summary of Changes

Add a new class QRCodeSetupPayloadParser that converts a base45 string to a SetupPayload object

fixes #145

pan-apple · 2020-03-30T16:11:24Z

src/setup_payload/QRCodeSetupPayloadParser.cpp

+static uint64_t readBits(vector <uint8_t> buf, int &index, size_t numberOfBitsToRead)
+{
+    uint64_t dest = 0;
+    if (index + numberOfBitsToRead > buf.size() * 8 || numberOfBitsToRead > 64) {


maybe change 64 to sizeof(uint64_t) * 8 (or document why 64, i.e. the return type can only hold 64 bits)?
Also, the caller of this function won't know if this is an error, or the bits were actually 0.

For example, https://github.com/project-chip/connectedhomeip/pull/146/files#diff-958871ab391ca31ed8a2a6cee29fade0R68

if someone to change kVersionFieldLengthInBits to 65, the QRCodeSetupPayloadParser::payload will continue to run successfully, but return incorrect values.

Will change to sizeof(uint64_t) * 8

if someone to change kVersionFieldLengthInBits to 65, the QRCodeSetupPayloadParser::payload will continue to run successfully, but return incorrect values.

@bhaskar-apple Pankaj has a point here. We should return a proper error here so the caller knows it was an error?

@sagar-apple he does and Rob is working on a patch to fix it both for parsing and encoding.

gerickson · 2020-03-30T17:02:27Z

Overall, as this code starts to adopt more error handling, I'd advocate, since it is new code, picking a convention for error handling.

Personally, I'd advocate for a single entry point, single return point style, supported by macros of the sort embodied by here https://github.com/project-chip/connectedhomeip/blob/master/src/lib/support/CodeUtils.h, which in turn, are supported by those here, https://github.com/nestlabs/nlassert/, which were inspired by those here, https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/AssertMacros.h.

bhaskar-apple · 2020-03-30T19:54:45Z

@gerickson - I agree having a convention for error handling for new code. But I would request not holding this PR before we agree on what that should be. I think we need to have a bit more offline discussion around the convention. I am happy to open a new PR to fix this one as soon as we land on a convention.
While the goto paradigm is useful; I think it has its drawbacks and is not applicable everywhere. I would personally refrain from using gotos where possible.

Just as as interesting read, one example of goto failing.
https://www.imperialviolet.org/2014/02/22/applebug.html
Note: not implying goto is the only reason for the failure in this link

gerickson · 2020-03-30T20:33:42Z

src/setup_payload/QRCodeSetupPayloadParser.cpp

@@ -0,0 +1,77 @@
+/*


I would encourage Doxygenizing this new code.

gerickson · 2020-03-30T20:33:51Z

src/setup_payload/QRCodeSetupPayloadParser.h

@@ -0,0 +1,42 @@
+/*


I would encourage Doxygenizing this new code.

woody-apple · 2020-03-30T21:38:20Z

@hawk248 ?

woody-apple · 2020-03-31T00:11:38Z

@bhaskar-apple can we fix this conflict? @BroderickCarlin or @jelderton can we get one more set of eyes on this? Thanks!

hawk248

👍

rwalker-apple · 2020-03-31T03:21:04Z

src/setup_payload/QRCodeSetupPayloadGenerator.cpp

@@ -37,6 +37,7 @@ static void populateBits(uint8_t * bits, int & offset, uint64_t input, size_t nu
    // do nothing in the case where we've overflowed. should never happen
    if (offset + numberOfBits > kTotalPayloadDataSizeInBits || input >= 1u << numberOfBits)
    {
+        fprintf(stderr, "Overflow while trying to generate a QR Code. Bailing.");


to other reviewers: these should never occur, except during development

rwalker-apple · 2020-03-31T03:23:10Z

src/setup_payload/tests/tests_qrcode.cpp

+    unequalPayload.discriminator         = 28;
+    unequalPayload.setUpPINCode          = 121233;
+
+    bool result = payload == unequalPayload;


you're testing the compiler here?

rwalker-apple · 2020-03-31T03:23:36Z

src/setup_payload/tests/tests_qrcode.cpp

+    equalPayload.discriminator         = 128;
+    equalPayload.setUpPINCode          = 2048;
+
+    bool result = payload == equalPayload;


you're testing the compiler?

rwalker-apple · 2020-03-31T03:24:37Z

src/setup_payload/tests/tests_qrcode.cpp

+int testInvalidQRCodePayload()
+{
+    int surprises                   = 0;
+    string invalidString            = "adas12AA";


might be more interesting to have valid base45 string that's too short, or fails verification (payload.is_valid()?)

This reverts commit cc85d6d.

…tribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

Add BLE MangerImpl ShutDown Function

…tribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

…mmand and attribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

…tribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

…mmand and attribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

…tribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

…mmand and attribute handler for unify bridge Merge in WMN_TOOLS/matter from bugix/no_jira_updating_zap_genarted_files to silabs Squashed commit of the following: commit b704f0b222259f9caf9d21885a1c0e49bb7b6741 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 15:46:53 2022 +0200 fixing data type mapping commit 4e866bea88f3f0af41de2d7eb2a90b0a6428d963 Author: Dereje Wassie <[email protected]> Date: Thu Oct 6 13:51:35 2022 +0200 Updated the zap files for genarting command and attribute handler for unify bridge

bhaskar-apple requested review from anush-apple, BroderickCarlin, chrisdecenzo, gerickson, hawk248, jelderton, robszewczyk, tpmanley and woody-apple as code owners March 29, 2020 19:18

pan-apple reviewed Mar 30, 2020

View reviewed changes

gerickson reviewed Mar 30, 2020

View reviewed changes

src/setup_payload/QRCodeSetupPayloadParser.h Outdated

@@ -0,0 +1,42 @@

/*

Copy link

Contributor

gerickson Mar 30, 2020

Choose a reason for hiding this comment

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

I would encourage Doxygenizing this new code.

gerickson approved these changes Mar 30, 2020

View reviewed changes

woody-apple approved these changes Mar 30, 2020

View reviewed changes

Bhaskar-Sarma and others added 4 commits March 30, 2020 15:53

Base45 to a Set up payload

1c56f4b

PR Feedback. Add doxygen comments and if clause

7d5e5aa

fixup after rebase

952415a

fixup after rebase

9a914af

hawk248 approved these changes Mar 31, 2020

View reviewed changes

rwalker-apple reviewed Mar 31, 2020

View reviewed changes

bhaskar-apple merged commit cc85d6d into project-chip:master Mar 31, 2020

woody-apple added a commit that referenced this pull request Mar 31, 2020

Revert "Base45 to a Set up payload (#146)"

6672955

This reverts commit cc85d6d.

woody-apple mentioned this pull request Mar 31, 2020

Revert "Base45 to a Set up payload" #160

Merged

bhaskar-apple mentioned this pull request Mar 31, 2020

Base45 to a Set up payload #164

Merged

shchen-Lab added a commit to shchen-Lab/connectedhomeip that referenced this pull request Aug 23, 2023

Merge pull request project-chip#146 from shchen-Lab/bl616_bouffalo_sdk

200f7b8

Add BLE MangerImpl ShutDown Function

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base45 to a Set up payload #146

Base45 to a Set up payload #146

bhaskar-apple commented Mar 29, 2020

pan-apple Mar 30, 2020

bhaskar-apple Mar 30, 2020

sagar-apple Mar 30, 2020

bhaskar-apple Mar 31, 2020

gerickson commented Mar 30, 2020

bhaskar-apple commented Mar 30, 2020

gerickson Mar 30, 2020

gerickson Mar 30, 2020

woody-apple commented Mar 30, 2020

woody-apple commented Mar 31, 2020

hawk248 left a comment

rwalker-apple Mar 31, 2020 •

edited

Loading

rwalker-apple Mar 31, 2020

rwalker-apple Mar 31, 2020

rwalker-apple Mar 31, 2020

Base45 to a Set up payload #146

Base45 to a Set up payload #146

Conversation

bhaskar-apple commented Mar 29, 2020

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerickson commented Mar 30, 2020

bhaskar-apple commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented Mar 30, 2020

woody-apple commented Mar 31, 2020

hawk248 left a comment

Choose a reason for hiding this comment

rwalker-apple Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwalker-apple Mar 31, 2020 •

edited

Loading