Skip to content

Conversation

gojimmypi
Copy link
Contributor

Description

This PR addresses #5795 and cleans up the benchmark and test Espressif examples.

Of particular interest:

  • Update wolfcrypt esp32_sha.c and sha256.c to now report unexpected reentry as verbose log rather than error message.
  • wolfcrypt/test.c now returns args.return_code when WOLFSSL_ESPIDF is defined. (here)
  • wolfcrypt/test.h now declares int wolf_test_task(void) when WOLFSSL_ESPIDF is defined. (here)

Other changes:

  • Benchmark example no longer runs tests; Each example is fully separate
  • Fixed Test example that was missing code files; No longer installed locally, uses latest installed version.
  • setup.sh no longer copies benchmark.c(.h) to local project directory.
  • Rename benchmark/main and test/main filenames from helper.c to main.c, cleaned up code.
  • Added main.h
  • Revised main/CMakeLists.txt to use only main.c
  • Set components main and wolfssl for project CMakeLists.txt
  • Added libs/Tigard.cfg file for Tigard JTAG debugger.
  • Update sdkconfig.defaults with compiler optimizations and stack check.
  • Added VisualGDB Project file & Visual Studio solution file.
  • Added optional time_helper for wolfssl_test
  • Exclude ssl_misc.c in component cmake to fix warning: #warning ssl_misc.c does not need to be compiled separately from ssl.c

Fixes zd# (n/a)

Testing

Delete wolfssl component in ESP-IDF components directory. Reminder: there may be multiple versions in esp32\esp-idf\vX.Y.Z

Install fresh wolfssl component with ./setup.sh from ESP-IDF directory.

Assuming Espressif ESP-IDF in /mnt/c/SysGCC/esp32/esp-idf (e.g. VisualGDB), ensure ESP-IDF environment is configured:

. /mnt/c/SysGCC/esp32/esp-idf/v4.4.1/export.sh

Assuming ESP32 on COM20 / ttyS20:

 idf.py clean build flash -p /dev/ttyS20 -b 921600 monitor

Observe output.

Successful output examples added to benchmark and test README.md files.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi self-assigned this Nov 18, 2022
@dgarske dgarske self-assigned this Nov 21, 2022
@gojimmypi gojimmypi linked an issue Nov 21, 2022 that may be closed by this pull request
@dgarske dgarske requested review from JacobBarthelmeh and removed request for cconlon and kaleb-himes November 21, 2022 18:18
@dgarske dgarske assigned JacobBarthelmeh and unassigned dgarske and gojimmypi Nov 21, 2022
@miyazakh
Copy link
Contributor

miyazakh commented Nov 22, 2022

Run crypt, bench, client and server on my MCU, ESP32-D0WDQ6-V3. Crypt and Bench work fine! On client and server, I am seeing many message like below:

I (208745) wolf_hw_sha: >>>> Hardware in use; Mode REVERT to ESP32_SHA_SW
I (208745) wolf_hw_sha: >>>> Hardware in use; Mode REVERT to ESP32_SHA_SW
I (208745) wolf_hw_sha: >>>> Hardware in use; Mode REVERT to ESP32_SHA_SW
I (208755) wolf_hw_sha: >>>> Hardware in use; Mode REVERT to ESP32_SHA_SW
I (208755) wolf_hw_sha: >>>> Hardware in use; Mode REVERT to ESP32_SHA_SW

We would want to display these message when user sets verbose to high?

@@ -0,0 +1,23 @@
# Espressif Updates
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want these new files to get bundled into the wolfSSL dist? They will need added to one of the include.am's if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I don't know. What are the benefits of including in wolfSSL dist?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little subjective if this UPDATE.md would be helpful or not in the distribution, I'd recommend though giving the dist bundle a try and making sure all files wanted are in it. On a unix like system with autoconf this is doing something like "./configure && ./config.status && make dist-zip" which will make the wolfssl-X.X.X.zip distribution having similar files to what would be sent out in release bundles.

@gojimmypi
Copy link
Contributor Author

We would want to display these message when user sets verbose to high?
@miyazakh yes, the ESP_LOGI has been changed to ESP_LOGV

@kaleb-himes I've added a few commits to address your suggestions.

I've also added 4 new files for VisualGDB projects created when I was testing the wolfssl_client and wolfssl_server examples.

Also - there's a new default stack size of 55,000 for the benchmark sdkconfig.

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.

Looks good, just a few small things.

*/
InitSha256(sha256); /* unlock mutex, set mode to ESP32_SHA_INIT */
ESP_LOGE("sha256", "ERROR: hardware unlock needed in wc_Sha256Free");
ESP_LOGV("sha256", "ERROR: hardware unlock needed in wc_Sha256Free");
Copy link
Contributor

Choose a reason for hiding this comment

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

This indicates "ERROR" but is flagged as verbose. Please fix the message or leave as LOGE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text of the message has been updated.

@@ -0,0 +1,30 @@
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

I am okay with pragma, but the #ifndef _TIME_HELPER_H syntax is more portable.

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 was not aware of portability issues. Noted and changed.


#ifdef __cplusplus
} /* extern "C" */
#endif No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you have a new line at end.

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.


#ifndef TIME_ZONE
#define TIME_ZONE "PST-8"
#endif // !#define TIME_ZONE
Copy link
Contributor

Choose a reason for hiding this comment

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

Use C style /* */ comments. Thanks!

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

/* free slot array */
void my_atmel_free(int slotId)
{
if(slotId >= 0 && slotId < ATECC_MAX_SLOT){
Copy link
Contributor

Choose a reason for hiding this comment

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

if ( and ) {

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

/* allocate slot depending on slotType */
int my_atmel_alloc(int slotType)
{
int i, slot = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ATECC_INVALID_SLOT, not -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, much better.

@dgarske dgarske assigned gojimmypi and unassigned dgarske Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Espressif benchmark and test examples fail to compile

4 participants