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.

Sign up for GitHub

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

Jump to bottom

Initial support atecc608a on 32se #2034

Merged
merged 4 commits into from Jan 18, 2019
Merged

Initial support atecc608a on 32se #2034

merged 4 commits into from Jan 18, 2019

Conversation

miyazakh
Copy link
Contributor

This PR is a checkpoint to support atecc608a on esp-wroom-32se.

  • Add WOLFSS_ESPWROOM32SE definition to enable atecc608a(atecc508a) use with wolfssl
  • Modified simple tls_server/tls_clinet
  • Use cryptoauthlib to access 608a on 32se

@miyazakh miyazakh changed the title Initial support atec608a on 32se Initial support atecc608a on 32se Jan 16, 2019
@dgarske dgarske self-requested a review January 16, 2019 16:57
@dgarske dgarske self-assigned this Jan 16, 2019
dgarske
dgarske requested changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Hi Hideki. Excellent work on this. Just a few minor cleanups.

IDE/Espressif/ESP-IDF/examples/wolfssl_benchmark/main/helper.c Outdated
/* proto-type */
extern void wolf_benchmark_task();
extern int benchmark_init();
extern int benchmark_test(void *args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include wolfcrypt/benchmark/benchmark.h instead for benchmark_init and benchmark_test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included wolfcrypt/benchmark/benchmark.h
Removed benchmark_init() and benchmark_test() proto-type

IDE/Espressif/ESP-IDF/examples/wolfssl_benchmark/main/helper.c Outdated
extern int benchmark_test(void *args);

#ifdef WOLFSSL_ESPWROOM32SE
const static char* TAG = "wolfbenchmark";
Copy link
Contributor

Choose a reason for hiding this comment

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

For these "TAG", can you just standardize on static const char*? The static just makes it "private" and it should work for either build type (WOLFSSL_ESPWROOM32SE). Same applies for the other example "TAG" variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion.
Done.

IDE/Espressif/ESP-IDF/examples/wolfssl_benchmark/main/helper.c Outdated

#include "wolfssl/wolfcrypt/port/atmel/atmel.h"

int atcatls_set_callbacks(struct WOLFSSL_CTX* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function prototype is inwolfssl/wolfcrypt/port/atmel/atmel.h, so you should not need this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks. Removed.

IDE/Espressif/ESP-IDF/examples/wolfssl_client/main/client-tls.c Outdated

#include "wolfssl/wolfcrypt/port/atmel/atmel.h"

int atcatls_set_callbacks(struct WOLFSSL_CTX* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function prototype is inwolfssl/wolfcrypt/port/atmel/atmel.h, so you should not need this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

IDE/Espressif/ESP-IDF/examples/wolfssl_client/main/client-tls.c Outdated

static void ShowCiphers(void)
{
static char ciphers[4096];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be static. If its static the compiler will reserve 4096 bytes globally all the time for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. And only enabled the function when DEBUG_WOLFSSL is enabled.

IDE/Espressif/ESP-IDF/examples/wolfssl_server/main/server-tls.c Outdated

static void ShowCiphers(void)
{
static char ciphers[4096];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be static. If its static the compiler will reserve 4096 bytes globally all the time for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. And only enabled the function when DEBUG_WOLFSSL is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

The server-tls.c:51 still has static and should be removed.

IDE/Espressif/ESP-IDF/examples/wolfssl_client/main/client-tls.c Outdated
@@ -42,25 +40,115 @@
#include <wolfssl/wolfcrypt/mem_track.h>
#endif

const char *TAG = "tls_client";
#ifdef WOLFSSL_ESPWROOM32SE
static const char* TAG = "tls_client";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please standardize on static const char* for either build type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

IDE/Espressif/ESP-IDF/examples/wolfssl_server/main/server-tls.c Outdated
@@ -42,7 +42,87 @@
#include <wolfssl/wolfcrypt/mem_track.h>
#endif

const char *TAG = "tls_server";
#if defined(WOLFSSL_ESPWROOM32SE)
static const char* TAG = "tls_server";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please standardize on static const char* for either build type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

wolfssl/wolfcrypt/settings.h Outdated
#ifndef NO_ESP32WROOM32_CRYPT
#define WOLFSSL_ESP32WROOM32_CRYPT
#endif
#endif
#if defined(WOLFSSL_ESPWROOM32SE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these would be better to have in the example user_settings.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. Moved these into user_settings.h

@dgarske dgarske assigned miyazakh and unassigned dgarske Jan 16, 2019
@miyazakh miyazakh assigned dgarske and unassigned miyazakh Jan 17, 2019
dgarske
dgarske requested changes Jan 17, 2019
View reviewed changes
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Hi Hideki, just a few minor cleanup still remaining and this will be ready.

IDE/Espressif/ESP-IDF/examples/wolfssl_server/main/server-tls.c Outdated

static void ShowCiphers(void)
{
static char ciphers[4096];
Copy link
Contributor

Choose a reason for hiding this comment

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

The server-tls.c:51 still has static and should be removed.

IDE/Espressif/ESP-IDF/examples/wolfssl_server/main/server-tls.c Outdated

#include "wolfssl/wolfcrypt/port/atmel/atmel.h"

int atcatls_set_callbacks(struct WOLFSSL_CTX* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not need this as its defined in wolfssl/wolfssl/wolfcrypt/port/atmel/atmel.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

IDE/Espressif/ESP-IDF/examples/wolfssl_benchmark/main/helper.c Outdated


ESP_LOGI(TAG, "Start benchmark..");
wolf_benchmark_task( );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to cleanup this and remove the wolf_benchmark_task call here and in the benchmark.c and just use benchmark_test(NULL); below? Also these calls should be wrapped in #ifndef NO_CRYPT_BENCHMARK

Copy link
Contributor Author

@miyazakh miyazakh Jan 18, 2019 •
edited

Choose a reason for hiding this comment

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

I'd like to call wolf_benchmark_task() rather than benchmark_test(NULL) here because I'd like to pass arguments. This is nicer for me and user when we want to only see a part of benchmark.

Removed #ifndef WOLFSSL_ESPIDF in wolf_benchmark_task().
Surrounded NO_CRYPT_BENCHARK.

@dgarske dgarske assigned miyazakh and unassigned dgarske Jan 17, 2019
@miyazakh miyazakh assigned dgarske and unassigned miyazakh Jan 18, 2019
dgarske
dgarske approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Approved! Thanks Hideki San. Passing over to Todd for final review.

@dgarske dgarske assigned toddouska and unassigned dgarske Jan 18, 2019
toddouska
toddouska approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@toddouska toddouska left a comment

Choose a reason for hiding this comment

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

thanks

@toddouska toddouska merged commit d07cf53 into wolfSSL:master Jan 18, 2019
@miyazakh miyazakh deleted the Espressif_port_Phase2B branch January 19, 2019 13:12
@dgarske dgarske mentioned this pull request May 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddouska toddouska toddouska approved these changes

@dgarske dgarske dgarske approved these changes

Assignees

@toddouska toddouska

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@miyazakh @toddouska @dgarske