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

New component esp_wifi_remote #516

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

david-cermak
Copy link
Collaborator

@david-cermak david-cermak commented Feb 28, 2024

Initial version

TODO

** This version **

  • Add README and docs
  • Cleanup git history
  • new component stuff (changelog, versions)

** Next versions **

  • Add APIs from esp_wifi_he.h
  • Add cross-check that slave and host wifi config matches
  • Add coexistence of the wifi_remote and wifi_native configurations

@david-cermak david-cermak self-assigned this Feb 28, 2024
@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

*
* SPDX-License-Identifier: Apache-2.0
*/
#include <esp_private/wifi.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

private may create issues?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@mantriyogesh
Copy link
Collaborator

Hello @david-cermak ,

I had reviewed the changes, looks fine. But let me test these and confirm.
Some small code changes I had to do while running, which I will attach as patch here..

components/esp_wifi_remote/README.md Outdated Show resolved Hide resolved
components/esp_wifi_remote/include/esp_wifi_remote.h Outdated Show resolved Hide resolved
dependencies:
idf:
version: '5.3'
espressif/esp_hosted:
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

components/esp_wifi_remote/idf_component.yml Show resolved Hide resolved
components/esp_wifi_remote/scripts/parse_header.py Outdated Show resolved Hide resolved
components/esp_wifi_remote/test/smoke_test/sdkconfig.ci.6 Outdated Show resolved Hide resolved
}


esp_err_t esp_wifi_internal_set_sta_ip(void)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

components/esp_wifi_remote/wifi_remote_net.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@alisitsyn alisitsyn left a 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/README.md Outdated Show resolved Hide resolved
components/esp_wifi_remote/scripts/parse_header.py Outdated Show resolved Hide resolved
components/esp_wifi_remote/scripts/check_headers.sh Outdated Show resolved Hide resolved
components/esp_wifi_remote/README.md Outdated Show resolved Hide resolved
ci/ignore_astyle.txt Show resolved Hide resolved
components/esp_wifi_remote/scripts/README.md Show resolved Hide resolved
components/esp_wifi_remote/scripts/README.md Show resolved Hide resolved
@@ -0,0 +1,5 @@
/*
* SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

components/esp_wifi_remote/README.md Outdated Show resolved Hide resolved
@david-cermak david-cermak force-pushed the feat/wifi_remote branch 4 times, most recently from c033d07 to 75e2a37 Compare March 22, 2024 11:53
@david-cermak
Copy link
Collaborator Author

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.

@mantriyogesh
Copy link
Collaborator

hosted_hooked_with_esp_wifi_remote.patch

@david-cermak
Copy link
Collaborator Author

hosted_hooked_with_esp_wifi_remote.patch

Thanks for sharing the patch. I've updated the PR accordingly and simplified the component a little:

  • removed esp_wifi_remote_slave_init() altogether while making the channel operations public -- helps esp_hosted to implement it based on public channels
  • defined most of the channel operations WEAK, so esp_hosted can update them
  • made the minor modification and updates (logs, nullptr checks)

@david-cermak
Copy link
Collaborator Author

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!

@david-cermak david-cermak merged commit cb682a7 into espressif:master Apr 2, 2024
27 checks passed
kassane added a commit to kassane/zig-esp-idf-sample that referenced this pull request Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants