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

Add WireGuard VPN usermod #3270

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Add WireGuard VPN usermod #3270

merged 4 commits into from
Jul 27, 2023

Conversation

acvigue
Copy link
Contributor

@acvigue acvigue commented Jun 25, 2023

Adds a WireGuard usermod for ESP32 that allows WLED devices to join a remote network in situations where wireless isolation prevents local control.

@acvigue
Copy link
Contributor Author

acvigue commented Jul 3, 2023

Is there any chance someone could take a quick look at this?

Thank you all!

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

I have no way of testing it so I trust it works for you.
There are some things I'd like you to address before merging, though.

usermods/wireguard/wireguard.h Outdated Show resolved Hide resolved
usermods/wireguard/wireguard.h Outdated Show resolved Hide resolved
usermods/wireguard/wireguard.h Outdated Show resolved Hide resolved
@blazoncek
Copy link
Collaborator

One more thing: Please add your contact info into readme.md for user reference in case someone needs help with usermod.

@acvigue
Copy link
Contributor Author

acvigue commented Jul 21, 2023

Can you look at the changes I made?

@blazoncek
Copy link
Collaborator

Using char[xx] eats a lot of stack, which is limited on MCU.
It may interfere with WLED behaviour (which itself uses a lot of stack space on some occasions) so it would be better to use new or malloc() for all the strings you want to store in setup() or readFromConfig().

Otherwise I see you updated as requested and see no objection to this usermod.

@acvigue
Copy link
Contributor Author

acvigue commented Jul 24, 2023

fixed

@blazoncek
Copy link
Collaborator

Are you sure this works?
setup() is called after readFromConfig(). So malloc() happens after all strncpy() which is not good.

I may have unintentionally mislead you when I suggested setup(). Constructor would be a better place, and then free() in destructor. Sorry about that.

@acvigue
Copy link
Contributor Author

acvigue commented Jul 25, 2023

So malloc() happens after all strncpy() which is not good.

Oops, ok. I'll take a look at fixing it tomorrow.

@Aircoookie
Copy link
Owner

Hmm, correct me if I'm wrong, but since these keys are private members, they are created on the heap anyways as the usermod constructor is called in usermods_list.cpp. Stack would only be used if they are local variables in a given function.
Sorry that I'm only taking a look at it now, but to me this seems fine as implemented in 7296e87 ...
Either way as this is an ESP32-only mod, memory usage is far less of a concern than with mods intended for ESP8266 as well.

@acvigue
Copy link
Contributor Author

acvigue commented Jul 26, 2023

Sorry that I'm only taking a look at it now, but to me this seems fine as implemented in 7296e87 ...
Either way as this is an ESP32-only mod, memory usage is far less of a concern than with mods intended for ESP8266 as well.

All good, thanks for the input. I went ahead and reverted my branch back to that commit and then merged in all the changes from main so it should be good to go.

@blazoncek
Copy link
Collaborator

they are created on the heap anyways

You are correct and I was wrong. Mea culpa.

@Aircoookie
Copy link
Owner

All good :) I'd say this is ready to merge!

@Aircoookie Aircoookie merged commit 0ccadb2 into Aircoookie:main Jul 27, 2023
11 checks passed
@acvigue acvigue deleted the wireguard branch August 1, 2023 18:20
@blazoncek blazoncek mentioned this pull request Aug 5, 2023
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.

3 participants