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

cpu/esp*: remove dependencies on GNRC for ESP network device drivers #12967

Merged
merged 8 commits into from
Jan 31, 2020

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Dec 16, 2019

Contribution description

This PR removes the dependencies of ESP network device drivers on GNRC.

Testing procedure

The following compilations should still succeed:

  • esp_eth: make BOARD=esp32-olimex-evb -C tests/shell
  • esp_now: make BOARD=esp32-wroom-32 -C tests/lwip
  • esp_wifi: USEMODULE=esp_wifi make BOARD=esp32-wroom-32 -C tests/lwip

Issues/PRs references

Fixes #12964
Prerequisite for PR #12931

@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Dec 16, 2019
@miri64
Copy link
Member

miri64 commented Dec 16, 2019

What's with the GNRC dependencies in the driver code?

@gschorcht
Copy link
Contributor Author

@miri64 The fix can be tested without having the hardware. If the compilations succeed, there is no further dependency on gnrc.

I hope it is Ok, that I'm pushing another very small fix related to lwip with this PR. The call of esp_wifi_setup has to be placed outside the conditional compilation. Otherwise, esp_wifi_netdev is not initialized for lwip_ipv4.

@gschorcht
Copy link
Contributor Author

gschorcht commented Dec 16, 2019

What's with the GNRC dependencies in the driver code?

Are there any? Hm, I will check again, but the compilations didn't complain.

@gschorcht
Copy link
Contributor Author

Indeed, when auto_init is enabled.

@gschorcht
Copy link
Contributor Author

Could it happen that auto_init_... is called without having gnrc enabled?

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

I suggest to put the gnrc_netif initialization into auto_init_gnrc_netif as all the rest. The gnrc_netif_t (if it is needed by the device for some reason) should be a more generic context: either a pointer to a netif_t instead or just a void pointer (the latter would be preferable as not every stack might use netif_t

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

Could it happen that auto_init_... is called without having gnrc enabled?

auto_init_gnrc_netif shouldn't be, but auto_init can be used without GNRC of course.

@gschorcht
Copy link
Contributor Author

gschorcht commented Dec 16, 2019

Hm, es simple #if MODULE_GNRC wrapper should help.

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

Hm, es simple #if MODULE_GNRC wrapper should help.

Yepp as a quick-fix this is also acceptable. However, I think in the long run the device / network interface initialization should be unified across all devices.

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

Rather use #ifdef MODULE_GNRC_NETIF or MODULE_GNRC_NETIF_ETHERNET (so the least broadest dependency)

@gschorcht
Copy link
Contributor Author

I suggest to put the gnrc_netif initialization into auto_init_gnrc_netif as all the rest.

I was a bit unsure whether sys/auto_init/netif is the right location for platform specific initializations. The only other architecture I can see there is STM32.

Do you think that such a change would be good for this PR? I can do it.

@gschorcht
Copy link
Contributor Author

The gnrc_netif_t (if it is needed by the device for some reason)

To be honest, I don't remember for what porpose the network devices have this gnrc_netif_t pointer at all. The netif is assigned during setup but never used. I will remove them.

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

Do you think that such a change would be good for this PR? I can do it.

The only other SoC-network device which has the init code not in auto_init_gnrc_netif seems to be nrfmin. However for that the GNRC code is separate from the device code (including the init function). I'd say it is cleaner to have it with the CPU-specific code (as you don't need to hand it parameters), but put it into a GNRC-specific context (e.g. esp_..._gnrc.c).

@gschorcht
Copy link
Contributor Author

gschorcht commented Dec 16, 2019

I'd say it is cleaner to have it with the CPU-specific code (as you don't need to hand it parameters), but put it into a GNRC-specific context (e.g. esp_..._gnrc.c).

I will do it in that way.

The longer I'm crawling through sys/auto_init/netif the more I'm wondering why it is for gnrc only? At least it is not really name giving, neither function name auto_init_* nor the directory nor the file names

@gschorcht
Copy link
Contributor Author

I have removed the gnrc_netif_t pointer in netdevs and moved the auto intialization for gnrc in separate files called esp_*_gnrc.c.

@miri64
Copy link
Member

miri64 commented Dec 16, 2019

The longer I'm crawling through sys/auto_init/netif the more I'm wondering why it is for gnrc only? At least it is not really name giving, neither function name auto_init_* nor the directory nor the file names

Mostly historical reasons. ^^" Hopefully, this will be a bit cleaner after @jia200x's rework :-)

@gschorcht gschorcht changed the title cpu/esp32: fixes for lwIP cpu/esp32: remove dependencies on GNRC for ESP network device drivers Dec 17, 2019
@gschorcht gschorcht changed the title cpu/esp32: remove dependencies on GNRC for ESP network device drivers cpu/esp*: remove dependencies on GNRC for ESP network device drivers Dec 26, 2019
@fjmolinas
Copy link
Contributor

@miri64 have your comments been addressed?

@benpicco
Copy link
Contributor

benpicco commented Jan 31, 2020

  • esp_now: make BOARD=esp32-wroom-32 -C tests/lwip

With this I end up with an empty ifconfig output - is that to be expected?
I get something with USEMODULE=esp_wifi

(No IP in examples/gnrc_networking, but that's a known behavior SLAAC not activated…, I still get some interface information)

@miri64
Copy link
Member

miri64 commented Jan 31, 2020

@miri64 have your comments been addressed?

Which ones are you referring to? From what I can see above, I'd say yes.

@gschorcht
Copy link
Contributor Author

gschorcht commented Jan 31, 2020

  • esp_now: make BOARD=esp32-wroom-32 -C tests/lwip

With this I end up with an empty ifconfig output - is that to be expected?
I get something with USEMODULE=esp_wifi

Yes. esp-now isn't know as netdev driver in pkg/lwip/contrib. That is, there can't be a lwIP netif.

The tests are compilation tests only to prove that gnrc modules are not enabled if one of these netdev drivers is enabled.

@benpicco
Copy link
Contributor

Alright, then I'd say everything works as expected.
I didn't test esp_eth, but I assume you tested this already.
The code looks good - please squash.

@gschorcht
Copy link
Contributor Author

gschorcht commented Jan 31, 2020

(No IP in examples/gnrc_networking, but that's a known behavior SLAAC not activated…, I still get some interface information)

Yes, because it is not rebased onto a version with merged PR #12998.

@gschorcht gschorcht force-pushed the pkg/lwip/fix_esp_wifi branch from 0cd199d to fc09a1a Compare January 31, 2020 08:16
@gschorcht
Copy link
Contributor Author

gschorcht commented Jan 31, 2020

@benpicco Please wait, I just squashed and pushed a wrong branch. I had cherry picks from other PR in my branch that were merged in the meantime.

@gschorcht gschorcht force-pushed the pkg/lwip/fix_esp_wifi branch from fc09a1a to 208174a Compare January 31, 2020 08:32
@gschorcht
Copy link
Contributor Author

@benpicco I have rebased and squashed. Everything is fine now.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Changes look good and a quick test showed no regressions.

@benpicco benpicco merged commit d48471a into RIOT-OS:master Jan 31, 2020
@gschorcht gschorcht deleted the pkg/lwip/fix_esp_wifi branch January 31, 2020 09:50
@gschorcht
Copy link
Contributor Author

@benpicco Thanks for reviewing, testing and merging.

@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esp32: network devices pull in GNRC dependencies statically
5 participants