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.

Sign up for GitHub

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

Jump to bottom

Add Corewifi #245

Merged
merged 3 commits into from
Jul 15, 2021
Merged

Add Corewifi #245

merged 3 commits into from
Jul 15, 2021

Conversation

YannLocatelli
Copy link
Member

No description provided.

@YannLocatelli YannLocatelli self-assigned this May 7, 2021
@YannLocatelli YannLocatelli force-pushed the yann/feature/add-corewifi branch 3 times, most recently from f778716 to 5fb5de1 Compare June 3, 2021 18:29
ladislas
ladislas reviewed Jun 24, 2021
View reviewed changes
Copy link
Member

@ladislas ladislas left a comment

Choose a reason for hiding this comment

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

First review, it looks good to me modulo the different comments.

I'm wondering what is the use of this class, where in the chain is it used? How connecting to wifi will impact the rest of the program? What is the difference with say using Ethernet for that. Is it wifi for local network or wifi to access the internet?

Wifi being just a mean, it might be better to have a ConnectivityKit or WebKit to handle "internet" related tasks (downloading a file, uploading a file, pinging an url, etc.) and having the wifi just being the "pipe".

Maybe that's the goal here, but it's not that clear from reading the code, so a bit of explanation would greatly help :)

drivers/CoreWifi/include/CoreWifi.h Outdated Show resolved Hide resolved
drivers/CoreWifi/include/CoreWifi.h Outdated Show resolved Hide resolved
drivers/CoreWifi/source/CoreWifi.cpp Outdated Show resolved Hide resolved
include/interface/drivers/Wifi.h Outdated Show resolved Hide resolved
include/interface/drivers/Wifi.h Outdated Show resolved Hide resolved
tests/unit/mocks/mocks/mbed/WifiInterface.h Outdated Show resolved Hide resolved
tests/unit/mocks/mocks/mbed/WifiInterface.h Outdated Show resolved Hide resolved
tests/unit/mocks/mocks/mbed/WifiInterface.h Outdated Show resolved Hide resolved
@YannLocatelli YannLocatelli force-pushed the yann/feature/add-corewifi branch 3 times, most recently from e6b8d7f to 085749a Compare June 25, 2021 02:50
@leka leka deleted a comment from codecov bot Jun 25, 2021
@leka leka deleted a comment from codeclimate bot Jun 25, 2021
ladislas
ladislas reviewed Jun 25, 2021
View reviewed changes
drivers/CoreWifi/include/internal/NetworkInformation.h Outdated Show resolved Hide resolved
drivers/CoreWifi/include/CoreWifi.h Outdated Show resolved Hide resolved
drivers/CoreWifi/include/internal/NetworkInformation.h Outdated Show resolved Hide resolved
drivers/CoreWifi/include/internal/NetworkInformation.h Outdated Show resolved Hide resolved
include/interface/drivers/NetworkInformation.h Outdated Show resolved Hide resolved
include/interface/drivers/NetworkInformation.h Outdated Show resolved Hide resolved
include/interface/drivers/NetworkInformation.h Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 26, 2021 •
edited
Loading

Codecov Report

Merging #245 (dd99164) into develop (c6e0d16) will not change coverage.
The diff coverage is n/a.

❗ Current head dd99164 differs from pull request most recent head d739c30. Consider uploading reports for the commit d739c30 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #245   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines          878       878           
=========================================
  Hits           878       878

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6e0d16...d739c30. Read the comment docs.

@YannLocatelli YannLocatelli force-pushed the yann/feature/add-corewifi branch 3 times, most recently from c7bfa2c to 9a8e893 Compare July 1, 2021 13:32
ladislas
ladislas reviewed Jul 2, 2021
View reviewed changes
Copy link
Member

@ladislas ladislas left a comment

Choose a reason for hiding this comment

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

quelques questions (voir slack) puis on est bon 👍

drivers/CoreWifi/source/CoreWifi.cpp Outdated Show resolved Hide resolved
drivers/CoreWifi/source/CoreWifi.cpp Outdated Show resolved Hide resolved
spikes/lk_wifi/main.cpp Outdated

rtos::ThisThread::sleep_for(2s);

wifi_thread.start({&leka_wifi, &Wifi::start});
if (not(corewifi.connect(wifi_network))) {
Copy link
Member

Choose a reason for hiding this comment

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

je connaissais pas le not https://en.cppreference.com/w/cpp/language/operator_alternative

les discussions ici sont intéressantes: https://stackoverflow.com/questions/2376448/the-written-versions-of-the-logical-operators

Suggested change
if (not(corewifi.connect(wifi_network))) {
if (not corewifi.connect(wifi_network)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui effectivement j'avais essayé un peu par hasard le not, un héritage de python.

Je rejoins aussi la réponse validé de la discussion avec && et || qui à mon sens reflète l'idée d'opération binaire / booléenne et donc préféré à and et or.
Maintenant pour le not, je le trouve plus visuel que ! : j'ai souvent loupé en lecture le !

if (not corewifi.connect(wifi_network)) {
}

if (!corewifi.connect(wifi_network)) {
}

spikes/lk_wifi/main.cpp Show resolved Hide resolved
@YannLocatelli YannLocatelli force-pushed the yann/feature/add-corewifi branch 2 times, most recently from e0540f9 to c5f1f42 Compare July 5, 2021 17:07
@ladislas ladislas force-pushed the yann/feature/add-corewifi branch 4 times, most recently from 361d231 to 3a4221c Compare July 8, 2021 14:07
ladislas
ladislas approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@ladislas ladislas left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

YannLocatelli
YannLocatelli commented Jul 12, 2021
View reviewed changes
spikes/lk_wifi/main.cpp
if (auto is_not_connected = !corewifi.connect(wifi_network); is_not_connected) {
log_error("Wifi connection failed");
Copy link
Member Author

Choose a reason for hiding this comment

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

Il y a un déjà un message d'échec de connexion dans la fonction corewifi.connect

LekaOS/drivers/CoreWifi/source/CoreWifi.cpp

Line 34 in 3a4221c

log_debug("Connection failure.\n");

Le log_error ici peut être redondant

spikes/lk_wifi/main.cpp Outdated
auto wifi_enable = mbed::DigitalOut {WIFI_ENABLE, 1};
auto wifi_module = CoreESP8266 {WIFI_USART_TX, WIFI_USART_RX, false, WIFI_USART_RTS, WIFI_USART_CTS, WIFI_RESET};
auto corewifi = leka::CoreWifi {wifi_module, wifi_enable};
auto corewifi = leka::CoreWifi::get_default_instance();
Copy link
Member Author

Choose a reason for hiding this comment

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

Super 👍

A voir si il n'y a pas de conflit si la fonction est appelée 2 fois

Copy link
Member Author

Choose a reason for hiding this comment

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

J'ai fait des tests sur le robot, le dernier commit où cette modification apparait ne permet plus le bon fonctionnement de l'exemple: le robot ne se connecte plus du tout au réseau wifi indiqué.

Je propose de revenir au commit précédent 06bd0a8 et de penser au (vrai) singleton pour une autre PR pour faciliter l'utilisation du wifi.

Copy link
Member

Choose a reason for hiding this comment

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

ok parfait! je te laisse fixup et on merge :)

@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

YannLocatelli and others added 3 commits July 15, 2021 14:39
@YannLocatelli @ladislas
✨ (drivers): Add CoreWifi driver
da56d8e 
Co-Authored-By: ladislas <ladislas@detoldi.me>
@YannLocatelli @ladislas
🎨 (spikes): Update wifi spike to use CoreWifi
9d7ab2e 
Co-authored-by: ladislas <ladislas@detoldi.me>
@ladislas
🩹 (stubs): Fix clang tidy gpio error
d739c30
@ladislas ladislas merged commit 3c85db3 into develop Jul 15, 2021
@YannLocatelli YannLocatelli deleted the yann/feature/add-corewifi branch July 15, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ladislas ladislas ladislas approved these changes

@Kabroc Kabroc Awaiting requested review from Kabroc

Assignees

@YannLocatelli YannLocatelli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YannLocatelli @ladislas