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.

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

Hotfix to porting library to arduino-esp32 v3 #90

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

GotRobbd
Copy link
Contributor

@GotRobbd GotRobbd commented Jul 11, 2024

This pull request fixes issue #83 , as Espressif has introduced a new version of MbedTLS into their framework. This in term breaks compatibility with the current version of SSLClient. These changes are not backwards compatible with arduino-esp32 v2

@RobertByrnes RobertByrnes self-requested a review July 11, 2024 18:46
@RobertByrnes
Copy link
Collaborator

RobertByrnes commented Jul 11, 2024

Hi @GotRobbd Firstly, this is great to see.

I think what we will need to see are these changes with a feature flag wrapped around them, at least for now (using a #define probably - off of something related to the environment). That way, this isn't a breaking change. We could also issue a compiler warning ⚠️ depending on how this is achieved.

From the look I gave this topic we need to cater for at leaat PlaformIO arduino-espressif32, PlatformIO espidf and also the Arduino IDE and ensure we can get compilation in all environments.

Also, we need to get the tests working. I am very familiar with the test structure and framework, so happy to help.

@RobertByrnes RobertByrnes linked an issue Jul 11, 2024 that may be closed by this pull request
@RobertByrnes RobertByrnes added the enhancement New feature or request label Jul 11, 2024
@RobertByrnes RobertByrnes changed the base branch from master to v1.3.0 July 11, 2024 19:35
@RobertByrnes
Copy link
Collaborator

RobertByrnes commented Jul 11, 2024

Have also changed the base branch from master to v1.3.0 as this is the next release branch and a few other update will go in with this...

Copy link
Collaborator

@RobertByrnes RobertByrnes left a comment

Choose a reason for hiding this comment

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

All the code changes I have suggested for this PR wrap your changes in a feature flag. These changes are great by the way. Given that we have to manually configure the platformio.ini file for this to work the feature flag will work from either adding the a define build flag in the platformio.ini file:

build_flags =
    -D MBEDTLS_3X_FEATURE

Or by adding the define in code above the include:

#define MBEDTLS_3X_FEATURE

Please make these changes. The other thing we need to is use a similar flag for the unit tests.

src/certBundle.c Show resolved Hide resolved
src/ssl__client.cpp Show resolved Hide resolved
src/ssl__client.cpp Show resolved Hide resolved
@duyle1402
Copy link

Just a suggestion, when using Arduino IDE, like @GotRobbd mentioned, these changes are not backwards compatible with arduino-esp32 v2. Can we use a define like #if (ESP_ARDUINO_VERSION_MAJOR >= 3) to keep these changes compatible with arduino-esp32 v2? For example, we can wrap the changes in:

#if (ESP_ARDUINO_VERSION_MAJOR >= 3)
  #include <mbedtls/net_sockets.h>
#else 
  #include <mbedtls/net.h>
#endif

This way, the code will work with both versions based on the MbedTLS library used.

@GotRobbd
Copy link
Contributor Author

I think @duyle1402 has a point. It might be better to use already defined macros instead of adding more friction. ESP_ARDUINO_VERSION_MAJOR is already part of arduino-esp32 only, so we can use that to differentiate my additions from the rest of the code, whilst maintaining compatibility with other platforms/environments.

@GotRobbd
Copy link
Contributor Author

I found out that MbedTLS does have versioning macros as well. I will use those instead since certBundle.c does not like ESP_ARDUINO_VERSION_MAJOR due to it not including Arduino as a library. MBEDTLS_VERSION_MAJOR should suffice for now.

@duyle1402
Copy link

Thank you, @GotRobbd, for the quick update. Using MBEDTLS_VERSION_MAJOR is a great approach to ensure compatibility. It's fantastic to know that this will keep the code functional across different versions without adding extra complexity.

I hope @RobertByrnes can release the update soonest possible, as many people might need to migrate their projects.

@RobertByrnes
Copy link
Collaborator

@GotRobbd @duyle1402 Hi Both, yeah this is looking great now. Only thing left to sort is the tests.

@GotRobbd I'll pull the master of your fork and PR to that and then you can push that change to this PR.

It'll run those test again and we are good to go.

I'll merge this once they pass to v1.3.0 as I've got some other stuff to go into the next release.

Should be on PlatformIO package repo and Arduino lib manager within the week!

@RobertByrnes
Copy link
Collaborator

@GotRobbd @duyle1402 I did notice one gotcha with this but it won't stop a release with a feature flag:

When the platform updates are pulled in via:

platform_packages =
    framework-arduinoespressif32 @ https://github.com/espressif/arduino-esp32.git#3.0.2
    framework-arduinoespressif32-libs @ https://github.com/espressif/arduino-esp32/releases/download/3.0.2/esp32-arduino-libs-3.0.2.zip

SSLClient compiles fine but that may break others! Probably because PlatformIO doesn't officially support the newer versions yet and therefore, vendor libraries are not updated for compatibility yet. For example, ArduinoHttpClient.h failed to compile.

build_flags =
    -std=gnu++2a
    -std=c++11
    -std=gnu++11

build_unflags = -std=gnu++2b

I haven't yet had time to investigate the build flag changes. @GotRobbd is this a requirement for the new platform package version?

@GotRobbd
Copy link
Contributor Author

@RobertByrnes the build flags and unflags are optional. I included them because I thought they may be useful to prevent compilation errors in the first place, since I use them in my workspace. You can remove them and it should compile just fine. Sorry for the confusion.

@RobertByrnes RobertByrnes changed the base branch from v1.3.0 to master July 12, 2024 21:55
@RobertByrnes RobertByrnes merged commit 9f4812b into govorox:master Jul 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Will ESP32 core 3.0.0 MBEDTLS issue be an issue going forward?
3 participants