esp32: Add support for espnow based pktradio #15732

Laczen · 2025-01-31T16:34:36Z

Summary

Espnow is a connectionless WiFi communication protocol that can be used to exchange information between esp nodes.

Impact

A espnow pktradio driver is proposed that allows building a 6lowpan network of nodes.

Testing

The driver has been evaluated using udpclient & server running on different devices.

A sample configuration is found in esp32-devkitc:espnow. The node address can be changed under ESP32 Peripherals option Espnow. To test the communication using udpserver and udpclient two nodes need to be prepared using e.g. node address 0x0a and 0x0b.

On node 0x0a the server can be started using:

nsh> udpserver &

On node 0x0b the client can be started using:

nsh> udpclient fe80::ff:fe00:a

The client node will show that the messages are send, while the server shows that messages are received.

Solves #15347

@tmedicci fyi

nuttxpr · 2025-01-31T16:35:53Z

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it provides a high-level overview, it lacks crucial details.

Here's a breakdown of what's missing:

Summary:
- Missing: How the ESP-NOW driver works. What code was changed/added? What specific files are impacted? What's the overall architecture of the integration?
Impact:
- All impact sections are inadequately addressed. Simply stating "A driver is proposed" doesn't describe the impact. Consider:
  - New Feature? Yes, the ESP-NOW driver.
  - User Impact? Probably yes. How do users configure and use the new driver? Any new Kconfig options? New APIs?
  - Build Impact? Likely yes. What needs to be added to the configuration to enable this driver?
  - Hardware Impact? Definitely yes. This is specific to ESP chips with WiFi capabilities. Which ones are supported?
  - Documentation Impact? Absolutely yes. New features require documentation. Where is this documentation?
  - Security Impact? ESP-NOW has security implications. These need to be addressed.
  - Compatibility Impact? Potentially yes. Does this impact other network drivers or configurations?
Testing:
- Insufficient testing information. "Evaluated using udpclient & server" is vague. Provide specific commands, configurations, and expected output. The logs before and after the change are missing. What platform was this tested on (specific board and configuration)? What compiler was used?

In short, the PR description needs significantly more detail to be considered complete. It needs to clearly explain the technical implementation, its impact on the system, and provide concrete evidence of successful testing.

fdcavalcanti · 2025-01-31T17:19:12Z

Hi @Laczen that's a great contribution!
Please improve the PR description.
I'll take a look and run some tests on it.

fdcavalcanti · 2025-01-31T17:23:25Z

Can you add a defconfig ready to use with this functionality? You can call it espnow.

Also some documentation would be good. You can a brief description of this to Documentation/platforms/xtensa/esp32 following other functionalities as example.

tmedicci · 2025-01-31T17:29:01Z

Amazing work, @Laczen!

Congratulations.

Apart from the defconfig, can you improve the testing method with clear instructions on how to build and test it (considering that others may try to test and review this PR)?

fdcavalcanti · 2025-02-03T10:40:21Z

Please do not merge yet, still running tests.

fdcavalcanti · 2025-02-03T13:38:18Z

Please fix KConfig on this point, seems to be missing the node ID description. There's only a '[0]'

  │ │  [*] ESPNOW packet radio support                                                                                                                                │ │  
  │ │  (0x0) panid                                                                                                                                                    │ │  
  │ │     *** Node address for espnow packet radio ***                                                                                                             │ │  
  │ │     (0x0b) [0]                                                                                                                                                     │ │

tmedicci

Just minor suggestions. In general, I liked the solutions adopted to make ESP-NOW works on NuttX. Congrats!

Documentation/platforms/xtensa/esp32/boards/esp32-devkitc/index.rst

arch/xtensa/src/esp32/Kconfig

arch/xtensa/src/esp32/esp32_espnow_pktradio.c

tmedicci · 2025-02-03T17:47:04Z

arch/xtensa/src/esp32/esp32_espnow_pktradio.c

+  iob = iob_tryalloc(false);
+
+  /* Ignore the packet if allocation fails or packet too large */
+
+  if ((iob == NULL) || (data_len > CONFIG_IOB_BUFSIZE))
+    {
+      return;
+    }


Instead of failing if data_len > CONFIG_IOB_BUFSIZE, should we try the same approach of the arch/risc-v/src/common/espressif/esp_wlan.c (check how many IOB buffers we have, allocate one iob with iob_tryalloc and calling iob_trycopyin)?

I think this would make it to complex. As we now we are using 6lowpan the messages are limited in size, just use one iob per message for simplicity. I can add a check that IOB_BUFSIZE is sufficiently large.

Yes, it would make it more complex. Alternatively, set the CONFIG_IOB_BUFSIZE to 250 in the defconfig file (max payload size according to https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/network/esp_now.html#frame-format)

Wouldn't 250 be too large given that 6lowpan frames are smaller than 127 octets (I could be making an error since 6lowpan, espnow and NuttX are all new for me) ?

Currently, CONFIG_IOB_BUFSIZE=196. Did you see any packet being dropped due to small buffer during your tests?

About the frame size, I understand that IEEE 802.15.4's standard frame size is 127 octets, but we are using a non-standardized radio (ESP-NOW), so we don't have such limitation (but I couldn't find any code related to that on NuttX too, even for 802.15.4).

I didn't see any drops, but then again the udpclient is only sending packets of 96 octets.

From the sixlowpan_send() method's documentation I was thinking that packets are fragmented automatically, but I'm doubting this now.

It seems it does. Check sixlowpan_queue_frames. This same function calls sixlowpan_radio_framelen, which returns properties.sp_framelen. This is the radio frame length.

After checking it, it may be "adjusted" to the max size of a IOB (here). So, at least one change is required:

properties->sp_framelen = CONFIG_IOB_BUFSIZE; /* Fixed frame length */ must be set to the ESP-NOW frame length (250 bytes). CONFIG_IOB_BUFSIZE being set as 250 bytes would enable using the ESP-NOW more efficiently (but isn't mandatory).

Since I would like to keep the receive limited to one iob and make sure that different esp's are using the same framelength I would propose to introduce a CONFIG_ESP32_ESPNOW_FRAMELEN that should be defined equal for different nodes and to use this as the properties->sp_framelen. A compilation check will be added to verify that CONFIG_IOB_BUFSIZE is at least CONFIG_ESP32_ESPNOW_FRAMELEN.

Does this seem OK to you ?

Of course. It's a clever solution. Please go ahead!

arch/xtensa/src/esp32/Kconfig

arch/xtensa/src/esp32/esp32_espnow_pktradio.c

tmedicci · 2025-02-04T13:16:02Z

arch/xtensa/src/esp32/esp32_espnow_pktradio.c

+  dev->d_flags = IFF_UP;
+  return espnow_ifup(&priv->radio.r_dev);


(Ow, I thought I had commented about it, but I couldn't see my review. Repeating, then...)

This isn't usual. Usually, we don't bring up the network interface during initialization. There is a service for doing that (CONFIG_NETUTILS_NETINIT=y), although we may have problems with it.

Specifically for this implementation, it's recommended the Wi-Fi be initialized before bringing up ESP-NOW (according to here). On the other hand, netutil's netinit_net_bringup can bring up only a single network defined by NET_DEVNAME. The problem here is that Wi-Fi network is evaluated before other possible networks and that's why Wi-Fi is bring up (and other networks don't).

So, here, I'd recommend either 1) changing netutil's behavior (instead of bringing up only a single interface, create a list of available interfaces and bring up them all) or 2) (easier) just remove it and call ifup on NuttShell directly to bring up the ESP-NOW interface.

No problem, I will change the setup (reminder to self: update the documentation !).

I would also like to be able to setup the espnow pktradio without the wifi netinterface but this is for later.

Corrected, espnow_ifup() is no longer called.

No problem, I will change the setup (reminder to self: update the documentation !).

I would also like to be able to setup the espnow pktradio without the wifi netinterface but this is for later.

Hi @Laczen . Is it possible to change the node address in runtime? That would be an interesting feature (not for now, different PR).

Hi @fdcavalcanti IMO everything is available to change the node address in runtime (trough ioctl call). I need to make some small changes to make sure that they are applied correctly in the espnow_ifup() call. I will make a separate PR for this once this PR is merged.

jerpelea · 2025-02-04T15:00:52Z

please split arch and documentation in separate PR
if we try to maintain LTS releases this is a requirement

Laczen · 2025-02-04T16:04:32Z

please split arch and documentation in separate PR if we try to maintain LTS releases this is a requirement

Hi @jerpelea could you please help me how to do this correct taking into account the LTS release requirement. This PR introduces a new 6lowpan network interface (in arch) together with the documentation and a defconfig. When they are in one PR it is possible for reviewers to test it, when it is in separate PR's this is not so easy. Please help me identify exactly how many PR's are required and if there needs to be coupling (depends on ...) between the PR's.

On another os I have been working on the api changes/additions where added to a "release" file to keep track of what was added in (or sometimes broken by) a specific release, but I don't know how NuttX does this.

tmedicci · 2025-02-04T17:13:51Z

please split arch and documentation in separate PR if we try to maintain LTS releases this is a requirement

In this case, @jerpelea , It makes sense to keep them in the same PR (two separate commits) because the documentation is coupled with the current implementation, so backporting it would also be associated. Does it make sense?

Espnow is a connectionless WiFi communication protocol that can be used to exchange information between esp nodes. A espnow pktradio driver is proposed that allows building a 6lowpan network of nodes. The driver has been evaluated using udpclient & server running on different devices. Solves apache#15347 Signed-off-by: Laczen JMS <[email protected]>

Added documentation for espnow and a default defconfig to illustrate the use of espnow for udp communication. Signed-off-by: Laczen JMS <[email protected]>

tmedicci

For me it's fine. Can you take a look @fdcavalcanti too?

fdcavalcanti · 2025-02-05T11:31:51Z

Looks good to me.

Laczen · 2025-02-06T11:37:09Z

please split arch and documentation in separate PR if we try to maintain LTS releases this is a requirement

In this case, @jerpelea , It makes sense to keep them in the same PR (two separate commits) because the documentation is coupled with the current implementation, so backporting it would also be associated. Does it make sense?

@jerpelea You have made this same requirement in the dev mailinglist.

However it is not clear to me why this would be a requirement for LTS releases. I propose before making this requirement for PR's a github issue is created regarding LTS releases, what they are, how they will be used (e.g. only backport bugs) and what to expect from LTS releases (e.g. any defconfig created out of tree should remain valid, disallowing changes to Kconfig options). Once this is clear it should be possible to decide how PR's should align with these requirements.

tmedicci · 2025-02-06T13:13:32Z

I think this PR can be merged as-is, right @jerpelea ?

4 reviewers, no request for changes...

github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Board: xtensa Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Jan 31, 2025

acassis requested a review from tmedicci January 31, 2025 22:31

github-actions bot added the Area: Documentation Improvements or additions to documentation label Feb 1, 2025

Laczen force-pushed the espnow branch 2 times, most recently from 8a7e954 to 555f6e2 Compare February 2, 2025 08:43

acassis approved these changes Feb 2, 2025

View reviewed changes

jerpelea approved these changes Feb 3, 2025

View reviewed changes

tmedicci reviewed Feb 3, 2025

View reviewed changes

Laczen force-pushed the espnow branch 2 times, most recently from 5cb003e to d2958f7 Compare February 4, 2025 09:36

tmedicci reviewed Feb 4, 2025

View reviewed changes

Laczen added 2 commits February 4, 2025 20:07

esp32: add espnow documentation and defconfig.

c0a0adc

Added documentation for espnow and a default defconfig to illustrate the use of espnow for udp communication. Signed-off-by: Laczen JMS <[email protected]>

Laczen force-pushed the espnow branch from d2958f7 to c0a0adc Compare February 4, 2025 19:13

tmedicci approved these changes Feb 4, 2025

View reviewed changes

tmedicci requested a review from xiaoxiang781216 February 5, 2025 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp32: Add support for espnow based pktradio #15732

esp32: Add support for espnow based pktradio #15732

Laczen commented Jan 31, 2025 •

edited

Loading

nuttxpr commented Jan 31, 2025

fdcavalcanti commented Jan 31, 2025

fdcavalcanti commented Jan 31, 2025

tmedicci commented Jan 31, 2025

fdcavalcanti commented Feb 3, 2025

fdcavalcanti commented Feb 3, 2025 •

edited

Loading

tmedicci left a comment

tmedicci Feb 3, 2025

Laczen Feb 4, 2025

tmedicci Feb 4, 2025

Laczen Feb 4, 2025

tmedicci Feb 4, 2025

Laczen Feb 4, 2025

tmedicci Feb 4, 2025

Laczen Feb 4, 2025

tmedicci Feb 4, 2025

Laczen Feb 4, 2025

tmedicci Feb 4, 2025

Laczen Feb 4, 2025

Laczen Feb 4, 2025

fdcavalcanti Feb 5, 2025 •

edited

Loading

Laczen Feb 6, 2025

jerpelea commented Feb 4, 2025

Laczen commented Feb 4, 2025

tmedicci commented Feb 4, 2025

tmedicci left a comment

fdcavalcanti commented Feb 5, 2025

Laczen commented Feb 6, 2025

tmedicci commented Feb 6, 2025

		dev->d_flags = IFF_UP;
		return espnow_ifup(&priv->radio.r_dev);

esp32: Add support for espnow based pktradio #15732

Are you sure you want to change the base?

esp32: Add support for espnow based pktradio #15732

Conversation

Laczen commented Jan 31, 2025 • edited Loading

Summary

Impact

Testing

nuttxpr commented Jan 31, 2025

fdcavalcanti commented Jan 31, 2025

fdcavalcanti commented Jan 31, 2025

tmedicci commented Jan 31, 2025

fdcavalcanti commented Feb 3, 2025

fdcavalcanti commented Feb 3, 2025 • edited Loading

tmedicci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdcavalcanti Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerpelea commented Feb 4, 2025

Laczen commented Feb 4, 2025

tmedicci commented Feb 4, 2025

tmedicci left a comment

Choose a reason for hiding this comment

fdcavalcanti commented Feb 5, 2025

Laczen commented Feb 6, 2025

tmedicci commented Feb 6, 2025

Laczen commented Jan 31, 2025 •

edited

Loading

fdcavalcanti commented Feb 3, 2025 •

edited

Loading

fdcavalcanti Feb 5, 2025 •

edited

Loading