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

tests/cpu_esp: tests for ESP specific modules #12971

Closed
wants to merge 7 commits into from

Conversation

gschorcht
Copy link
Contributor

Contribution description

A number of optional features of ESP boards have to be explicitly activated by enabling the corresponding modules. However, because these modules are not enabled by default, the CI compilation test cannot detect potential compilation problems. As a result, PRs can be merged that cause compilation problems when an optional ESP feature is enabled through the corresponding module.

To make it possible for CI compilation tests to recognize potential compilation problems when optional ESP features are enabled, a number of specific tests are provided by this PR. Some of them are only for compilation tests, but others can also be used to test the optional feature.

  • cpu_esp_eth:
    Compile test for module esp_eth using a shell-based GNRC networking application for ESP32 boards that provides feature esp_eth.
  • cpu_esp_hw_counter:
    Uses tests/periph_timer with hardware counters on ESP32 boards (module esp_hw_counter).
  • cpu_esp_i2c_hw:
    Uses tests/periph_i2c with hardware I2C on ESP32 boards (module esp_i2c_hw).
  • cpu_esp_heap:
    Uses tests/malloc with SDK heap on ESP8266/ESP32 boards (module esp_idf_heap) and the SPI RAM (module esp_spi_ram) for ESP32 boards that provide the feature esp_spi_ram.
  • cpu_esp_log:
    Uses tests/shell with enabled startup information (module esp_log_startup) and tagged output format (module esp_log_tagged).
  • cpu_esp_wifi:
    Compile test for module esp_wifi using a shell-based GNRC networking application .

Testing procedure

Compilation and automatic tests should succeed on Murdock.

Issues/PRs references

Tracked in issue #12960

@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 17, 2019
@gschorcht
Copy link
Contributor Author

@kaspar030 @benpicco What do you think about such tests? That would allow Murdock to detect compilation or even runtime problems for optional ESP features. I know, it is the only CPU that defines so many specific tests 😎 But these tests are necessary to cover all optional features.

@gschorcht gschorcht removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Dec 17, 2019
@gschorcht gschorcht force-pushed the cpu/esp/tests branch 2 times, most recently from 140c359 to 26fecc9 Compare December 17, 2019 08:17
@gschorcht gschorcht added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Dec 17, 2019
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Sorry, forgot about this.
But yea we should definitely have a compile-test for each driver.

I'm not sure about copying entire tests just to try out another config option, we might get away with less code duplication by just having a (test) board that has these options enabled by default.

Comment on lines +5 to +8
The application corresponds exactly to the application `tests/periph_i2c`.
It is only for testing the timer implementation using hardware counters.
In fact, the application is nothing more than the `tests/periph_i2c`
application compiled with the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

What timer implementation gets used on the esp for tests/periph_i2c?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hardware implementation esp_i2c_hw doesn't require a timer. The bit-banging software implementation uses busy waiting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you default to bit-banging when there is a hardware i2c?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is buggy, instable and slower than the software implementation.

Comment on lines +5 to +8
The application corresponds exactly to the application `tests/periph_timer`.
It is only for testing the timer implementation using hardware counters.
In fact, the application is nothing more than the `tests/periph_timer`
application compiled with the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of copying every test to have a version with hardware counters, why not add a test-esp board that has

USEMODULE += esp_hw_counter

What is used by default instead? When would the user chose to use hardware counters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESP32 has two counters and four 64-bit timers that can be used as periph_timer. By default, the 64-bit timers are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a single test application - just use the existing tests tests/periph_i2c, tests/periph_timer but have a board that uses the other timer / i2c implementation by default.
At least that's how I understand the problem.

But I wouldn't like to expose this test board in the boards directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe adding an external board that lives inside the test works.
I'm not sure what the latest state of compatibility with external boards and in-tree boards was, but if that works it would probably be the cleanest solution.

@benpicco
Copy link
Contributor

Also please rebase.

Copy link
Contributor Author

@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.

I'm not sure about copying entire tests just to try out another config option, we might get away with less code duplication by just having a (test) board that has these options enabled by default.

You mean we should have a single test application that contains a test board definition with all required functions and all modules enabled? Tricky, but really a good idea. I really like hacks like this 🕶️

@benpicco
Copy link
Contributor

Not a single test application - just use the existing tests tests/periph_i2c, tests/periph_timer but have a board that uses the other timer / i2c implementation by default.
At least that's how I understand the problem.

@benpicco
Copy link
Contributor

benpicco commented Aug 7, 2020

Please rebase

The test is just a copy of tests/periph_timer but uses the hardware counter for timers (module esp_hw_counter).
The test is just a copy of tests/periph_i2c but uses the hardware module for I2C (module esp_i2c_hw).
The test is just a copy of tests/malloc but uses the heap of the SDK (module esp_idf_heap) instead of the heap provided by the newlib. If feature esp_spi_ram is provided, the module esp_esp_ram is enabled. Thus the test is good for testing modules esp_idf_heap and esp_spi_ram.
The application corresponds exactly to the application `tests/shell`.
It is only for testing the ESP-specific logging functions enabled by the
`esp_log_startup` and `esp_log_tagged` modules.
@gschorcht
Copy link
Contributor Author

Rebased, but I just saw that the discussion about a virtual test board got stuck when the online semester came. I'm sorry, the PR seems to be not finished yet. esp_wifi_enterprise is also not covered yet.

@fjmolinas
Copy link
Contributor

Rebased, but I just saw that the discussion about a virtual test board got stuck when the online semester came. I'm sorry, the PR seems to be not finished yet. esp_wifi_enterprise is also not covered yet.

I don't think we should block this because esp_wifi_enterprise is not covered. Regarding the virtual board, you could do the same as tests/external_boards_dir, I can help with that if you need help :).

@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 19, 2021
@gschorcht gschorcht added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: stale State: The issue / PR has no activity for >185 days labels Mar 20, 2021
@gschorcht
Copy link
Contributor Author

Still in progress.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@benpicco benpicco 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 Dec 1, 2021
@benpicco benpicco requested a review from maribu December 1, 2021 13:22
@benpicco
Copy link
Contributor

benpicco commented Dec 1, 2021

Since there seems to be no good other way right now, let's get this in as is.
Please rebase to update the CI configuration.

@gschorcht gschorcht removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Dec 1, 2021
@gschorcht
Copy link
Contributor Author

I have removed the CI: ready for build label because I think I can further reduce the number of these applications a bit. If this is the only way for now, I will rework the PR a bit.

@gschorcht
Copy link
Contributor Author

Closed in favor of #17314.

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: tests Area: tests and testing framework Platform: ESP Platform: This PR/issue effects ESP-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants