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/esp8266: esp-now network device support #9917

Merged
merged 9 commits into from
Mar 6, 2019
Merged

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Sep 11, 2018

Contribution description

This PR introduces the ESP-NOW netdev device driver. The ESP-NOW netdev device allows ESP8266 nodes module to communicate using the built-in WiFi interface and the GNRC stack.

Test procedure

Since ESP-NOW is a technology that only works between ESP nodes, at least two esp8266 boards are needed for testing. The easiest way to test is to use examples/gnrc_networking. Once the boards have been flashed with the following make command

USEMODULE=esp_now make flash -C examples/gnrc_networking BOARD=...

the commands ifconfig and ping6 can be used for testing.

Issues/PRs references

This PR depends on #10126, #10536, #10581, #10656, #10700.

@gschorcht gschorcht changed the title cpu/esp8266: minor makefile and doc changes cpu/esp8266: esp-now network device support Sep 11, 2018
@RIOT-OS RIOT-OS deleted a comment Sep 11, 2018
@RIOT-OS RIOT-OS deleted a comment Sep 11, 2018
@RIOT-OS RIOT-OS deleted a comment Sep 11, 2018
@RIOT-OS RIOT-OS deleted a comment Sep 11, 2018
@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties labels Sep 11, 2018
@emmanuelsearch
Copy link
Member

@gschorcht nice! How does one test this PR?

@gschorcht
Copy link
Contributor Author

@emmanuelsearch Please refer the updated description.

Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

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

Without having a deeper look into the code (yet), I took two node-mcu clones and checked examples/gnrc_networking as mentioned in the PR description. And it works!
Not that I had any doubts about that ... :)

I did also a quick check with tests/gnrc_udp and involved a third device with success.

Two observations:
In gnrc_udp when using udp send with more than 202 bytes no packet has been received on the server side.

The USEMODULE=esp_now should get some kind conditional check. As dumb as I am, I ran make without it by accident. It builds without error, flashing is done, but then the node goes into a crazy "PANIC,reboot" loop. Even the tty-device disappeared.

@gschorcht
Copy link
Contributor Author

@A-Paul Many thanks for your test.

Don't wonder please, when I was investigating your problems, I realized some minor inconsistencies in some functions when debugging output is enabled. So, I have fixed them with the last commit and would like to include them with this PR. These are in detail:

  • hexdump was renamed somewhere in the past to esp_hexdump (cpu/esp8266/esp-now/esp_now_netdev)
  • missing spaces after format specifier in esp_hexdump (cpu/esp8266/tools.c)
  • hex format specifier are not supported by ets_printf (cpu/esp8266/include/sys/_intsup.h)
  • module netopt is required when debug output is enabled (cpu/esp8266/Makefile.include)

As stated, I could realize these inconsistencies only after setting ENABLE_DEBUG.

@gschorcht
Copy link
Contributor Author

Two observations:
In gnrc_udp when using udp send with more than 202 bytes no packet has been received on the
server side.

That behavior seems to be caused by upper layers of GNRC or the application. esp-now netdev allows messages with a maximum length of 250 bytes and. Since it is of type NETDEV_TYPE_RAW, IPv6 packets are embedded without changes in esp-now messages. Since the IPv6 header has 40 byte and the UDP header has 8 byte, you can send 202 bytes at maximum as UDP payload. The maximum packet size is reported correctly by the esp-now netdev with NETOPT_MAX_PACKET_SIZE and seems to be is used by upper layers. Although you see the message Success: send 203 byte to ..., the _send function of the esp-now netdev is not called. Probably, the size is checked in the upper layer. Maybe, @miri64 can explain why you a success message instead of an an error message when overlength packet are tried to be sent. Maybe the return code is not handled in tests/gnrc_udp/main.c.

The USEMODULE=esp_now should get some kind conditional check. As dumb as I am, I ran make without it by accident. It builds without error, flashing is done, but then the node goes into a crazy "PANIC,reboot" loop. Even the tty-device disappeared.

Yes, that is a problem. The short answer is: It is not reasonable to have such a conditional check and the application developer has to take care.

The long asnwer: It is a bit mode tricky and rather a configuration problem of the application. Using gnrc_* tests or examples usually activate module gnrc_netdev_default. Using module gnrc_netdev_default requires that the board has at least one netdev since GNRC_NETIF_NUMOF is set automatically to 1 in sys/include/net/gnrc/netif/conf.h if it is not already declared by application's makefile. That's why you need at least one netdev, if module gnrc_netdev_default is used. Having no netdev leads to the PANIC reboot loop in GNRC you have seen when it tries to initialize the netdevs.

Yes, this dependency is usually solved by the board's Makefile.dep

ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
  USEMODULE += esp_now
endif

And yes, it would be possible to add this dependency. But, here comes the problem. RIOT port for ESP8266 is a bare metal implementation. However, when you activate esp-now netdev, the RIOT port has to use the proprietary binary blob of the ESP8266 SDK to get acces to the built-in WiFi. This ESP8266 SDK

  • increases the code size by 160 kByte (this is the double for tests/gnrc_udp),
  • reduces the available memory in heap from 64 kByte to only 38 kByte,
  • decreases the performance to the half,
  • uses an own interrupt handling which competes with RIOT's interrupt handling,
  • and the biggest problem, it uses its own operating system with multiple tasks that has to be kept alive.

Therefore, I would always prefer the bare metal implementation. When esp-now netdev is used, the SDK operating system is used and it becomes quite tricky to get the system running.

If esp-now netdev would be the only option for networking, it would be reasonable to activate esp-now netdev when gnrc_netdev_default is used. But, RIOT port for ESP8266 also works with IEEE 802.15.4 modules. If you have such IEEE 802.15.14 or Ethernet netdev, it propably makes no sense to activate esp-now netdev as default. That's the reason why the dependency is not in Makefile.dev.

At the end of the day, the use of GNRC and the configuration of GNRC requires some care on the part of the application developer. That's why you can't use GNRC test/examples as they are for all platforms. The are configured for exavtly one netdev which is always available. It would be great to have a dynamic configuration. This problem is handled in #9903.

@miri64
Copy link
Member

miri64 commented Sep 27, 2018

That behavior seems to be caused by upper layers of GNRC or the application. esp-now netdev allows messages with a maximum length of 250 bytes and. Since it is of type NETDEV_TYPE_RAW, IPv6 packets are embedded without changes in esp-now messages. Since the IPv6 header has 40 byte and the UDP header has 8 byte, you can send 202 bytes at maximum as UDP payload. The maximum packet size is reported correctly by the esp-now netdev with NETOPT_MAX_PACKET_SIZE and seems to be is used by upper layers. Although you see the message Success: send 203 byte to ..., the _send function of the esp-now netdev is not called. Probably, the size is checked in the upper layer. Maybe, @miri64 can explain why you a success message instead of an an error message when overlength packet are tried to be sent. Maybe the return code is not handled in tests/gnrc_udp/main.c.

Due to the asynchronous nature of GNRC there is no error reporting (basically sending is just fire-and-forget). There is an experimental gnrc_neterr module that helps reporting errors, but don't think there is any application supporting it yet.

@gschorcht
Copy link
Contributor Author

@A-Paul How do we proceed, the test was successful and the observed behavior isn't caused by the driver. Would you give your OK to get it built with murdock?

@jia200x jia200x added this to the Release 2018.10 milestone Oct 10, 2018
@jia200x jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2018
@A-Paul A-Paul added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 12, 2018
@A-Paul
Copy link
Member

A-Paul commented Oct 12, 2018

Hi @gschorcht,
Because I have a bunch of nodemcu clones lying around, I wanted to provide a quick "hands on" report in the first place. At this moment I don't have the time to look deeper into your code and hence don't have enough insight to approve the PR alone.

I might repeat the quick test based on your last changes during this weekend.

@smlng, i seems you had a look into the code already. What is your affirmation level? :)

@A-Paul
Copy link
Member

A-Paul commented Oct 16, 2018

Repeated tests as in #9917 (review)
Connected three esp8266-esp-12x. ping6, udp server/send or nib neigh show look plausible.

@jia200x
Copy link
Member

jia200x commented Oct 23, 2018

any news here? Feature Freeze is tomorrow

@PeterKietzmann
Copy link
Member

So far this PR did not get a proper code review and neither in-depth testing. I propose we skip this feature for the release but try to get it merged somewhat soon so we have a stable version in the next release. I suppose many people are interested in networking with ESPs so let's not take the risk of merging prematurely.

@gschorcht
Copy link
Contributor Author

How do we proceed with this PR? All requests for changes were considered some time ago.

ESP-NOW for ESP8266 is compatible with ESP-NOW for ESP32 and is a prerequisite to use ESP32 and ESP8266 in the same network. ESP-NOW allows that ESP nodes to communicate without having infrastructure mode WiFi. And finally, this PR is prerequisite for the cleanup in PR #10929 and would make #10980 obsolete.

@tcschmidt
Copy link
Member

@miri64 @smlng - comments addressed? How about practical testing?

@smlng
Copy link
Member

smlng commented Feb 18, 2019

will test today, need to find a 2nd esp8266 though.

@gschorcht
Copy link
Contributor Author

@smlng Thanks 😄 BTW, testing should also be possible with one ESP8266 and one ESP32 node.

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

otherwise tested ACK!

cpu/esp8266/include/common.h Show resolved Hide resolved
@smlng smlng added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 18, 2019
@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Feb 18, 2019
@smlng
Copy link
Member

smlng commented Feb 18, 2019

@miri64 good to get this merged?

@miri64 miri64 dismissed their stale review February 18, 2019 12:46

My comments are addressed.

@miri64
Copy link
Member

miri64 commented Feb 26, 2019

@gschorcht I think you can squash ;-)

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 26, 2019
The documentation had to be changed due to the relation of module `esp_now` to `esp_wifi` module. In addition, a number of corrections have been made. In the case of documentation, it is impossible to do this in various commits.
Calling the initialization function ensures that the dummy lwIP library is used instead of the real lwIP, even if the esp_wifi module for esp8266 is not used.
@gschorcht
Copy link
Contributor Author

squashed

@gschorcht
Copy link
Contributor Author

@smlng Is it ok to merge?

@smlng smlng merged commit b98fdac into RIOT-OS:master Mar 6, 2019
@gschorcht
Copy link
Contributor Author

Thanks.

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 Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants