-
Notifications
You must be signed in to change notification settings - Fork 136
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
New component esp_wifi_remote
#516
Conversation
fab7b03
to
edc3c2d
Compare
ab37f98
to
b93f373
Compare
9803e8b
to
b14d37b
Compare
2a7635c
to
25e0788
Compare
25e0788
to
8896d10
Compare
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
#include <esp_private/wifi.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private may create issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out! I agree, I think we can remove it now (at least for the test to pass), but we should check with esp_hosted
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep it as is, since we're using wifi netifs as esp_hosted channels. we need to use wifi_internal_
APIs for it.
Hello @david-cermak , I had reviewed the changes, looks fine. But let me test these and confirm. |
dependencies: | ||
idf: | ||
version: '5.3' | ||
espressif/esp_hosted: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't better to set a base version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove esp_hosted
dependency for the initial version, but will add it later with more strict deps
} | ||
|
||
|
||
esp_err_t esp_wifi_internal_set_sta_ip(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a LOG stating that this is not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to check where it is used. but will definitely add an error/debug log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a shortcut at the moment, this API is called from netif layers and notifies wifi library that we got an IP.
I've added a TODO:...
comment, that we need to transfer this info to the slave device and all esp_wifi_internal_set_sta_ip()
there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left just minor comments.
components/esp_wifi_remote/test/smoke_test/components/esp_hosted/include/esp_hosted_mock.h
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,5 @@ | |||
/* | |||
* SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How it is handled when year changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll think about a better way and disscus offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the workflow, but added a description in the checker what to do and which file to update if it fails.
c033d07
to
75e2a37
Compare
Hi guys, I've addressed most of the comments, could you please take another look and check mainly the unresolved discussions if it's okay with you? I likely won't be able to resolve it correctly with the initial version, though. |
Thanks for sharing the patch. I've updated the PR accordingly and simplified the component a little:
|
25cd6a9
to
d053d67
Compare
Thanks @euripedesrocha for the review. I'm taking the liberty of merging this now, I'll address the outstanding comments in the upcoming version. Thanks everyone for having a look! |
Initial version
TODO
** This version **
** Next versions **
esp_wifi_he.h