cpu/esp32: add IEEE 802.15.4 support for ESP32-H2 #21590
Conversation
There was a problem hiding this comment.
Mostly (as usual) some styling related comments. I haven't tested it, as I don't have the hardware for it.
I'm not familiar how this is integrated into the rest of RIOT's network code and how a user would use it, but I wonder if it wouldn't be good to add some more documentation about the inner workings of the code. Either in Doxygen format or in the long cpu/esp32/esp-ieee802154/esp_ieee802152_hal.c (yes, it's mostly wrapper functions, but only mostly).
| * @return 0 on success | ||
| * @return negative errno on error |
There was a problem hiding this comment.
| * @return 0 on success | |
| * @return negative errno on error | |
| * @retval 0 on success | |
| * @retval negative errno on error |
It might be nice to write which errno it would return. However in the current implementation, it will always return 0, even on failure.
Is it planned to change that behavior? Perhaps it would make sense to adapt the documentation otherwise.
There was a problem hiding this comment.
Is it planned to change that behavior?
I'm not sure. The function is called by auto_init_esp_ieee80215, which has no return value. So whatever esp_ieee802154_init returns is not used anyway.
We could embed each API function call in a series of if () else constructs and return a more meaningful return value, but does that make sense if no one evaluates it?
There was a problem hiding this comment.
Difficult indeed. Will esp_ieee802154_init only ever be called by auto_init_esp_ieee80215 or could it be expected that it'll be called by a custom program as well?
In the former case I'd say we could omit the return value, in the latter case it would be nice to handle the return.
In both cases, the documentation should reflect the actual behavior.
There was a problem hiding this comment.
🤔 Theoretically, if someone doesn't use auto_init it could be that esp_ieee802154_init is called by the application.
On the othe hand, it is implemented exactly in the same way as all other IEEE 802.15.4 drivers, e.g., mrjf24j40, kw2xrf_radio and nrf802154. All of them document
* @return 0 on success
* @return <0 on error
but simply return 0.
1e1b744 to
0af92ee
Compare
| #define _USE_SET_RX 1 | ||
| #define _USE_SET_IDLE 1 |
There was a problem hiding this comment.
Is there a case where those would not be set to 1?
There was a problem hiding this comment.
Good question, I used them to be able to enable/disable the corresponding code quickly. I would prefer to leave them there for future testing.
| return -EINVAL; | ||
| } | ||
|
|
||
| void esp_ieee802154_cca_done(bool channel_busy) |
There was a problem hiding this comment.
Those appear unused - are they called from within the IDF?
There was a problem hiding this comment.
Exactly, these are callbacks.
There was a problem hiding this comment.
Better add a comment that the section is for IDF callbacks
|
Fine with me! |
fixup! tests/net/ieee802154_submac: add ESP32x IEEE 802.15.4 suppport
96492a2 to
705a240
Compare
|
Although the full build in Murdock was successful, the PR was removed from merge queue due to no response for status checks. The same with PR #21604. |
Contribution description
This PR provides the support for the IEEE 802.15.4 interface for the ESP32-H2.
Testing procedure
Flash the test application
examples/networking/gnrc/gnrc_networkingwhich uses the IEEE 802.15.4 interface as
netdev_default. It should be possible to ping other IEEE 802.15.4 nodes.IEEE 802.15.4 for ESP32-H2 has been tested with an
esp32h2-devkitboard together with awaveshare-nrf52840-eval-kitboard:Issues/PRs references