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

build: wifi: allow espressif boards to test net/wifi use cases #80785

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sylvioalves
Copy link
Collaborator

@sylvioalves sylvioalves commented Nov 3, 2024

This aims to allow Espressif boards to test Wi-Fi drivers without binary blobs. The following changes were added:

  1. Convert NRF_WIFI_BUILD_ONLY_MODE and NXP_WIFI_BUILD_ONLY_MODE to use the same common Kconfig name BUILD_ONLY_NO_BLOBS.
  2. Modify hal_nxp and hal_espressif to support the above change. Include espressif Wi-Fi RF stubs.
  3. Modify all esp32-based boards.yml to allow net tests.

@sylvioalves sylvioalves force-pushed the enh/make_wifi_build_only_global branch from 264b271 to e00ed56 Compare November 3, 2024 00:16
@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 3, 2024

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_espressif zephyrproject-rtos/hal_espressif@980d61c zephyrproject-rtos/hal_espressif#374 zephyrproject-rtos/hal_espressif#374/files
hal_nxp zephyrproject-rtos/hal_nxp@d291bdc (master) zephyrproject-rtos/hal_nxp#457 zephyrproject-rtos/hal_nxp#457/files

DNM label due to: 2 projects with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_espressif manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Nov 3, 2024
@sylvioalves sylvioalves force-pushed the enh/make_wifi_build_only_global branch 3 times, most recently from 42bd0e8 to 95b4643 Compare November 3, 2024 00:33
@sylvioalves
Copy link
Collaborator Author

sylvioalves commented Nov 3, 2024

@krish2718 @dleach02 As you can see in this PR (commit bd260a9), I have aimed to consolidate the WIFI_BUILD_ONLY_MODE as common Kconfig. I have also submitted a similar approach to BT driver as you can see in #80786.

Please, let me know your thoughts about both. I think that we could rename XXX_BUILD_ONLY_MODE to something more clear as such as XXX_BUILD_WITH_NO_BLOBS or similar.

  1. Are you OK making this WIFI symbol global as in this PR?
  2. Does it sound good renaming it?
  3. We could create a global symbol to be used for both Wi-Fi and BT instead of splitting in 2. Any thoughts? Could be something like CONFIG_BUILD_WITH_NO_BLOBS.

@carlescufi PTAL.

@sylvioalves sylvioalves requested a review from kartben December 2, 2024 12:36
@sylvioalves
Copy link
Collaborator Author

PR was updated with the common CONFIG_BUILD_ONLY_NO_BLOBS. Will merge hal side when we get proper approvals here. Thanks.

krish2718
krish2718 previously approved these changes Dec 2, 2024
@@ -35,6 +35,13 @@ config WIFI_USE_NATIVE_NETWORKING
When selected, this hidden configuration enables Wi-Fi driver
to use native ethernet stack interface.

config BUILD_ONLY_NO_BLOBS
Copy link
Member

Choose a reason for hiding this comment

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

I guess drivers/wifi/Kconfig is the wrong place for this, now that it's completely generic. Perhaps modules/Kconfig instead?

Copy link
Member

@jhedberg jhedberg Dec 2, 2024

Choose a reason for hiding this comment

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

Btw, while I know I suggested the option name, I'm a little bit hesitant about it, since it's easily read as "Build only <something>", i.e. only "something" should be built, which is different from the actual meaning. Alternatives that come to mind are BUILD_WITHOUT_BLOBS, BLOB_STUBS, or NO_BLOBS. (sorry for adding extra hassle with this, but I think it's better to get the name right from the beginning rather than having to fix it when something has already been merged). I also don't have a super strong opinion here, i.e. if others think that BUILD_ONLY_NO_BLOBS is clear enough, then I won't object :)

Copy link
Collaborator Author

@sylvioalves sylvioalves Dec 2, 2024

Choose a reason for hiding this comment

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

Of course, I need to move it out of wifi section.
My initial suggestion as described in this PR was CONFIG_BUILD_WITH_NO_BLOBS. Sounds CONFIG_BUILD_WITHOUT_BLOBS is even better. Are you ok with it? I can rename it again and wait for feedbacks.

Copy link
Member

Choose a reason for hiding this comment

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

BUILD_WITHOUT_BLOBS is fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

BUILD_WITHOUT_BLOBS is fine, ONLY was added to imply that this is build-only and cannot be used on a real board for testing.

Copy link
Member

Choose a reason for hiding this comment

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

BUILD_WITHOUT_BLOBS is fine, ONLY was added to imply that this is build-only and cannot be used on a real board for testing.

Yeah, that was my idea when I proposed it, but then I realized that when you try to read it as normal English it's mildly confusing. Either way is fine with me.

Copy link
Member

Choose a reason for hiding this comment

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

Also, as long as we output a clear warning during build, I think that should do the job of informing the user that the result is something non-functional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Considering the commments above, I would keep as is: BUILD_ONLY_NO_BLOBS.
There is a big warning banner during build that states that it is not for real-world operation, but, BUILD_WITHOUT_BLOBS leaves a bit of margin to bad interpretation as it sounds to "work" without blobs.

@kartben kartben assigned jukkar and unassigned kartben Dec 2, 2024
jukkar
jukkar previously approved these changes Dec 2, 2024
wmrsouza
wmrsouza previously approved these changes Dec 2, 2024
@jhedberg
Copy link
Member

jhedberg commented Dec 9, 2024

@sylvioalves some rebasing is needed. I'd be happy to approve once the Kconfig option has been moved away from its current wifi-specific location.

@sylvioalves
Copy link
Collaborator Author

sylvioalves commented Dec 9, 2024

@sylvioalves some rebasing is needed. I'd be happy to approve once the Kconfig option has been moved away from its current wifi-specific location.

I was expecting hal_nxp to be merged so that I could update this PR accordingly.
Will do so.

Convert vendor specific **_WIFI_BUILD_ONLY_MODE symbol as global
in order to provide common build flag to enable CI with no blobs.

Signed-off-by: Sylvio Alves <[email protected]>
Add stubs to allow building Wi-Fi interface
without binary blobs. Only for CI tests.

Update ESP32 Wi-Fi kconfig entry accordingly.

Signed-off-by: Sylvio Alves <[email protected]>
Allow espressif platform to be properly tested in CI.

Signed-off-by: Sylvio Alves <[email protected]>
This lets esp32-based boards to be tested regarding
Wi-Fi changes.

Signed-off-by: Sylvio Alves <[email protected]>
@sylvioalves sylvioalves dismissed stale reviews from wmrsouza, jukkar, and krish2718 via e59899c December 9, 2024 13:20
@sylvioalves sylvioalves force-pushed the enh/make_wifi_build_only_global branch from 269d00b to e59899c Compare December 9, 2024 13:20
@zephyrbot zephyrbot added the platform: nRF Nordic nRFx label Dec 9, 2024
@sylvioalves sylvioalves requested a review from jhedberg December 9, 2024 13:22
@dleach02 dleach02 requested a review from jukkar December 9, 2024 15:39
@dleach02
Copy link
Member

dleach02 commented Dec 9, 2024

@sylvioalves some rebasing is needed. I'd be happy to approve once the Kconfig option has been moved away from its current wifi-specific location.

I was expecting hal_nxp to be merged so that I could update this PR accordingly. Will do so.

This PR is tricky because we don't want to merge the NXP HAL until this one is fully ready to merge. There are a lot of NXP HAL PRs that need to be synchronized. The ESP hal PR has merged so I would update this PR with the SHA.

@jukkar seems to have one request yet on a kconfig change, has that been done?

@sylvioalves
Copy link
Collaborator Author

@sylvioalves some rebasing is needed. I'd be happy to approve once the Kconfig option has been moved away from its current wifi-specific location.

I was expecting hal_nxp to be merged so that I could update this PR accordingly. Will do so.

This PR is tricky because we don't want to merge the NXP HAL until this one is fully ready to merge. There are a lot of NXP HAL PRs that need to be synchronized. The ESP hal PR has merged so I would update this PR with the SHA.

@jukkar seems to have one request yet on a kconfig change, has that been done?

We can wait another round of approval and sync up when you feel confortable merging hal_nxp PR. I do not see any additional request.

@sylvioalves
Copy link
Collaborator Author

@dleach02 I've rebased hal_nxp PR. Will rebase this one and submit again once merged in there. Up to you now.

@sylvioalves
Copy link
Collaborator Author

@dleach02, are you able to move on with this? Do you expect anything else from my side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: Samples Samples area: Sockets Networking sockets area: Wi-Fi Wi-Fi DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_espressif manifest-hal_nxp platform: ESP32 Espressif ESP32 platform: nRF Nordic nRFx platform: NXP Drivers NXP Semiconductors, drivers Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants