Repository navigation
Conversation
Murdock results❌ FAILED 710d72e tests/net/ieee802154_hal: add timed tx test
Build failures (1)
Artifacts |
| * @brief Get the current radio time | ||
| * | ||
| * The radio time is a free running counter in microseconds that wraps | ||
| * around at 2^32. It is only valid while the device is on. Time values |
There was a problem hiding this comment.
| * around at 2^32. It is only valid while the device is on. Time values | |
| * around at 2^32. It is only valid while the device is on. |
| * @pre The device is on | ||
| * @pre The device supports @ref IEEE802154_CAP_TIMED_TX |
There was a problem hiding this comment.
| * @pre The device is on | |
| * @pre The device supports @ref IEEE802154_CAP_TIMED_TX | |
| * @pre The device is on. | |
| * @pre The device supports @ref IEEE802154_CAP_TIMED_TX. |
There was a problem hiding this comment.
I know, the period at the end of this short sentence is also missing in other functions, but this doesn't make it right 😉
| #include "esp_timer.h" | ||
| #include "iolist.h" | ||
| #include "log.h" | ||
| #include "net/ieee802154/radio.h" |
There was a problem hiding this comment.
| #include "esp_timer.h" | |
| #include "iolist.h" | |
| #include "log.h" | |
| #include "net/ieee802154/radio.h" | |
| #include "iolist.h" | |
| #include "log.h" | |
| #include "net/ieee802154/radio.h" |
| static uint32_t _get_time(ieee802154_dev_t *dev) | ||
| { | ||
| (void)dev; | ||
| return (uint32_t)esp_timer_get_time(); |
There was a problem hiding this comment.
| return (uint32_t)esp_timer_get_time(); | |
| return ztimer::get_now(ZTIMER_MSEC); |
From the very beginning, the goal was to keep the RIOT code as independent as possible from ESP-IDF in order to limit the amount of source code that needed to be compiled in from ESP-IDF. The dependencies between the ESP-IDF modules are very complex and more and more ESP-IDF source code must be compiled in very quickly, which makes the manual maintenance of the Makefiles in cpu/esp32/esp-idf very hard. For this reason, various basic functions have been implemented in RIOT, including a system time in milliseconds that can be retrieved using system_get_time.
Since we already use ztimer_msec modul, the RIOT function should be used.
There was a problem hiding this comment.
I've given it some more thought. If the intention was to use a timer that is independent of RIOT's ztimer, you should use system_get_time to be independent of ESP-IDF, even though the ESP-IDF IEEE 802.15.4 driver uses esp_timer_get_time internally.
| return (uint32_t)esp_timer_get_time(); | |
| return (uint32_t)system_get_time(); |
There was a problem hiding this comment.
That's some useful insight I didn't have. The idea behind it was if the radio has a dedicated only for the radio it should be used instead of ztimer if this not the case for example at86rf233 don't have such a timer the functionality would be offloaded to the submac instead. Going through the esp code I notice that the transmit_at function uses esp_timer_get_time() and thought it would use the dedicated timer then. But didn't seems like it would
There was a problem hiding this comment.
Okay, I've thought about it some more 🤔 As you suggested 👍, we should use esp_timer_get_time instead of system_get_time here. So please ignore my change request.
The reason for this is that system_get_time uses a different timer than esp_timer_get_time. While system_get_time uses a real timer (TIMER_0 from TIMER_GROUP_0) for the system time, esp_timer_get_time uses a special system timer peripheral. On the ESP32, an additional counter (the LACT) from TIMER_GROUP_0 is used; all other ESP32x SoC variants have a dedicated SYSTIMER peripheral that is used for the system time.
Since the implementation of esp_ieee80215_transmit_at uses the ESP-IDF system time as its time base, we should also use this for our interface. The time returned by esp_time_get_time is likely to differ slightly from the time returned by system_get_time.
Since the ESP-IDF esp_timer is compiled in anyway in cpu/esp32/esp-idf/common, using the esp_timer_get_time function here doesn't increase the dependency on ESP-IDF.
Sorry for the confusion.
There was a problem hiding this comment.
This discussion inspires me to clean up the implementation of the system time in RIOT for the ESP32. Since we’re including the ESP-IDF module esp_timer anyway, we could implement the function system_get_time based on esp_timer_get_time.
The reason system_get_time hasn’t relied on esp_time_get_time until now is that, in earlier versions of ESP-IDF, it wasn’t necessary to include the esp_timer module. This requirement was added over time, but system_get_time wasn’t updated accordingly.
Contribution description
This PR introduces timed tranmission for the radio HAL. Timed transmission is useful in various MAC-Layers and Protocols like TSCH(openwsn), DSME(opendsme) or Thread(openthread). Right now this functionality is unused but feel free to pick this up.
Could be also used in #22539. Also this can be used for periodically send 15.4 frames like in the hal-test
Testing procedure
Run the HAL test for the support devices
tests/net/ieee802154_haland execute thetxtsnd_aftercommand:NRF52840DK
ESP32C6-devkit
FRDM-KW41Z
Issues/PRs references
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: