Factory data accessor #72

kkasperczyk-no · 2022-06-07T17:09:04Z

The nrfconnect uses test values defined in the header files for the factory data purpose. The data belonging
to factory data should be read from dedicated location in flash memory.

For nrfconnect:

Added FactoryDataProvider class that overrides API to get factory data elements.
Added FactoryDataParser module that read factory data partition from flash and parses cbor format to get raw
data.
For most examples added new factory data partition in the memory.
For most examples added using FactoryDataProvider in the AppTask.

For all:

Moved GetVendorId, GetProductId, GetVendorName and GetProductName methods from ConfigurationManager to
the DeviceInstanceInfoProvider.
For platforms that use different implementation than default LegacyDeviceInstanceInfoProvider (darwin,
linux, tizen, webos, android) added DeviceInstanceInfoProviderImpl.cpp.

The nrfconnect uses test values defined in the header files for the factory data purpose. The data belonging to factory data should be read from dedicated location in flash memory. For nrfconnect: * Added FactoryDataProvider class that overrides API to get factory data elements. * Added FactoryDataParser module that read factory data partition from flash and parses cbor format to get raw data. For all: * Moved GetVendorId, GetProductId, GetVendorName and GetProductName methods from ConfigurationManager to the DeviceInstanceInfoProvider. * For platforms that use different implementation than default LegacyDeviceInstanceInfoProvider (darwin, linux, tizen, webos, android) added DeviceInstanceInfoProviderImpl.cpp.

Damian-Nordic · 2022-06-09T11:22:09Z

I haven't reviewed it yet, but I would suggest splitting the change into PR that moves GetVendorID etc methods to DeviceInstanceInfoProvider and nRF Connect-specific changes. Now the change is large and for other reviewers it may not be clear that we change common interfaces.

config/nrfconnect/chip-module/CMakeLists.txt

* Changed parser implementation to be able to compile it using GN * Added extracting DAC public key from cert instead of reading it from factory data.

kkasperczyk-no · 2022-06-09T12:44:11Z

I haven't reviewed it yet, but I would suggest splitting the change into PR that moves GetVendorID etc methods to DeviceInstanceInfoProvider and nRF Connect-specific changes. Now the change is large and for other reviewers it may not be clear that we change common interfaces.

Ok, it seems to be reasonable. I will keep this PR open and contribute it to upstream in two separate PRs.

config/nrfconnect/chip-module/CMakeLists.txt

examples/all-clusters-app/nrfconnect/main/AppTask.cpp

examples/light-switch-app/nrfconnect/main/AppTask.cpp

src/platform/nrfconnect/BUILD.gn

src/platform/nrfconnect/FactoryDataProvider.h

src/platform/nrfconnect/FactoryDataProvider.cpp

src/platform/nrfconnect/FactoryDataParser.h

src/platform/nrfconnect/FactoryDataParser.c

Damian-Nordic · 2022-06-13T07:27:36Z

src/platform/device.gni

@@ -89,6 +89,9 @@ declare_args() {
    chip_mdns = "none"
  }

+  # Enable factory data support
+  chip_enable_factory_data = false


Note that it can be put in platform/nrfconnect/args.gni since this is nrfconnect-only.

Damian-Nordic · 2022-06-13T07:32:49Z

src/platform/nrfconnect/FactoryDataProvider.h

+{
+    CHIP_ERROR GetFactoryDataPartition(uint8_t ** data, size_t & dataSize)
+    {
+        *data    = reinterpret_cast<uint8_t *>(reinterpret_cast<uint32_t *>(PM_FACTORY_DATA_ADDRESS));


Can we cast directly to uint8_t*? Also, data is passed by pointer and dataSize by reference. Would be good to make it consistent (e.g. pass data as uint8_t*&).

ArekBalysNordic

Good Job :) I have some minor questions, but overall it seems good!

ArekBalysNordic · 2022-06-13T07:47:35Z

src/app/server/Dnssd.cpp

 #include <protocols/secure_channel/PASESession.h>
 #if CHIP_ENABLE_ROTATING_DEVICE_ID && defined(CHIP_DEVICE_CONFIG_ROTATING_DEVICE_ID_UNIQUE_ID)
-#include <platform/DeviceInstanceInfoProvider.h>


Are you sure this should be moved outside CHIP_ENABLE_ROTATING_DEVICE_ID? I remember I have CI failures when it wasn't within this if statement.

I think so, as it needs to be used in order to get vendor id and product id for DNS-SD advertising purpose.

ArekBalysNordic · 2022-06-13T08:06:01Z

src/platform/nrfconnect/FactoryDataProvider.cpp

+    const uint8_t kCdForAllExamples[541] = {
+        0x30, 0x82, 0x02, 0x19, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, 0xa0, 0x82, 0x02, 0x0a, 0x30,
+        0x82, 0x02, 0x06, 0x02, 0x01, 0x03, 0x31, 0x0d, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02,
+        0x01, 0x30, 0x82, 0x01, 0x71, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x01, 0xa0, 0x82, 0x01, 0x62,
+        0x04, 0x82, 0x01, 0x5e, 0x15, 0x24, 0x00, 0x01, 0x25, 0x01, 0xf1, 0xff, 0x36, 0x02, 0x05, 0x00, 0x80, 0x05, 0x01, 0x80,
+        0x05, 0x02, 0x80, 0x05, 0x03, 0x80, 0x05, 0x04, 0x80, 0x05, 0x05, 0x80, 0x05, 0x06, 0x80, 0x05, 0x07, 0x80, 0x05, 0x08,
+        0x80, 0x05, 0x09, 0x80, 0x05, 0x0a, 0x80, 0x05, 0x0b, 0x80, 0x05, 0x0c, 0x80, 0x05, 0x0d, 0x80, 0x05, 0x0e, 0x80, 0x05,
+        0x0f, 0x80, 0x05, 0x10, 0x80, 0x05, 0x11, 0x80, 0x05, 0x12, 0x80, 0x05, 0x13, 0x80, 0x05, 0x14, 0x80, 0x05, 0x15, 0x80,
+        0x05, 0x16, 0x80, 0x05, 0x17, 0x80, 0x05, 0x18, 0x80, 0x05, 0x19, 0x80, 0x05, 0x1a, 0x80, 0x05, 0x1b, 0x80, 0x05, 0x1c,
+        0x80, 0x05, 0x1d, 0x80, 0x05, 0x1e, 0x80, 0x05, 0x1f, 0x80, 0x05, 0x20, 0x80, 0x05, 0x21, 0x80, 0x05, 0x22, 0x80, 0x05,
+        0x23, 0x80, 0x05, 0x24, 0x80, 0x05, 0x25, 0x80, 0x05, 0x26, 0x80, 0x05, 0x27, 0x80, 0x05, 0x28, 0x80, 0x05, 0x29, 0x80,
+        0x05, 0x2a, 0x80, 0x05, 0x2b, 0x80, 0x05, 0x2c, 0x80, 0x05, 0x2d, 0x80, 0x05, 0x2e, 0x80, 0x05, 0x2f, 0x80, 0x05, 0x30,
+        0x80, 0x05, 0x31, 0x80, 0x05, 0x32, 0x80, 0x05, 0x33, 0x80, 0x05, 0x34, 0x80, 0x05, 0x35, 0x80, 0x05, 0x36, 0x80, 0x05,
+        0x37, 0x80, 0x05, 0x38, 0x80, 0x05, 0x39, 0x80, 0x05, 0x3a, 0x80, 0x05, 0x3b, 0x80, 0x05, 0x3c, 0x80, 0x05, 0x3d, 0x80,
+        0x05, 0x3e, 0x80, 0x05, 0x3f, 0x80, 0x05, 0x40, 0x80, 0x05, 0x41, 0x80, 0x05, 0x42, 0x80, 0x05, 0x43, 0x80, 0x05, 0x44,
+        0x80, 0x05, 0x45, 0x80, 0x05, 0x46, 0x80, 0x05, 0x47, 0x80, 0x05, 0x48, 0x80, 0x05, 0x49, 0x80, 0x05, 0x4a, 0x80, 0x05,
+        0x4b, 0x80, 0x05, 0x4c, 0x80, 0x05, 0x4d, 0x80, 0x05, 0x4e, 0x80, 0x05, 0x4f, 0x80, 0x05, 0x50, 0x80, 0x05, 0x51, 0x80,
+        0x05, 0x52, 0x80, 0x05, 0x53, 0x80, 0x05, 0x54, 0x80, 0x05, 0x55, 0x80, 0x05, 0x56, 0x80, 0x05, 0x57, 0x80, 0x05, 0x58,
+        0x80, 0x05, 0x59, 0x80, 0x05, 0x5a, 0x80, 0x05, 0x5b, 0x80, 0x05, 0x5c, 0x80, 0x05, 0x5d, 0x80, 0x05, 0x5e, 0x80, 0x05,
+        0x5f, 0x80, 0x05, 0x60, 0x80, 0x05, 0x61, 0x80, 0x05, 0x62, 0x80, 0x05, 0x63, 0x80, 0x18, 0x24, 0x03, 0x16, 0x2c, 0x04,
+        0x13, 0x5a, 0x49, 0x47, 0x32, 0x30, 0x31, 0x34, 0x32, 0x5a, 0x42, 0x33, 0x33, 0x30, 0x30, 0x30, 0x33, 0x2d, 0x32, 0x34,
+        0x24, 0x05, 0x00, 0x24, 0x06, 0x00, 0x25, 0x07, 0x94, 0x26, 0x24, 0x08, 0x00, 0x18, 0x31, 0x7d, 0x30, 0x7b, 0x02, 0x01,
+        0x03, 0x80, 0x14, 0x62, 0xfa, 0x82, 0x33, 0x59, 0xac, 0xfa, 0xa9, 0x96, 0x3e, 0x1c, 0xfa, 0x14, 0x0a, 0xdd, 0xf5, 0x04,
+        0xf3, 0x71, 0x60, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x30, 0x0a, 0x06, 0x08,
+        0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x02, 0x04, 0x47, 0x30, 0x45, 0x02, 0x20, 0x24, 0xe5, 0xd1, 0xf4, 0x7a, 0x7d,
+        0x7b, 0x0d, 0x20, 0x6a, 0x26, 0xef, 0x69, 0x9b, 0x7c, 0x97, 0x57, 0xb7, 0x2d, 0x46, 0x90, 0x89, 0xde, 0x31, 0x92, 0xe6,
+        0x78, 0xc7, 0x45, 0xe7, 0xf6, 0x0c, 0x02, 0x21, 0x00, 0xf8, 0xaa, 0x2f, 0xa7, 0x11, 0xfc, 0xb7, 0x9b, 0x97, 0xe3, 0x97,
+        0xce, 0xda, 0x66, 0x7b, 0xae, 0x46, 0x4e, 0x2b, 0xd3, 0xff, 0xdf, 0xc3, 0xcc, 0xed, 0x7a, 0xa8, 0xca, 0x5f, 0x4c, 0x1a,
+        0x7c,
+    };
+
+    return CopySpanToMutableSpan(ByteSpan{ kCdForAllExamples }, outBuffer);


We have the same array in src/credentials/examples/DeviceAttestationCredsExample.cpp. What do you think about including here DeviceAttestationCredsExample.h, and calling GetExampleDACProvider()->GetCertificationDeclaration()? Maybe including this array copy here is the best option, but I see another option, and I consider it. :)

I will take a look on that, but I'm not sure whether it's a good idea, as this header is part of another example. So I think we should base our example on core code and not other examples.

ArekBalysNordic · 2022-06-13T08:14:29Z

src/platform/nrfconnect/FactoryDataProvider.h

+
+struct InternalFlashFactoryData
+{
+    CHIP_ERROR GetFactoryDataPartition(uint8_t ** data, size_t & dataSize)


nit: What do you think about renaming these arguments to better document the code?

Suggested change

CHIP_ERROR GetFactoryDataPartition(uint8_t ** data, size_t & dataSize)

CHIP_ERROR GetFactoryDataPartition(uint8_t ** partitionAddress, size_t & partitionSize)

* [Telink] Introduce power management (#72) * [Telink] Moved LightingManager into PWMDevice * [Telink] Enabled PM for Light Switch example * [Telink] Added OpenThread power management support * [Telink] Configured project for the lowest power consumption * [Telink] Restyle * [Telink] Adapt system status LED and buttons with Power Mode usage * [Telink] Fixed factory reset in pool mode of button manager * [Telink] Updated Factory Reset trigger * Restyled by clang-format * [Telink] Adding lighting color feature (#59) * [Telink] Added lighting color feature * [Telink] Added tlsr9518adk80d.overlay to lighting-app * [Telink] Updated RGB functionality * [Telink] Updated RGB PWM pins Co-authored-by: Alex Tsitsiura <[email protected]> * Restyled by whitespace * Restyled by clang-format * [Telink] Minor changes * [Telink] Add 'telink reboot' shell CLI command (#63) * [Telink] Restyled * [Telink] Enable CHIP SED support * [Telink] Update new configs name/location * [Telink] restyle * [Telink] Add PM to all apps * [Telink] set default configs * [Telink] Revert EOL * [Telink] Revert EOL --------- Co-authored-by: Serhii Salamakha <[email protected]> Co-authored-by: Misha.Tkachenko <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Dmytro Huz <[email protected]> * [Telink] Copyright update * [Telink] Fix builds after merge of master * [Telink] Review fixes * [Telink] Custom RF power values example * [Telink] Remove Level Control cluster from binding * [Telink] Add CONFIG_MATTER_LOG_LEVEL --------- Co-authored-by: Serhii Salamakha <[email protected]> Co-authored-by: Misha.Tkachenko <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Dmytro Huz <[email protected]>

kkasperczyk-no force-pushed the factory_data_accessor branch 7 times, most recently from 4689eb1 to 420229b Compare June 9, 2022 09:57

kkasperczyk-no force-pushed the factory_data_accessor branch from 420229b to c53e6d1 Compare June 9, 2022 10:26

kkasperczyk-no requested review from ArekBalysNordic, Damian-Nordic, LuDuda and markaj-nordic June 9, 2022 10:26

Damian-Nordic reviewed Jun 9, 2022

View reviewed changes

config/nrfconnect/chip-module/CMakeLists.txt Outdated Show resolved Hide resolved

Compiled FactoryDataParser using GN instead of cmake

9a51e22

* Changed parser implementation to be able to compile it using GN * Added extracting DAC public key from cert instead of reading it from factory data.

Fixed CI tests

1e4ee53

kkasperczyk-no force-pushed the factory_data_accessor branch from afb161b to 1e4ee53 Compare June 10, 2022 06:45

Damian-Nordic reviewed Jun 10, 2022

View reviewed changes

kkasperczyk-no added 3 commits June 10, 2022 12:47

Fixed fake and darwin builds

6855a61

Addressed some review comments and fixed byte spans lengths

229e22e

Addressed review comments - second part

9b8f66f

kkasperczyk-no requested a review from Damian-Nordic June 12, 2022 16:57

Damian-Nordic approved these changes Jun 13, 2022

View reviewed changes

ArekBalysNordic approved these changes Jun 13, 2022

View reviewed changes

kkasperczyk-no closed this Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factory data accessor #72

Factory data accessor #72

kkasperczyk-no commented Jun 7, 2022 •

edited

Loading

Damian-Nordic commented Jun 9, 2022

kkasperczyk-no commented Jun 9, 2022

Damian-Nordic Jun 13, 2022

Damian-Nordic Jun 13, 2022

ArekBalysNordic left a comment

ArekBalysNordic Jun 13, 2022

kkasperczyk-no Jun 13, 2022 •

edited

Loading

ArekBalysNordic Jun 13, 2022

kkasperczyk-no Jun 13, 2022 •

edited

Loading

ArekBalysNordic Jun 13, 2022

	CHIP_ERROR GetFactoryDataPartition(uint8_t ** data, size_t & dataSize)
	CHIP_ERROR GetFactoryDataPartition(uint8_t ** partitionAddress, size_t & partitionSize)

Factory data accessor #72

Factory data accessor #72

Conversation

kkasperczyk-no commented Jun 7, 2022 • edited Loading

Damian-Nordic commented Jun 9, 2022

kkasperczyk-no commented Jun 9, 2022

Damian-Nordic Jun 13, 2022

Choose a reason for hiding this comment

Damian-Nordic Jun 13, 2022

Choose a reason for hiding this comment

ArekBalysNordic left a comment

Choose a reason for hiding this comment

ArekBalysNordic Jun 13, 2022

Choose a reason for hiding this comment

kkasperczyk-no Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

ArekBalysNordic Jun 13, 2022

Choose a reason for hiding this comment

kkasperczyk-no Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

ArekBalysNordic Jun 13, 2022

Choose a reason for hiding this comment

kkasperczyk-no commented Jun 7, 2022 •

edited

Loading

kkasperczyk-no Jun 13, 2022 •

edited

Loading

kkasperczyk-no Jun 13, 2022 •

edited

Loading