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

bugfix and additional functionality for Wifi.cpp #114

Merged
merged 9 commits into from
Nov 9, 2019
Merged

bugfix and additional functionality for Wifi.cpp #114

merged 9 commits into from
Nov 9, 2019

Conversation

squonk11
Copy link
Contributor

@squonk11 squonk11 commented Nov 5, 2019

  • bugfix for Wifi::get_mac_address() which now returns the right mac address of the used interface
  • added Wifi:: get_local_mac_address(uint8_t m[6]) for getting the binary representation of the mac address
  • added Wifi::get_local_ip() for getting the IP address of the ESP32

…address of the used interface

- added Wifi:: get_local_mac_address(uint8_t m[6]) for getting the binary representation of the mac address
- added Wifi::get_local_ip() for getting the IP address of the ESP32
@squonk11
Copy link
Contributor Author

squonk11 commented Nov 5, 2019

I created the PR with my modifications. Unfortunately I see that the two checks failed:

  1. The formatting check gives the error message that file sizes changed - but why is this an issue?

  2. The compilation check complains that esp_wifi_get_mode() was not declared. But in my environment it compiles.

Do you have an idea what I have to modify?

Copy link
Owner

@PerMalmberg PerMalmberg left a comment

Choose a reason for hiding this comment

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

The format check fails because the code doesn't follow the formatting rules set in the uncrustify config. If you run it on the files in question you'll see it will reformat your code, hence the file-size change.

The reason for the other failure is that esp_wifi_get_mode isn't defined in mock-idf used when compiling Smooth for Linux/macOS:

I've not had any code that has needed it yet. If you add stubs with the correct signature for esp_wifi_get_mode it should compile.

lib/smooth/core/network/Wifi.cpp Show resolved Hide resolved
lib/smooth/include/smooth/core/network/Wifi.h Outdated Show resolved Hide resolved
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/network/Wifi.h Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/network/Wifi.h Outdated Show resolved Hide resolved
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
@PerMalmberg
Copy link
Owner

Also, please update the contributors.md file to list this PR 👍

@squonk11
Copy link
Contributor Author

squonk11 commented Nov 5, 2019

thank you for reviewing the code. I will take care about your suggestions tomorrow.

- bugfix for Wifi::get_mac_address() which now returns the right mac address of the used interface
- added Wifi:: get_local_mac_address(std::array<uint8_t, 6>& m>) for getting the binary representation of the mac address
- added Wifi::get_local_ip() for getting the IP address of the ESP32
@squonk11
Copy link
Contributor Author

squonk11 commented Nov 6, 2019

Now I saw that your last comments were in parallel with my last commit. Maybe you can check the actual status and give additional comments on that?

- added comment to "static ip4_addr_t get_local_ip()" as a warning concerning threading issue
- made "std::string get_mac_address()"static also
lib/smooth/core/network/Wifi.cpp Outdated Show resolved Hide resolved
removing unnecessary initialization of ip
@PerMalmberg
Copy link
Owner

Are you planning to update a test project to use this new functionality?

@squonk11
Copy link
Contributor Author

squonk11 commented Nov 7, 2019 via email

@PerMalmberg
Copy link
Owner

Yes, I could modify the AP test

Cool, lets finish with that before merging.

@squonk11
Copy link
Contributor Author

squonk11 commented Nov 8, 2019

I try to do it this evening.

- added test for Wifi::get_local_ip()
- added test for Wifi::get_local_mac_address(std::array<uint8_t, 6> mac)
... my original version looked nicer (see ugly indentation of lines #102 and #103)
@PerMalmberg
Copy link
Owner

I'm going to accept this as-is now, but will make a few modifications afterwards as I can't push to the master branch of your repository.

@PerMalmberg PerMalmberg merged commit 8b13300 into PerMalmberg:master Nov 9, 2019
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.

2 participants