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

wifi: esp32: enhance handling of AP connect events #75786

Merged
merged 1 commit into from
Dec 16, 2024
Merged

wifi: esp32: enhance handling of AP connect events #75786

merged 1 commit into from
Dec 16, 2024

Conversation

ndrs-pst
Copy link
Contributor

@ndrs-pst ndrs-pst commented Jul 11, 2024 •
edited
Loading

This commit aims to improve the integration of esp_wifi_drv by
providing the link mode information to wifi_mgmt when a station device
is connected to the AP.

@zephyrbot zephyrbot added platform: ESP32 Espressif ESP32 area: Wi-Fi Wi-Fi labels Jul 11, 2024
@marekmatej
Copy link

hi @ndrs-pst thanks for the contribution! Would you mind re-write the commit message, so it is more clear what was intended to achieve here?

@ndrs-pst
Copy link
Contributor Author

Yes, for sure. So if you can guide the direction of the message it would be great.

krish2718
krish2718 reviewed Jul 11, 2024
View reviewed changes
drivers/wifi/esp32/src/esp_wifi_drv.c Outdated
Comment on lines 732 to 778
.scan = esp32_wifi_scan,
.connect = esp32_wifi_connect,
.disconnect = esp32_wifi_disconnect,
.ap_enable = esp32_wifi_ap_enable,
.ap_disable = esp32_wifi_ap_disable,
.iface_status = esp32_wifi_status,
.scan = esp32_wifi_scan,
.connect = esp32_wifi_connect,
.disconnect = esp32_wifi_disconnect,
.ap_enable = esp32_wifi_ap_enable,
.ap_disable = esp32_wifi_ap_disable,
.iface_status = esp32_wifi_status,
#if defined(CONFIG_NET_STATISTICS_WIFI)
.get_stats = esp32_wifi_stats,
.get_stats = esp32_wifi_stats,
#endif
};

static const struct net_wifi_mgmt_offload esp32_api = {
.wifi_iface.iface_api.init = esp32_wifi_init,
.wifi_iface.iface_api.init = esp32_wifi_init,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just whitespace noise, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback about the whitespace noise. I've already removed it from the commit.
By the way, could you let me know when or in what situations I can remove such whitespace?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, if it was intentional, then you can move that to a separate commit with commit logs detailing the reason. The code review is done per-commit esp. for large PR, so, thumb rule is one logical change per-commit.

@marekmatej
Copy link

Yes, for sure. So if you can guide the direction of the message it would be great.

Ok, so perhaps, instead of naming the function calls used (which is visible in the diff), you could write about actual improvement that change brings to the esp32 wifi management.

@ndrs-pst ndrs-pst force-pushed the pr_wifi_esp32_enhance_ap_connect_disconnect branch from 7d330cc to 1e535cf Compare July 12, 2024 05:51
krish2718
krish2718 previously approved these changes Jul 12, 2024
View reviewed changes
marekmatej
marekmatej previously approved these changes Jul 12, 2024
View reviewed changes
sylvioalves
sylvioalves previously approved these changes Jul 12, 2024
View reviewed changes
LucasTambor
LucasTambor previously approved these changes Jul 12, 2024
View reviewed changes
jukkar
jukkar previously approved these changes Jul 12, 2024
View reviewed changes
@ndrs-pst ndrs-pst force-pushed the pr_wifi_esp32_enhance_ap_connect_disconnect branch from 1e535cf to 68453eb Compare July 12, 2024 16:45
@ndrs-pst
Copy link
Contributor Author

Sorry for the inconvenience. I just realized that the link mode assignment was wrong.

  • phy_11n should be WIFI_4.
  • phy_11g should be WIFI_3.

There is no 802.11a, which is WIFI_2. 🫡

krish2718
krish2718 reviewed Jul 12, 2024
View reviewed changes
drivers/wifi/esp32/src/esp_wifi_drv.c Outdated
sta_info.link_mode = WIFI_LINK_MODE_UNKNOWN;
memcpy(sta_info.mac, event->mac, WIFI_MAC_ADDR_LEN);
sta_info.mac_length = WIFI_MAC_ADDR_LEN;
sta_info.twt_capable = false; /* Only support in 802.11ax */
Copy link
Collaborator

Choose a reason for hiding this comment

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

another way would be to just memset the sta_info and only fill in non-default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that most of the data is non-zero except for twt_capable, memset may not be the choice in this situation. 😅

@ndrs-pst ndrs-pst requested a review from marekmatej July 31, 2024 08:27
@ndrs-pst ndrs-pst requested a review from sylvioalves August 5, 2024 19:20
jukkar
jukkar previously approved these changes Aug 6, 2024
View reviewed changes
@ndrs-pst ndrs-pst dismissed stale reviews from jukkar and marekmatej via 5658f22 September 8, 2024 11:45
@ndrs-pst ndrs-pst force-pushed the pr_wifi_esp32_enhance_ap_connect_disconnect branch from 68453eb to 5658f22 Compare September 8, 2024 11:45
@ndrs-pst ndrs-pst changed the title wifi: esp32: enhance handling of AP connect/disconnect events wifi: esp32: enhance handling of AP connect events Sep 8, 2024
uLipe
uLipe previously approved these changes Sep 11, 2024
View reviewed changes
jukkar
jukkar previously approved these changes Sep 12, 2024
View reviewed changes
@kartben
Copy link
Collaborator

kartben commented Oct 16, 2024

Ping @sylvioalves

@dkalowsk dkalowsk added this to the v4.1.0 milestone Oct 29, 2024
@kartben
Copy link
Collaborator

kartben commented Nov 21, 2024

@sylvioalves Please take a look :)

@sylvioalves
Copy link
Collaborator

@ndrs-pst sorry it took me so long to check this, I just missed (a few times). LGTM. Would you mind taking the clang-format suggestions regarding comments and code style?

@ndrs-pst
wifi: esp32: enhance handling of AP connect events
d62ccb0 
This commit aims to improve the integration of `esp_wifi_drv` by
providing the link mode information to `wifi_mgmt` when a station device
is connected to the AP.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst dismissed stale reviews from jukkar and uLipe via d62ccb0 November 22, 2024 04:42
@ndrs-pst ndrs-pst force-pushed the pr_wifi_esp32_enhance_ap_connect_disconnect branch from 5658f22 to d62ccb0 Compare November 22, 2024 04:42
@ndrs-pst
Copy link
Contributor Author

@ndrs-pst sorry it took me so long to check this, I just missed (a few times). LGTM. Would you mind taking the clang-format suggestions regarding comments and code style?

Sure, I've already addressed it. :)

@ndrs-pst
Copy link
Contributor Author

Ping @sylvioalves 👀

@kartben kartben merged commit 60a2888 into zephyrproject-rtos:main Dec 16, 2024
25 checks passed
@ndrs-pst ndrs-pst deleted the pr_wifi_esp32_enhance_ap_connect_disconnect branch December 16, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar approved these changes

@marekmatej marekmatej marekmatej approved these changes

@sylvioalves sylvioalves sylvioalves approved these changes

@wmrsouza wmrsouza wmrsouza approved these changes

@Kludentwo Kludentwo Awaiting requested review from Kludentwo

@rlubos rlubos Awaiting requested review from rlubos

@krish2718 krish2718 Awaiting requested review from krish2718

@raffarost raffarost Awaiting requested review from raffarost

@LucasTambor LucasTambor Awaiting requested review from LucasTambor

@uLipe uLipe Awaiting requested review from uLipe

Assignees

@sylvioalves sylvioalves

Labels
area: Wi-Fi Wi-Fi platform: ESP32 Espressif ESP32
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.