-
Notifications
You must be signed in to change notification settings - Fork 831
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.
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
Espressif examples run with local wolfSSL (no setup!) #6018
Conversation
Jenkins retest this please |
Including a reference to espressif/esp-idf#9929 (comment) that has this valuable comment on Espressif component CMakeLists files as related to changes in this PR:
That's where I got the idea to wrap some items in the CMake like this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over all, you seem to have a lot of duplicated code in your CMakeLists.txt. Would it make sense to have a common section of the file in a parent folder which each example references? Then you could have the unique parts in the individual examples.
Might also want to consider having a common initialization.c file for each of the main.c/wifi_connect.c files. At the very least have consistency in calling the file main.c.
|
||
#if defined(WOLFSSL_MULTI_INSTALL_WARNING) | ||
ESP_LOGI(TAG, ""); | ||
ESP_LOGI(TAG, "WARNING: Multiple wolfSSL installs found."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something that should be a compile time warning. Not runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's both: as the make/build process is rather long and the build warning could be missed.
It would be good to include the app as well for higher visibility.
# wolfSSL found in both ESP-IDF and local project - needs to be resolved by user | ||
# | ||
message(STATUS "") | ||
message(STATUS "WARNING: Found components/wolfssl in both local project and IDF_PATH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider making this an error and not just a warning. Bad/unexpected things could happen because of an unreliable location of the component source files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered making it a failure. But I wanted to to be a "it just works" feature. Sure, the user may have an different version installed, but do you think that should really make it a hard fail? This method we are pointing out which is being used, with a warning in the app (noted above).
It might however, be a good idea in a future revision to determine the versions of each and display that information as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reconsidered this and yes: I'll submit a change to make this a hard fail error. The risks of mixing code from each of the (potentially different version) component directories if the user ignores the warning seems ripe for support headaches.
message(STATUS "") | ||
message(STATUS "WARNING: wolfSSL not found.") | ||
message(STATUS "") | ||
else() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary 'else' block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Actually I was missing a message, so the else()
stayed and I added a message regarding path.
I assume you mean across the different projects? Yes. This is as intended; I realize that there could be a more efficient way for this group of examples. Instead, this design pattern is for portability: just take the |
In the latest update, I added a CMake function that is easier to understand and use. |
Jenkins retest this please |
1 similar comment
Jenkins retest this please |
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Mac:
./IDE/Espressif/ESP-IDF/compileAllExamples.sh
Found IDF_PATH = /Users/davidgarske/GitHub/esp-idf
~/GitHub/wolfssl/IDE/Espressif/ESP-IDF/examples/wolfssl_benchmark ~/GitHub/wolfssl
Traceback (most recent call last):
File "/Users/davidgarske/GitHub/esp-idf/tools/idf.py", line 45, in <module>
from idf_py_actions.tools import (executable_exists, idf_version, merge_action_lists, realpath) # noqa: E402
File "/Users/davidgarske/GitHub/esp-idf/tools/idf_py_actions/tools.py", line 1, in <module>
import click
ImportError: No module named click
~/GitHub/wolfssl
Failed in benchmark
Both python and python3 have "click" installed...
% pip install click
Requirement already satisfied: click in /usr/local/lib/python3.10/site-packages (8.1.3)
davidgarske@Davids-MBP wolfssl % pip3 install click
Requirement already satisfied: click in /usr/local/lib/python3.10/site-packages (8.1.3)
@dgarske Thanks for taking this for a test drive. Can you please confirm you have a standard ESP-IDF release install? It appears from the message that you may have a special (GitHub clone? modified? non-release?) version in your Could you please try from the default Espressif release install and let me know the output? For example: # switch to your installed test of PR6018
cd ~/
git clone https://github.com/gojimmypi/wolfssl.git wolfssl_PR6018b_test
cd ~/wolfssl_PR6018b_test/IDE/Espressif/ESP-IDF
git checkout Espressif_No_Install
# setup ESP-IDF
. ~/esp/esp-idf/export.sh
# build all the examples
./compileAllExamples.sh I'm also curious as to exactly which version you are attempting. Could you please let me know the result of I've tested this with both ESP-IDF v4.4 and v5.0 on WSL and a "real" Ubuntu VM, but I don't have a Mac to test your exact setup.
|
That worked. I needed to checkout the normal esp-idf master and run the ./install.sh. |
Note there's a new hard fail for wolfSSL being installed in both the ESP-IDF components directory and this new local component makefile. After a discussion with @bandi13 we decided it would be an unnecessary challenge for support in determining which source was being used, and perhaps even a non-zero chance of cross-contamination between them when each is a different version. |
Description
Helpful for wolfSSL Espressif first-time users: This PR allows the examples to just work without needing to manually install wolfSSL as an Espressif component.
Helpful for wolfSSL Espressif developers: This PR allows live changes to be made in the code that's actually in the repo. Otherwise the executing wolfssl code would be in the
$IDF_PATH
components directory, with any changes needing to be copied back to the local repo to be checked in to GitHub. After doing this wonky thing as part of my build process, well... enough was enough. Allowing in-place editing is how things are supposed to work.This PR also addresses the "exactly what version is this" question a bit more gracefully than the install-time edit of source code in PR#5955. This particular implementation is for the local wolfSSL examples only. The component install method will need a slightly different implementation.
See #6234 for a roadmap of Espressif updates.
Fixes zd# n/a
Testing
Open wolfSSL VisualGDB projects as usual, or run with ESP-IDF
idf.py build flash
. The examples will run without needing to install wolfssl! If wolfSSL is already installed, there will be a warning about having multiple installs, but the code should still run properly, provided the existing wolfSSL install is valid.Checklist