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: migrate to ztimer #17386

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

fjmolinas
Copy link
Contributor

Contribution description

esp has a couple of uses of xtimer that can't be directly translated to ztimer, so it seems better to do it in its own PR. I also don't have hardware to tests so would appreciate if maybe @gschorcht could complement murdock's testing.

The most delicate change is https://github.com/fjmolinas/RIOT/blob/6a736e4cdfb57ee446050d8fd0c455f120dd381c/cpu/esp32/esp_ztimer.c#L153-L157 since this is not a direct translation, so depending on how sensitive this value is some adjustment might be needed, thougts @gschorcht?

Testing procedure

  • All esp tests should pass
  • Murdock tests should pass

Issues/PRs references

depends on #17385
part of #16903

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: sys Area: System Area: timers Area: timer subsystems Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Dec 13, 2021
@fjmolinas fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2021
@fjmolinas
Copy link
Contributor Author

I'll run tests to see if somethin breaks....

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved
cpu/samd5x/cpu.c Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Dec 13, 2021
@kaspar030
Copy link
Contributor

needs rebase

@gschorcht
Copy link
Contributor

@fjmolinas I will take a look later today.

cpu/esp32/Makefile.dep Show resolved Hide resolved
cpu/esp32/doc.txt Outdated Show resolved Hide resolved
@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 13, 2021
@gschorcht
Copy link
Contributor

Sorry, when I pushed you to change everything back to ztimer_usec, but I was a bit confused since you added it unconditionally here

USEMODULE += ztimer_usec
So I though that xtimer was enabled herd by default also before your changes and usec resolution is required. However, this wasn't the case.

The only place where it was enabled before and used usec resolution was here.

ifneq (,$(filter esp_freertos,$(USEMODULE)))

void vTaskDelay( const TickType_t xTicksToDelay )
{
DEBUG("%s xTicksToDelay=%d\n", __func__, xTicksToDelay);
#if defined(MODULE_ESP_WIFI_ANY)
uint64_t us = xTicksToDelay * MHZ / xPortGetTickRateHz();
xtimer_usleep(us);
#endif
}
But after looking into the code, I'm really sure that this could also be changed to msec. A tick in FreeRTOS is 10 or 20 ms.
uint32_t xPortGetTickRateHz(void) {
return MSEC_PER_SEC / portTICK_PERIOD_MS;
}
So we could change all timers to msec. Sorry for this inconvenience, I was just confused by the unconditional ztimer_usec added by this PR.

cpu/esp8266/doc.txt Outdated Show resolved Hide resolved
cpu/esp8266/periph/timer.c Outdated Show resolved Hide resolved
@gschorcht
Copy link
Contributor

Since you touch this file, please correct

DEBUG("wifi_scan_get_ap_num ret=%d num=%d\n", ret ,ap_num);

-     DEBUG("wifi_scan_get_ap_num ret=%d num=%d\n", ret ,ap_num);
+     DEBUG("wifi_scan_get_ap_num ret=%d num=%d\n", ret, ap_num);

@gschorcht
Copy link
Contributor

gschorcht commented Dec 14, 2021

@fjmolinas It seems that we have a dependencies problem in Kconfig that wasn't catched by CI compilation when #17232 was merged. Let me try to fix it first. You can observe it when you compare the modules list with commands:

make BOARD=esp32-olimex-evb -C examples/gnrc_networking info-modules
TEST_KCONFIG=1 make BOARD=esp32-olimex-evb -C examples/gnrc_networking info-modules

Without TEST_KCONFIG=1 the following modules are enabled because the board has esp_eth as netdev default.

esp_eth
esp_idf_eth
esp_idf_eth_phy

where esp_eth then enables xtimer automatically.

With TEST_KCONFIG=1 these modules including xtimer are not enabled.

@gschorcht
Copy link
Contributor

It seems that we have a dependencies problem in Kconfig

This is because esp_wifi*, esp_eth dependencies were not modeled at all til now.

cpu/esp_common/Makefile.dep Outdated Show resolved Hide resolved
cpu/esp_common/freertos/Kconfig Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Area: Kconfig Area: Kconfig integration label Dec 14, 2021
@fjmolinas
Copy link
Contributor Author

@gschorcht I conditionally included the ztimer file I hope you don't mind doing this instead, it's a bit more explicit regarding dependencies IMO.

@fjmolinas
Copy link
Contributor Author

@gschorcht I conditionally included the ztimer file I hope you don't mind doing this instead, it's a bit more explicit regarding dependencies IMO.

But I can also revert if you prefer, I also realize this is turning into one big blob commit with many chances, I can split them up when squashing if you prefer.

cpu/esp32/periph/timer.c Outdated Show resolved Hide resolved
cpu/esp8266/periph/timer.c Outdated Show resolved Hide resolved
cpu/esp32/periph/timer.c Outdated Show resolved Hide resolved
@gschorcht
Copy link
Contributor

You can squash your last changes directly to have a single commit.

Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Looks good, lets see what Murdock says.

@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2021
@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2021
@fjmolinas
Copy link
Contributor Author

Yeiii! All green!

@kaspar030 kaspar030 merged commit dad01c1 into RIOT-OS:master Dec 14, 2021
@kaspar030
Copy link
Contributor

Nice one!

@fjmolinas fjmolinas deleted the pr_esp_ztimer branch December 14, 2021 22:55
@fjmolinas
Copy link
Contributor Author

Thanks for the review and suggestions @gschorcht!

@gschorcht
Copy link
Contributor

gschorcht commented Dec 15, 2021

@fjmolinas Great work. Thanks.

@gschorcht
Copy link
Contributor

gschorcht commented Dec 18, 2021

@fjmolinas Unfortunatly, it seems that this PR has side effects on esp_wifi for ESP8266. When I tested some of my changes in esp_wifi today I had to figure out, that the WiFi interface with WPA2 authentication doesn't work anymore since this PR. When we changed the timer from us to msec, I tested it only for ESP32 😟 I will try to figure out, what the problem is.

@@ -242,7 +242,7 @@ SECTIONS
*freertos/*(.literal .text .literal.* .text.*)
*freertos_common/*(.literal .text .literal.* .text.*)
*periph/*(.literal .text .literal.* .text.*)
*xtimer/*(.literal .text .literal.* .text.*)
*ztimer/*(.literal .text .literal.* .text.*)
Copy link
Contributor

@gschorcht gschorcht Dec 18, 2021

Choose a reason for hiding this comment

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

This would have to be:

-     *ztimer/*(.literal .text .literal.* .text.*)
+     *ztimer_core/*(.literal .text .literal.* .text.*)

When I was reviewing it, I didn't know that objects are not stored in ztimer but in ztimer_core although the module is called ztimer. But this alone doesn't solve the problem, unfortunatly 😟

@gschorcht
Copy link
Contributor

@fjmolinas PR #17427 solves the problem with ztimer and ESP WiFi on ESP8266.

@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
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: doc Area: Documentation 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants