wolfssl icon indicating copy to clipboard operation
wolfssl copied to clipboard

Software-only support for recent esp32 targets

Open dpalffy opened this issue 2 years ago • 13 comments

Description

This adds generic support for any target supported in recent esp-idf SDKs including RISC-V.

Partially fixes #5215, #6234

Testing

  • all examples build for all supported targets on esp-idf v4.4 and v5.0.
  • wolfssl_test passes all tests on an esp32c3 with both esp-idf v4.4 and v5.0
  • eolfssl_benchmark gives reasonable results on an esp32c3 with both esp-idf v4.4 and v5.0

Checklist

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

dpalffy avatar Mar 31 '23 21:03 dpalffy

Can one of the admins verify this patch?

wolfSSL-Bot avatar Mar 31 '23 21:03 wolfSSL-Bot

Hello @dpalffy and thanks for your PR and your interest in wolfSSL on the Espressif ESP32!

I don't recognize your GitHub name. Have you contributed to wolfSSL before? If not, we'll need to have a contributor agreement on file for you in order to merge this PR. If you could, please: contact [email protected] to get started.

In the meantime, are there any specifics you can share about your project? If you'd like to discuss offline, you can reach me at jim (at wolfssl.com) or we can chat on discord.

You may be interested in my WIP branch. I have quite a few changes, including support for some of the chipsets in this PR. I plan to get many of those changes merged soon. I see you've found several helpful things that changed in the latest ESP-IDF. Your #define esp_cpu_get_cycle_count() cpu_hal_get_cycle_count() is quite handy, as I've been meaning to find the new function name.

Again, thank you for your proposed changes.

gojimmypi avatar Apr 03 '23 13:04 gojimmypi

Hi @gojimmypi, thanks for looking at my changes.

This would be my first contribution, I started with the contributor agreement.

I have way too many experimental systems around my house running unsupported bootloaders, kernels at hardly accessible locations that may need console access, so I decided to start making a more "productionized" version of the wolfssh serial console example, with BLE Wifi provisioning, runtime configurable authentication, etc, and apparently the cheapest boards have the esp32c3. Still very much work in progress, but will be released when I have something working.

I was looking around for ongoing work, but I missed that branch of yours, probably the name didn't ring a bell that it may be c3 related... :) But I will check now. Do you maybe have something like https://github.com/wolfSSL/wolfssl/pull/6018 for wolfssh in the works, too?

dpalffy avatar Apr 03 '23 15:04 dpalffy

Contributor agreement approved.

kareem-wolfssl avatar Apr 03 '23 20:04 kareem-wolfssl

OK to test

dgarske avatar Apr 03 '23 20:04 dgarske

Hi @dpalffy

This would be my first contribution, I started with the contributor agreement.

Awesome! Thank you and welcome. :)

It looks like your contributor agreement is in progress. You should be hearing something soon. We appreciate you taking the time to do that.

Do you maybe have something like https://github.com/wolfSSL/wolfssl/pull/6018 for wolfssh in the works, too?

Yes, absolutely. The "no install" wolfSSL has been really quite helpful for not only development of the wolfSSL libraries, but also to make it easy to get started. I have the same plans for wolfSSH.

experimental systems ...that may need console access, so I decided to start making a more "productionized" version of the wolfssh serial console

Have you seen my Espressif wolfSSH examples for SSH-to-UART? Are you building on that? It would be really cool to have the additional capabilities you mentioned.

I've taken a closer look at this PR. You should be aware that the automated build check is very particular and enforces wolfSSL standards; Consistent coding style is very important. For example this warning:

weird control chars, hard tabs, CRs, trailing whitespace:
./wolfcrypt/src/random.c:3396:»#include <esp_random.h>
./wolfcrypt/src/random.c:3398:»#include <esp_system.h>
./wolfcrypt/src/random.c:3403:»word32 rand;
./wolfcrypt/src/random.c:3404:»while (sz > 0) {
./wolfcrypt/src/random.c:3405:»    word32 len = sizeof(rand);
./wolfcrypt/src/random.c:3406:»    if (sz < len)
./wolfcrypt/src/random.c:3407:»»len = sz;
./wolfcrypt/src/random.c:3408:»    /* Get one random 32-bit word from hw RNG */
./wolfcrypt/src/random.c:3409:»    rand = esp_random( );
./wolfcrypt/src/random.c:3410:»    XMEMCPY(output, &rand, len);

Yes, a single trailing space on a line can cause this failure. I use Visual Studio with the VisualGDB extension. There's an option to "Remove trailing whitespace on save".

At some point the folks that merge code will also ask that individual commits be squashed, too. We might get away with not doing that for your first commit. Heads up for future reference. :)

But overall, there are some nice additions here. In particular, it's very cool you found the -C3 cycle counter! Otherwise, I was doing this wonky thing with a timer.

I've confirmed your changes work with the tests and benchmark apps. Both work fine on the ESP32-C3 and the Xtensa ESP32.

Heads up @PaulMartinsen has also been an active contributor for Espressif code. We've been focusing on the ESP32-S3 but it might be a good idea to coordinate updates so that we don't duplicate efforts.

Again, thank you so much for taking the time to contribute!

gojimmypi avatar Apr 04 '23 08:04 gojimmypi

@dpalffy thanks for your prompt reply & clarifications.

btw - you may be interested in my updated wolfssl component directory that allows for project-specific user_settings.h. I'll be working on getting a PR to get those merged soon.

Oh - and check out https://github.com/wolfSSL/wolfssl/pull/6149 that will improve the way all examples show key info at startup time.

gojimmypi avatar Apr 04 '23 09:04 gojimmypi

It looks like your contributor agreement is in progress. You should be hearing something soon. We appreciate you taking the time to do that.

It should be approved by now.

Yes, absolutely. The "no install" wolfSSL has been really quite helpful for not only development of the wolfSSL libraries, but also to make it easy to get started. I have the same plans for wolfSSH.

Plans only or anything WiP I could look at?

Have you seen my Espressif wolfSSH examples for SSH-to-UART? Are you building on that? It would be really cool to have the additional capabilities you mentioned.

Yes, I want to build on that.

I've taken a closer look at this PR. You should be aware that the automated build check is very particular and enforces wolfSSL standards; Consistent coding style is very important. For example this warning:

I know that, I used to work for Google.

Yes, a single trailing space on a line can cause this failure. I use Visual Studio with the VisualGDB extension. There's an option to "Remove trailing whitespace on save".

I use vim, time to make a vimrc for wolfssl.

Maybe add something to pre-commit.sh to catch these as early as possible? Where can I find the exact rules?

At some point the folks that merge code will also ask that individual commits be squashed, too. We might get away with not doing that for your first commit. Heads up for future reference. :)

No problem, just get something finished that you would approve first.

dpalffy avatar Apr 04 '23 21:04 dpalffy

@dpalffy I've been taking a look at your code. Thanks for the updates. It's going to be a bit challenging to merge with my my WIP branch, but those are changes that have been needed ever since Espressif started coming out with new chipsets.

One of the problems I've not been able to pinpoint, is there's a significant performance difference between my branch benchmark results and your benchmark updates to master.

Although, yes: I have many fixes and improvements in my branch, I don't think I can take credit for such a performance improvement. Perhaps one or more of the gated HW sections in your update is actually doing SW, instead?

As for your question:

I use vim, time to make a vimrc for wolfssl. Where can I find the exact [wolfssl style] rules?

I've reached out to the team to see if we have a list, or perhaps even a vimrc to share.

Plans only [for no-install wolfSSH] or anything WiP I could look at?

Plans only at this time. You can see my latest wolfssl component here.

edit: one of the engineers has kindly shared their vimrc file, attached: vimrc.txt

gojimmypi avatar Apr 05 '23 19:04 gojimmypi

One of the problems I've not been able to pinpoint, is there's a significant performance difference between my branch benchmark results and your benchmark updates to master.

Although, yes: I have many fixes and improvements in my branch, I don't think I can take credit for such a performance improvement. Perhaps one or more of the gated HW sections in your update is actually doing SW, instead?

I'm pretty sure it doesn't. For the libraries, my branch should compile pretty much exactly the same code as 753ad4c4c1b393da2b918410668c553b05caf5aa (HEAD a few days ago). I'm sorry I don't have other hardware than c3 to test, can you maybe compare to that version, too? That should clarify where the difference comes from.

edit: one of the engineers has kindly shared their vimrc file, attached: vimrc.txt

Thanks!

dpalffy avatar Apr 05 '23 20:04 dpalffy

I ordered an ESP32 board to test on xtensa too, if everything goes well I might get it on Saturday.

dpalffy avatar Apr 05 '23 21:04 dpalffy

I ordered an ESP32 board to test on xtensa too, if everything goes well I might get it on Saturday.

Awesome!

I received another suggestion regarding the 80-character line length limit for vimrc:

related, I have this in my vimrc to search for long lines with just shift + q

" search for columns wider than 79
" This replaces the Ex mode activation with Q
"   https://vim.fandom.com/wiki/Highlight_long_lines#Searching
noremap Q /\%>79v.\+

As for performance, there are multiple changing variables here. I see your branch is currently about 55 commits behind master (ya, this is a pretty active repo!). Of particular interest is this "math cleanup" recently merged PR: https://github.com/wolfSSL/wolfssl/pull/6123. Still, so far, I've been unable to account for the difference.

I'd be super interested in the resultant benchmark performance difference (and thankful!) if could could apply the same proposed macro updates to my branch.

Thanks again for your assistance! Cheers.

gojimmypi avatar Apr 06 '23 07:04 gojimmypi

Hello @dpalffy - new answer to this question:

Plans only [for no-install wolfSSH] or anything WiP I could look at?

I have an initial wolfssh Espressif "no setup" cmake file here.

It's in pretty rough shape and needs some cleanup before creating a PR, but should be functional.

One of the wonky things I need to resolve is how & which wolfssl source directory to point to from wolfssh component project. At the moment, it is hard-coded to point to ../wolfssl-gojimmypi/

gojimmypi avatar Apr 10 '23 10:04 gojimmypi