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_common: fixes common CPU configurations #13516

Merged
merged 5 commits into from
Mar 3, 2020

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Feb 29, 2020

Contribution description

This PR fixes the handling of cpu_conf.h in cpu/esp_common. Furthermore, it removes the platform specific heap_stats function in cpu/esp_common if module esp_idf_heap is not used.

ESP CPUs share common definitions in cpu/esp_common. There is also a cpu_conf.h file in cpu/esp_common which should contain common CPU configurations. However, this file was not included in the CPU specific cpu_conf.h in cpu/esp32 and cpu/esp8266 so that the definitions in this file had no affect. Furthermore, to include this file in CPU specific cpu_conf.h, it has to be renamed.

Testing procedure

Compilation should succeed in Murdock.

Issues/PRs references

Prerequisite for PR #13517

To be able to define common configurations for all ESP CPUs, the CPU specific configuration cpu_conf.h has to include a common configuration. For that purpose cpu_conf.h in cpu/esp_common is renamed to cpu_conf_common.h and included in CPU specific configurations.
@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 29, 2020
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.

Tested on esp8266-esp-12x

@benpicco
Copy link
Contributor

Hm, I ran tests/pkg_littlefs2 on both esp32-wroom-32 and esp8266-esp-12x but didn't see the crashes like on CI.

@gschorcht
Copy link
Contributor Author

Hm, I ran tests/pkg_littlefs2 on both esp32-wroom-32 and esp8266-esp-12x but didn't see the crashes like on CI.

Yes, this happens sporadically. The test synchronization seems a little unstable. It's the same as with nrf52dk. I bet it won't happen next time.

@gschorcht gschorcht 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 Feb 29, 2020
@benpicco
Copy link
Contributor

benpicco commented Feb 29, 2020

Yes, this happens sporadically. The test synchronization seems a little unstable.

Yea, but it prints a

EXCEPTION!! exccause=0 (IllegalInstructionCause) @8008783c excvaddr=00000000

.heap: 183848 (used 848, free 183000) [bytes]

register set
pc      : 400d96c5	ps      : 00060130	exccause: 00000000	excvaddr: 00000000
epc1    : 400d96c5	epc2    : 00000000	epc3    : 00000000	epc4    : 00000000
epc5    : 00000000	epc6    : 00000000	epc7    : 00000000	
a0      : 8008783c	a1      : 3ffb2e00	a2      : 3ffb30c4	a3      : 00000000
a4      : 00000000	a5      : 0000002b	a6      : 00000002	a7      : 3ffb2f00
a8      : 80082907	a9      : 3ffb2de0	a10     : 00000000	a11     : ffffffff
a12     : 00060120	a13     : 0000ff00	a14     : 00ff0000	a15     : ff000000
lbeg    : 40089f29	lend    : 40089f39	lcount  : fffffffc

Timeout in expect script at "child.expect(r'OK \(\d+ tests\)')" (tests/pkg_littlefs2/tests/01-run.py:14)

That's unusual.

@gschorcht
Copy link
Contributor Author

Yea but this time it prints a

EXCEPTION!! exccause=0 (IllegalInstructionCause) @8008783c excvaddr=00000000

Wow, it's a crash. But, I don't see how the changes in this PR should affect the littlefs tests. Furthermore, they work locally for me. The last nightly build had also no problems, but these tests were executed on another node. Maybe node pi-6356c652 has a hardware problem with the flash.

@gschorcht gschorcht 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 Feb 29, 2020
@gschorcht
Copy link
Contributor Author

gschorcht commented Feb 29, 2020

Hm, this time the tests failed again but on node pi-e4c1d860. This is really weird. Now, I'm trying to compile and test with master.

@gschorcht gschorcht 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 Mar 1, 2020
@gschorcht
Copy link
Contributor Author

@benpicco Obviously it was again a problem of a write access of the littlefs(2) file system to the flash memory. As explained in comment #12955 (comment), the cache of the flash memory is disabled during write accesses and the program code in the IROM is not available. Therefore, only program code from the IRAM can be executed. That it sometimes works and sometimes not is probably due to different delays during writing.

Although commit 5b4389c already moved the functions of the littlefs(2) filesystems to the IRAM, some of these functions in turn called system functions that were still in the IROM. Moving these functions to the IRAM seems to solve the problem.

The test runs are working now. The remaining failures are unrelated.

@gschorcht
Copy link
Contributor Author

@kaspar030 tests/nordic_softdevice fails permanently on nrf52dk. Is anything known?

@benpicco benpicco 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 and removed 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 Mar 3, 2020
@benpicco
Copy link
Contributor

benpicco commented Mar 3, 2020

Those tests have been failing quite reliably in the past…
CI was fine with the changes on esp, no need to bother Murdock again.

@benpicco benpicco added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs 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 Mar 3, 2020
@benpicco benpicco merged commit 8e8cfbf into RIOT-OS:master Mar 3, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 6, 2020
@leandrolanzieri leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Mar 6, 2020
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants