pkg: add library for Microchip CryptoAuth devices as package #13014

Einhornhool · 2019-12-29T19:16:38Z

Contribution description

This contribution adds the official library for Microchip CryptoAuth devices as a package. This is basically a driver included as a package, so this also adds a new auto_init function for ATCA devices and, since it didn't fit into any of the other auto init modules, a new module called "security".

I did my best to comply with the Riot coding standards for drivers, so there's an atca_params.h and an atca.h file for device descriptors and default configuration parameters. These can be found in the pkg/cryptoauthlib folder. Instead of declaring our own device descriptor, this implementation uses the already existing configuration structure declared and used by the library. They get stored in the atca_params array.
There's a wrapper in pkg/cryptoauthlib/contrib, which implements most of the HAL functions. Currently the functions hal_i2c_discover_devices and hal_i2c_discover_buses are unimplemented, as well as hal_i2c_post_init (I don't think they're needed – this assumption may need to be corrected in the future after more testing).

The library provides a basic interface. It implements high level functions for the use of standard configurations and defaults. It is easy to use and recommended for understanding the basic usage and operational flow of the library.
Further information on using the basic api can be found here: https://microchiptech.github.io/cryptoauthlib/a11680.html

Testing procedure

There's a folder test/pkg_cryptoauthlib_compare_sha256 in which I use the library to compare the runtime of the Riot software implementation and the CryptoAuth hardware implementation of SHA-256.
Currently the software implementation is much faster. The reason for this is my implementation of the hal_i2c_wake function in the wrapper, which gets called in the beginning of every command execution of the library. It drives the SDA pin low for 30 us, reinitializes the bus and then waits 1500 us before proceeding the operation. Another possible way to wake up the device described in the reference manual is writing to the address 0x00 at a very low baud rate. Since I couldn't find a way in Riot to set the baud rate that low I choose the first solution. Since this is obviously not ideal, I'm working on fixing that in the near future.

Time measurements for SHA-256 calculation:
Riot software implementation: 27 ms
ATECC508A: 51 ms
Wake function: 19 ms, gets called twice
Overhead due to wake function: 38 ms

The library provides unit tests, which I'm also trying to run with Riot, instead of writing them myself. Currently the way to achieve this is to write a conditional into the package's makefile that checks whether the module "cryptoauthlib_test" is defined. If so, the package gets built and all sourcefiles of its internal test folder are copied to one place and built for the testing application.
A first example of doing this can be found in test/pkg_cryptoauthlib_internal_tests. This application builds the tests of the library and runs some of them. Information on using the tests can be found in their readme file and documentation: https://github.com/MicrochipTech/cryptoauthlib
The goal is to add the library's unit tests to Riot's tests/unittests folder and be able to run them from there.

The main function of the cryptoauthlib unit tests (can be found in the binary folder after building the package: cryptoauthlib/tests/cmd_processor.c) implements a circular buffer to receive commands for testing. I haven't found out how to correctly use it, yet, so I'm using the following workaround:
Via patch I define a self written function for running commands in the cmd_processor.c file, which I call in test/pkg_cryptoauthlib_internal_tests to run the commands for defining the device and running the unit-tests.
Also the library's unit tests overwrite the device descriptor used by Riot so I had to define another function to change it back, that also gets called in cmd_processor.c.

Until now I have used the library with the ATECC508A on the cryptoauth-xpro extension (with samr21-xpro board) and the saml11-xpro. Some functions can only be tested if the data, config and otp zones of the device are locked. Since locking is permanent and cannot be undone I haven't tested those functions, yet.

PeterKietzmann · 2020-01-02T12:48:47Z

pkg/cryptoauthlib/Makefile

@@ -0,0 +1,59 @@
+PKG_NAME=cryptoauthlib


@cgundogan, @leandrolanzieri could you please review this Makefile?

PeterKietzmann

@Einhornhool congrats to your first "real" PR in RIOT :-). It is quite a tricky one but overall seems to be in a good shape. I've left a couple of comments that in almost all cases don't relate to core functionalities but rather ask for clarification and cleanup. Please address my comments in a separate commit or answer them inline. After your first revision I'll start runsning tests on hardware.

PeterKietzmann · 2020-01-02T12:50:35Z

pkg/cryptoauthlib/Makefile.cryptoauthlib

@@ -0,0 +1,3 @@
+MODULE = cryptoauthlib
+
+include $(RIOTBASE)/Makefile.bas


Do you actually use this Makefile? The include path has even a typo.

I tried building without it and it works. I put it in there because all the other packages have it in their module makefiles. Maybe someone should check, if they're useless there as well ;-)

Looking into Makefile.base (not[e] its base not bas suffix) there are lots of macros and stuff that might be needed at some point (even though it works now).

Also I would rather opt for consistency, i.e. matching other packages.

Further, Makefile.base is pulled in by many other modules (boards, cpus, sys/*) too.

Thx, I'll keep it, then

PeterKietzmann · 2020-01-02T12:53:10Z

pkg/cryptoauthlib/Makefile.include

@@ -0,0 +1,16 @@
+PKG_BUILDDIR = $(PKGDIRBASE)/cryptoauthlib
+TESTINCLDIR = $(PKG_BUILDDIR)/test


Isn't PKG_BUILDDIR redefined here?

PKG_BUILDDIR points to the directory where the library is built (in this case bin/pkg/boardname/cryptoauthlib.
That directory includes the test directory from which I need files to build the tests. This is what TESTINCLDIR points to.
Does that answer the question?

PKG_BUILDDIR is defined here and I don't think we need both. Do we?

pkg/cryptoauthlib/Makefile.dep

PeterKietzmann · 2020-01-02T12:59:32Z

pkg/cryptoauthlib/contrib/atca.c

@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2019 2019 HAW Hamburg


PeterKietzmann · 2020-01-02T13:04:34Z

pkg/cryptoauthlib/doc.txt

@@ -0,0 +1,58 @@
+/**
+ * @defgroup pkg_cryptoauthlib cryptoauthlib security crypto 


Better: @defgroup pkg_cryptoauthlib Microchip CryptoAuthentication Library

PeterKietzmann · 2020-01-02T14:48:56Z

sys/auto_init/security/auto_init_atca.c

+ * @file
+ * @brief       Initializes cryptoauth devices
+ *
+ * @author      >


License & author information?

PeterKietzmann · 2020-01-02T14:54:46Z

tests/pkg_cryptoauthlib_compare_sha256/Makefile

+include ../Makefile.tests_common
+
+FEATURES_REQUIRED += periph_i2c
+CFLAGS += -Wno-unused-parameter -DATCA_HAL_I2C


Why do we need to set these flags here in the application Makefile? If they are required always, they should be placed somewhere under pkg/cryptoauthlib

About the suppression of warnings: do they occur often within the package? Could we fix this upstream or add patch to fix it locally? If possible, I'd like not to disable warnings

Several of these warnings occur within the package. I could fix this with patches, but I can't say how many there are. I started inserting void casts into the library's code, but whenever I fix one file, another one throws an error message.

Yeah, patches seem overkill. You could try to propose PRs to the Microchip repo but this is up to you...

PeterKietzmann · 2020-01-02T15:02:01Z

tests/pkg_cryptoauthlib_internal-tests/Makefile

+INCLUDES += -I$(RIOTBASE)/pkg/cryptoauthlib/include
+FEATURES_REQUIRED += periph_i2c
+CFLAGS += -O3 -Wno-unused-parameter -Wno-missing-field-initializers -Wno-int-conversion -Wno-unused-function -Wno-unused-variable
+CFLAGS += -DATCA_HAL_I2C -DDO_NOT_TEST_CERT


Same as above regarding CFLAGS and INCLUDES: Fix what can be fixed so we won't need to set all these compiler flags. For what can't be fixed right away we should set respective flags/includes under pkg/cryptoauthlib.

At least the ATCA_HAL_I2C should be redundant now, tight?

PeterKietzmann · 2020-01-02T15:03:01Z

tests/pkg_cryptoauthlib_internal-tests/main.c

@@ -0,0 +1,11 @@
+#include "cryptoauthlib_test.h"


Copyright, license, author, doxygen....

PeterKietzmann · 2020-01-02T15:13:41Z

tests/pkg_cryptoauthlib_compare_sha256/main.c

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


Generally, this test seems helpful although I don't think we would merge it in this way. I don't know what would the best way to restructure it, but I'll think of it.

PeterKietzmann · 2020-01-02T15:20:52Z

PS: You might also wanna look at the Pull Request Guide especially in terms of "uncrustify-ing" your code.

smlng

Thanks for contributing this, PR looks good already.

However some minor coding style issues, see comments but also CI errors. Please look for trailing or wrong whitespaces, and also check placement of parentheses.

For whitespaces you might find a plugin for your editor which should fix most of that automatically. You can also run make static-test, or run a subset of these tests manually, look e.g. into dist/tools/whitespacecheck and others.

smlng · 2020-01-02T20:22:15Z

pkg/cryptoauthlib/Makefile.include

+    INCLUDES+= -I$(TESTINCLDIR)
+	INCLUDES+= -I$(TESTINCLDIR)/jwt
+	INCLUDES+= -I$(TESTINCLDIR)/tng
+	INCLUDES+= -I$(TESTINCLDIR)/atcacert


mixing spaces and tabs here, 2 spaces should be fine (and consistent with other Makefiles).

smlng · 2020-01-02T20:23:32Z

pkg/cryptoauthlib/contrib/atca.c

@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2019 2019 HAW Hamburg


2019 once should be enough 😉

smlng · 2020-01-02T20:24:16Z

pkg/cryptoauthlib/contrib/atca.c

+ATCA_STATUS hal_i2c_init(void *hal, ATCAIfaceCfg *cfg)
+{   
+    if(cfg->iface_type != ATCA_I2C_IFACE)
+    {


Coding style: opening brace must not be on separate line, please check throughout the code

smlng · 2020-01-02T20:25:02Z

pkg/cryptoauthlib/contrib/atca.c

+    while (retries-- > 0 && ret != 0)
+    {
+        ret = i2c_read_byte(cfg->atcai2c.bus, (cfg->atcai2c.slave_address >> 1), 
+        &length_package, 0);


indent to match opening parentheses from function above

smlng · 2020-01-02T20:25:30Z

pkg/cryptoauthlib/contrib/cryptoauthlib_test_contrib.c

@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2019 2019 HAW Hamburg


smlng · 2020-01-02T20:28:53Z

pkg/cryptoauthlib/include/atca_params.h

+/**
+ * @name    Set default configuration parameters for the ATCA device
+ *
+ * @brief   The CryptoAuth library defines the data structure ATCAIfaceCfg for


smlng · 2020-01-02T20:29:42Z

pkg/cryptoauthlib/include/atca_params.h

+#ifndef ATCA_GPIO_WAKE
+#define ATCA_GPIO_WAKE (GPIO_PIN(0, 16))
+#endif
+


I think the oxygen group above is missing a close here?

smlng · 2020-01-02T20:30:13Z

pkg/cryptoauthlib/include/cryptoauthlib_test.h

+#endif
+
+/**
+ * @name    Helper function to use the library's unittests


use @brief here, as there is no grouping

smlng · 2020-01-02T20:30:41Z

sys/auto_init/security/auto_init_atca.c

+ * @file
+ * @brief       Initializes cryptoauth devices
+ *
+ * @author      >


author tag broken?

sys/auto_init/security/auto_init_atca.c

Einhornhool · 2020-01-03T19:16:47Z

@PeterKietzmann @smlng thx for the feedback, I've read it and will adapt the code =)

pkg/cryptoauthlib/Makefile

kaspar030

Looking forward to this!
There are some issues with possible variable name clashes in the Makefile.include.
Other than that, some style nits.

pkg/cryptoauthlib/Makefile.include

kaspar030 · 2020-01-23T10:47:48Z

sys/auto_init/security/auto_init_atca.c

+{
+    for (unsigned i = 0; i < ATCA_NUMOF; i++) {
+        if (atcab_init((ATCAIfaceCfg *)&atca_params[i]) != ATCA_SUCCESS) {
+            continue;


should this be handled?

tests/pkg_cryptoauthlib_compare_sha256/main.c

kaspar030 · 2020-01-23T10:49:37Z

tests/pkg_cryptoauthlib_compare_sha256/main.c

+        printf("Riot SHA256: Success\n");
+    }
+    else {
+        printf("Riot SHA256: Not a success.\n");


"error" or "failure"? Not that we accidentally grep for success. ;)

tests/pkg_cryptoauthlib_internal-tests/Makefile

kaspar030 · 2020-01-23T10:50:51Z

tests/pkg_cryptoauthlib_internal-tests/Makefile

@@ -0,0 +1,9 @@
+include ../Makefile.tests_common
+
+INCLUDES += -I$(RIOTBASE)/pkg/cryptoauthlib/include


Isn't this already set in pkg/cryptoauthlib/Makefile.include? (otherwise, it should)

kaspar030 · 2020-01-23T10:51:47Z

pkg/cryptoauthlib/Makefile.include

+DIRS += $(RIOTPKG)/cryptoauthlib/contrib
+
+ifneq (,$(filter cryptoauthlib_test,$(USEMODULE)))
+  INCLUDES+= -I$(TESTINCLDIR)


missing spaces after INCLUDES

kaspar030 · 2020-01-23T10:52:51Z

pkg/cryptoauthlib/include/cryptoauthlib_test.h

+ *          This function is defined in the cryptoauth library via patch.
+ *          It is used to pass commands to run built-in unit tests of the library.
+ */
+int run_cmd(const char *command);


is this a symbol name from upstream? otherwise it should be namespaced.

kaspar030 · 2020-01-23T10:54:05Z

sys/auto_init/auto_init.c

@@ -620,4 +620,14 @@ void auto_init(void)
    test_utils_interactive_sync();
 #endif
 #endif /* MODULE_TEST_UTILS_INTERACTIVE_SYNC */
+
+#ifdef MODULE_AUTO_INIT_SECURITY


I think the test_utils_interactive_sync should stay last. @fjmolinas right?

leandrolanzieri · 2020-01-28T18:30:12Z

pkg/cryptoauthlib/Makefile

+CFLAGS += -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-function -Wno-type-limits -Wno-strict-aliasing -Wno-unused-variable -DATCA_HAL_I2C
+TOOLCHAIN_FILE=$(PKG_BUILDDIR)/xcompile-toolchain.cmake
+
+ifneq (,$(filter cryptoauthlib_test,$(USEMODULE)))


Can use PKG_TEST_NAME to keep the name only in one place

PeterKietzmann

I've tested:

tests/pkg_cryptoauthlib_compare_sha256/ on a saml11-xpro

results

2020-01-28 19:41:09,490 # main(): This is RIOT! (Version: 2020.01-devel-1654-g6f213-HEAD)
2020-01-28 19:41:09,492 # Riot SHA256: Success
2020-01-28 19:41:09,515 # ATCA SHA256: Success

tests/pkg_cryptoauthlib_internal-tests on a samr21-xpro with a CryptoAuth-XPRO-B shield attached

results

2020-01-28 19:30:31,410 # START
2020-01-28 19:30:31,416 # main(): This is RIOT! (Version: 2020.01-devel-1654-g6f213-HEAD)
2020-01-28 19:30:31,416 # 
2020-01-28 19:30:31,417 # Device Selected.
2020-01-28 19:30:31,418 # 
2020-01-28 19:30:31,418 # 
2020-01-28 19:30:31,420 # Unity test run 1 of 1
2020-01-28 19:30:31,425 # TEST(atca_cmd_unit_test, wake_sleep)
2020-01-28 19:30:31,433 # PASS
2020-01-28 19:30:31,433 # 
2020-01-28 19:30:31,438 # TEST(atca_cmd_unit_test, wake_idle)
2020-01-28 19:30:31,445 # PASS
2020-01-28 19:30:31,446 # 
2020-01-28 19:30:31,450 # TEST(atca_cmd_unit_test, crcerror)
2020-01-28 19:30:31,460 # PASS
2020-01-28 19:30:31,461 # 
2020-01-28 19:30:31,465 # TEST(atca_cmd_unit_test, info)
2020-01-28 19:30:31,475 # PASS
2020-01-28 19:30:31,476 # 
2020-01-28 19:30:31,481 # TEST(atca_cmd_unit_test, verify)
2020-01-28 19:30:31,500 # atca_tests_verify.c:464:TEST(atca_cmd_unit_test, verify):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,501 # SKIP
2020-01-28 19:30:31,501 # 
2020-01-28 19:30:31,506 # TEST(atca_cmd_unit_test, derivekey)
2020-01-28 19:30:31,526 # atca_tests_derivekey.c:464:TEST(atca_cmd_unit_test, derivekey):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,526 # SKIP
2020-01-28 19:30:31,527 # 
2020-01-28 19:30:31,531 # TEST(atca_cmd_unit_test, sha)
2020-01-28 19:30:31,555 # PASS
2020-01-28 19:30:31,556 # 
2020-01-28 19:30:31,559 # TEST(atca_cmd_unit_test, hmac)
2020-01-28 19:30:31,578 # atca_tests_hmac.c:464:TEST(atca_cmd_unit_test, hmac):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,579 # SKIP
2020-01-28 19:30:31,580 # 
2020-01-28 19:30:31,584 # TEST(atca_cmd_unit_test, sign)
2020-01-28 19:30:31,603 # atca_tests_sign.c:464:TEST(atca_cmd_unit_test, sign):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,603 # SKIP
2020-01-28 19:30:31,603 # 
2020-01-28 19:30:31,608 # TEST(atca_cmd_unit_test, mac)
2020-01-28 19:30:31,627 # atca_tests_mac.c:464:TEST(atca_cmd_unit_test, mac):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,627 # SKIP
2020-01-28 19:30:31,627 # 
2020-01-28 19:30:31,632 # TEST(atca_cmd_unit_test, checkmac)
2020-01-28 19:30:31,652 # atca_tests_mac.c:464:TEST(atca_cmd_unit_test, checkmac):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,652 # SKIP
2020-01-28 19:30:31,652 # 
2020-01-28 19:30:31,657 # TEST(atca_cmd_unit_test, ecdh)
2020-01-28 19:30:31,676 # atca_tests_ecdh.c:479:TEST(atca_cmd_unit_test, ecdh):IGNORE: Data zone must be locked for this test.
2020-01-28 19:30:31,676 # SKIP
2020-01-28 19:30:31,677 # 
2020-01-28 19:30:31,681 # TEST(atca_cmd_unit_test, write)
2020-01-28 19:30:31,701 # atca_tests_write.c:464:TEST(atca_cmd_unit_test, write):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,701 # SKIP
2020-01-28 19:30:31,701 # 
2020-01-28 19:30:31,706 # TEST(atca_cmd_unit_test, read)
2020-01-28 19:30:31,716 # PASS
2020-01-28 19:30:31,717 # 
2020-01-28 19:30:31,721 # TEST(atca_cmd_unit_test, genkey)
2020-01-28 19:30:31,741 # atca_tests_genkey.c:464:TEST(atca_cmd_unit_test, genkey):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,742 # SKIP
2020-01-28 19:30:31,742 # 
2020-01-28 19:30:31,747 # TEST(atca_cmd_unit_test, privwrite)
2020-01-28 19:30:31,766 # atca_tests_privwrite.c:464:TEST(atca_cmd_unit_test, privwrite):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,767 # SKIP
2020-01-28 19:30:31,768 # 
2020-01-28 19:30:31,772 # TEST(atca_cmd_unit_test, gendig)
2020-01-28 19:30:31,791 # atca_tests_gendig.c:464:TEST(atca_cmd_unit_test, gendig):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,792 # SKIP
2020-01-28 19:30:31,792 # 
2020-01-28 19:30:31,797 # TEST(atca_cmd_unit_test, random)
2020-01-28 19:30:31,811 # PASS
2020-01-28 19:30:31,812 # 
2020-01-28 19:30:31,817 # TEST(atca_cmd_unit_test, nonce_passthrough)
2020-01-28 19:30:31,830 # PASS
2020-01-28 19:30:31,830 # 
2020-01-28 19:30:31,835 # TEST(atca_cmd_unit_test, pause)
2020-01-28 19:30:31,846 # PASS
2020-01-28 19:30:31,847 # 
2020-01-28 19:30:31,852 # TEST(atca_cmd_unit_test, updateextra)
2020-01-28 19:30:31,872 # atca_tests_updateextra.c:464:TEST(atca_cmd_unit_test, updateextra):IGNORE: Config zone must be locked for this test.
2020-01-28 19:30:31,872 # SKIP
2020-01-28 19:30:31,873 # 
2020-01-28 19:30:31,878 # TEST(atca_cmd_unit_test, counter)
2020-01-28 19:30:31,899 # PASS
2020-01-28 19:30:31,899 # 
2020-01-28 19:30:31,900 # 
2020-01-28 19:30:31,900 # 
2020-01-28 19:30:31,901 # -----------------------
2020-01-28 19:30:31,904 # 22 Tests 0 Failures 12 Ignored 
2020-01-28 19:30:31,905 # OK
2020-01-28 19:30:31,905 #

And I even used the RNG on a 608A chip with this code base but this is out ouf scope for this PR

PeterKietzmann · 2020-01-28T16:39:00Z

pkg/cryptoauthlib/Makefile

+TOOLCHAIN_FILE=$(PKG_BUILDDIR)/xcompile-toolchain.cmake
+
+ifneq (,$(filter cryptoauthlib_test,$(USEMODULE)))
+all: cryptoauth build_tests


Indentation

PeterKietzmann · 2020-01-28T17:26:32Z

pkg/cryptoauthlib/Makefile.include

@@ -0,0 +1,16 @@
+CRYPTOAUTH_BUILDDIR ?= $(PKGDIRBASE)/cryptoauthlib


Please only use PKG_BUILDDIR

PeterKietzmann · 2020-01-28T17:34:07Z

pkg/cryptoauthlib/contrib/cryptoauthlib_test_contrib.c

+#include "atca.h"
+#include "atca_params.h"
+
+/* Function switches the default cfg in cryptoauthlib test to Riot cfg */


RIOT not Riot please (:

PeterKietzmann · 2020-01-28T17:34:36Z

pkg/cryptoauthlib/include/atca_params.h

+ *          device initialization. We use this instead of a self defined params
+ *          struct and store it in the params array.
+ *          ATCAIfaceCfg contains a variable for the bus address, which is never
+ *          used by the library. We use it to store Riot's I2C_DEV.


RIOT not Riot

PeterKietzmann · 2020-01-28T17:34:52Z

pkg/cryptoauthlib/include/atca_params.h

+ *          struct and store it in the params array.
+ *          ATCAIfaceCfg contains a variable for the bus address, which is never
+ *          used by the library. We use it to store Riot's I2C_DEV.
+ *          We also initialize the baud rate with zero, because Riot doesn't have


PeterKietzmann · 2020-01-28T18:21:49Z

pkg/cryptoauthlib/include/atca.h

+#define ATCA_IDLE_ADDR   (0x02) /**< Word address to write to for idle mode */
+#define ATCA_DATA_ADDR   (0x03) /**< Word address to read and write to data area */
+
+#define WAKE_LOW_DURATION (30)  /**< Duration time to keep SDA pin low in wake function */


Not needed anymore.

PeterKietzmann · 2020-01-28T18:21:59Z

pkg/cryptoauthlib/include/atca_params.h

+ */
+
+/**
+ * @ingroup     pkg_cryptoauthlib cryptoauthlib security crypto


Check groups

PeterKietzmann · 2020-01-28T18:24:15Z

pkg/cryptoauthlib/include/cryptoauthlib_test.h

@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2019 2019 HAW Hamburg


2019 once is enough.

PeterKietzmann · 2020-01-28T18:25:22Z

sys/auto_init/security/auto_init_atca.c

+        if (atcab_init((ATCAIfaceCfg *)&atca_params[i]) != ATCA_SUCCESS) {
+            LOG_ERROR(
+                "[auto_init_atca] error initializing cryptoauth device #%u\n",
+                i);


Weird linebreaks?

PeterKietzmann · 2020-01-28T18:43:04Z

tests/pkg_cryptoauthlib_internal-tests/Makefile

@@ -0,0 +1,7 @@
+include ../Makefile.tests_common
+
+CFLAGS += -DDO_NOT_TEST_CERT


Please add a comment that we ignore CERTs due to memory constraints.

PeterKietzmann · 2020-01-28T18:44:50Z

Ah and while you are at it: please squash all fixup commits before you address my latest comments

PeterKietzmann

Looks great! I re-tested tests/cryptoauth* with saml11-xpro and samr21-xpro + cryptoauth-xplained-pro B shield with succes (se previous report for further details) and I've built doxygen locally: looks all right. Please squash!

kaspar030 · 2020-01-29T08:50:34Z

pkg/cryptoauthlib/Makefile

+PKG_VERSION=af8187776cd3f3faf8bed412eaf6ff7221862e19
+PKG_LICENSE=LGPL-2.1
+PKG_TEST_NAME=cryptoauthlib_test
+PKG_BUILDDIR ?= $(PKGDIRBASE)/cryptoauthlib


isn't this the default as set in pkg/pkg.mk? (probably just remove this line)

kaspar030 · 2020-01-29T09:15:37Z

pkg/cryptoauthlib/Makefile

+endif
+
+cryptoauth: $(PKG_BUILDDIR)/lib/Makefile
+	$(MAKE) -C $(PKG_BUILDDIR)/lib && \


Suggested change

$(MAKE) -C $(PKG_BUILDDIR)/lib && \

$(MAKE) -C $(PKG_BUILDDIR)/lib

kaspar030

I don't see blockers anymore. Partial ACK.

benpicco · 2020-01-30T16:27:09Z

FYI: same54-xpro comes with an ATECC508A on the board, connected to I2C0, so I had to try this PR - all tests are running fine, no configuration needed 😃
(well I had to configure a lower CPU frequency until #12969 is merged, but that's unrelated)

PeterKietzmann

@Einhornhool there are some minor leftovers (see comments inline and Murdock output). Please address them and squash your latest commits (as discussed offline)

PeterKietzmann · 2020-01-30T16:44:30Z

pkg/cryptoauthlib/Makefile.include

+ifneq (,$(filter cortex-m%,$(CPU_ARCH)))
+  # relic package package is not using system includes right now, so
+  # many newlib headers (not even stdio.h) are not found.
+  # Fixed in #9821 for jerryscript, should be applicable here too.


Please remove this comment.

PeterKietzmann · 2020-01-30T16:45:26Z

tests/pkg_cryptoauthlib_internal-tests/Makefile.ci

+    mega-xplained \
+    microduino-corerf \
+    saml10-xpro \
+    waspmote-pro


Missing newline at end of file.

PeterKietzmann · 2020-01-30T16:47:50Z

pkg/cryptoauthlib/Makefile

+
+include $(RIOTBASE)/pkg/pkg.mk
+
+BOARD_BLACKLIST := stk3600 stk3700


In this specific case I would rather add this blacklist to both application Makefiles and add a comment that this relates to duplicate definitions from vendor header and library.

Further, looking at the Murdock output, the blacklist doesn't even seem to work when defined here.

PeterKietzmann · 2020-01-30T17:10:49Z

BTW: I ran both provided tests with this state too using samr21-xpro and the cryptoauth-xpro B shield -> all good!

aabadie

I had a brief look and have some comments/questions.

aabadie · 2020-01-30T18:34:22Z

pkg/cryptoauthlib/contrib/Makefile

@@ -0,0 +1,3 @@
+MODULE := cryptoauthlib_contrib


you don't need : here

aabadie · 2020-01-30T18:37:54Z

pkg/cryptoauthlib/patches/0001-change-include_directories-to-target_include_directo.patch

+ endif(ATCA_PRINTF)
+
+-include_directories(cryptoauth PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ../third_party/hidapi/hidapi ${USB_INCLUDE_DIR})
+target_include_directories(cryptoauth PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ../third_party/hidapi/hidapi ${USB_INCLUDE_DIR})


this seems like a bug upstream,why not propose the patch there instead ?

We do have a list of follow-ups which somehow includes fixing shortcomings in the remote repository. However, these should not block this PR.

aabadie · 2020-01-30T18:42:32Z

sys/auto_init/Makefile

@@ -22,4 +22,8 @@ ifneq (,$(filter auto_init_usbus,$(USEMODULE)))
  DIRS += usb
 endif

+ifneq (,$(filter auto_init_security,$(USEMODULE)))
+  DIRS += security


I don't understand the need for this new auto_init_security.

Among others, we do also have separate auto_init_can or auto_init_usb so I don't see a problem with that. Further, we might add other security related initialization in here later. To be more precise, I personally have some ongoing work which might go in there.

aabadie · 2020-01-30T18:45:32Z

tests/pkg_cryptoauthlib_compare_sha256/Makefile

+# Test fails to build for these boards fails due to
+# redefinition of define AES_COUNT in library, which
+# is also defined in efm32 header files
+BOARD_BLACKLIST := stk3600 stk3700


this could just be moved to the Make file.dep file of the package, and thus be removed from both test applications (with the benefit of less code duplication)

I explicitly requested to put it here. The need to blacklist these boards is stupid and simply related to name collisions of #defines from the efm32 vendor header and the library. Yet, we did not find an elegant solution to this problem. The intention is to make this "as visible as possible" to the user. And hopefully we find a better solution in future.

PeterKietzmann

After final testing with tests/pkg_cryptoauthlib_internal-tests/ on a samr21-xpro + cryptoauth-xpro B and tests/pkg_cryptoauthlib_compare_sha256/ on a saml11-xpro everything still works fine and so is Murdock. ACK and congrats @Einhornhool!

PeterKietzmann reviewed Jan 2, 2020

View reviewed changes

smlng reviewed Jan 2, 2020

View reviewed changes

smlng removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 2, 2020

leandrolanzieri reviewed Jan 6, 2020

View reviewed changes

pkg/cryptoauthlib/Makefile Outdated Show resolved Hide resolved

pkg/cryptoauthlib/Makefile Outdated Show resolved Hide resolved

pkg/cryptoauthlib/Makefile Outdated Show resolved Hide resolved

pkg/cryptoauthlib/Makefile Show resolved Hide resolved

leandrolanzieri reviewed Jan 6, 2020

View reviewed changes

pkg/cryptoauthlib/Makefile Outdated Show resolved Hide resolved

Einhornhool force-pushed the cryptoauthlib_implementation branch 2 times, most recently from 694d358 to 0eeae51 Compare January 8, 2020 12:49

kaspar030 requested changes Jan 23, 2020

View reviewed changes

leandrolanzieri reviewed Jan 28, 2020

View reviewed changes

PeterKietzmann reviewed Jan 28, 2020

View reviewed changes

Einhornhool force-pushed the cryptoauthlib_implementation branch from af35cde to fadc2ad Compare January 28, 2020 20:17

PeterKietzmann reviewed Jan 28, 2020

View reviewed changes

Einhornhool force-pushed the cryptoauthlib_implementation branch from f8a0df6 to d717516 Compare January 28, 2020 21:58

PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 28, 2020

kaspar030 reviewed Jan 29, 2020

View reviewed changes

kaspar030 approved these changes Jan 29, 2020

View reviewed changes

Einhornhool force-pushed the cryptoauthlib_implementation branch from d2928b9 to e252135 Compare January 30, 2020 16:04

PeterKietzmann reviewed Jan 30, 2020

View reviewed changes

PeterKietzmann added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Jan 30, 2020

PeterKietzmann added Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jan 30, 2020

Einhornhool force-pushed the cryptoauthlib_implementation branch from e252135 to 9f9ea29 Compare January 30, 2020 17:24

aabadie reviewed Jan 30, 2020

View reviewed changes

Lena Boeckmann and others added 4 commits January 30, 2020 23:51

pkg: add cryptoauthlib

529f3c8

tests: add pkg_cryptoauthlib_compare_sha256

7842c3d

tests: add pkg_cryptoauthlib_internal-tests

83de1cf

sys/auto_init: add security module and cryptoauth init

3dae0dd

PeterKietzmann force-pushed the cryptoauthlib_implementation branch from 9f9ea29 to 3dae0dd Compare January 30, 2020 22:52

PeterKietzmann approved these changes Jan 30, 2020

View reviewed changes

PeterKietzmann merged commit 92c7a7a into RIOT-OS:master Jan 30, 2020

benpicco mentioned this pull request Feb 5, 2020

[WIP] cpu/cc26x2_cc13x2: add sub-1-ghz netdev driver #13295

Closed

jue89 mentioned this pull request Feb 19, 2020

pkg/cryptoauthlib: wake pulse implementation for CPUs supporting periph_i2c_reconfigure #13412

Merged

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

jue89 mentioned this pull request Mar 4, 2020

pkg/cryptoauthlib: enable boards to set auto init parameters #13550

Merged

		@@ -0,0 +1,3 @@
		MODULE = cryptoauthlib

		include $(RIOTBASE)/Makefile.bas

		@@ -0,0 +1,16 @@
		PKG_BUILDDIR = $(PKGDIRBASE)/cryptoauthlib
		TESTINCLDIR = $(PKG_BUILDDIR)/test

		@@ -0,0 +1,58 @@
		/**
		* @defgroup pkg_cryptoauthlib cryptoauthlib security crypto

		@@ -0,0 +1,9 @@
		include ../Makefile.tests_common

		INCLUDES += -I$(RIOTBASE)/pkg/cryptoauthlib/include

		@@ -0,0 +1,16 @@
		CRYPTOAUTH_BUILDDIR ?= $(PKGDIRBASE)/cryptoauthlib

		@@ -0,0 +1,7 @@
		include ../Makefile.tests_common

		CFLAGS += -DDO_NOT_TEST_CERT

	$(MAKE) -C $(PKG_BUILDDIR)/lib && \
	$(MAKE) -C $(PKG_BUILDDIR)/lib


		include $(RIOTBASE)/pkg/pkg.mk

		BOARD_BLACKLIST := stk3600 stk3700

pkg: add library for Microchip CryptoAuth devices as package #13014

pkg: add library for Microchip CryptoAuth devices as package #13014

Conversation

Einhornhool commented Dec 29, 2019

Contribution description

Testing procedure

Choose a reason for hiding this comment

PeterKietzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng Jan 7, 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

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

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

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

PeterKietzmann commented Jan 2, 2020

smlng left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Einhornhool commented Jan 3, 2020

kaspar030 left a comment

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

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

PeterKietzmann left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann commented Jan 28, 2020

PeterKietzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 left a comment

Choose a reason for hiding this comment

benpicco commented Jan 30, 2020

PeterKietzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

PeterKietzmann commented Jan 30, 2020

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng Jan 7, 2020 •

edited

Loading

PeterKietzmann Jan 30, 2020 •

edited

Loading