Skip to content

cpu/esp32: support of multiple heaps for newlib malloc#13517

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/multiheap
Jul 10, 2020
Merged

cpu/esp32: support of multiple heaps for newlib malloc#13517
benpicco merged 1 commit intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/multiheap

Conversation

@gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Feb 29, 2020

Contribution description

This PR adds support of multiple heaps if module esp_idf_heap is not used.

For that purpose the following unused DRAM sections are added as additional heap segments:

0x3ffae6e0 ... 0x3ffaffff   heap1   6 kiB
0x3ffe0440 ... 0x3ffe3fff   heap2   14 kiB
0x3ffe4350 ... 0x3fffffff   heap3   111 kiB

With these additional heap segments, the overall heap size increases from 180 kiB to 315 kiB for the tests/heap_cmd application.

The standard heap is located in:

0x3ffb0000 ... 0x3ffdffff

It's size depends on other data since .data and .bss sections are also placed there. Therefore, the heap starts at _bss_end.

Testing procedure

Use the following command to flash an ESP32 board:

CFLAGS='-DNUMBER_OF_TESTS=1 -DCHUNK_SIZE=1000' BOARD=esp32-wroom-32 \
make -C tests/malloc flash

Check that all heap segments are used:

CHUNK_SIZE: 1000
NUMBER_OF_TESTS: 1
Allocated 1000 Bytes at 0x0x3ffb2900, total 1000
...
Allocated 1000 Bytes at 0x0x3ffdf900, total 182440
Allocated 1000 Bytes at 0x0x3ffae6e8, total 183448
...
Allocated 1000 Bytes at 0x0x3ffafa98, total 188488
Allocated 1000 Bytes at 0x0x3ffe0448, total 189496
...
Allocated 1000 Bytes at 0x0x3ffe3b68, total 203608
Allocated 1000 Bytes at 0x0x3ffe4358, total 204616
...
Allocated 1000 Bytes at 0x0x3ffffb78, total 316504
Allocations count: 314

Issues/PRs references

Depends on PR #13516
Requires PR RIOT-OS/riotdocker#102 to work correctly.

@gschorcht gschorcht requested a review from benpicco February 29, 2020 12:29
@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 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 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.

I guess https://doc.riot-os.org/group__cpu__esp32.html#esp32_manual_toolchain_installation will need an update too.

Looks good, please rebase (and remove the trailing whitespace while you're at it, so Travis is happy).

@benpicco benpicco added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Mar 29, 2020
@gschorcht gschorcht force-pushed the cpu/esp32/multiheap branch from f6af10c to 6fddd57 Compare March 30, 2020 07:28
@gschorcht
Copy link
Contributor Author

I guess https://doc.riot-os.org/group__cpu__esp32.html#esp32_manual_toolchain_installation will need an update too.

No. I just forgot to push the master. The documentation doesn't refer a certain commit as riotdocker does. For simplicity's sake, the master is always used. I have pushed the master so that it corresponds to the version used for riotdocker.

@benpicco benpicco added the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Mar 30, 2020
@benpicco
Copy link
Contributor

I thought you would have squashed the whitespace fix directly 😉
Anyway, @kaspar030 needs to update Murdock first so CI will not fail.

@gschorcht
Copy link
Contributor Author

gschorcht commented Mar 30, 2020

Anyway, @kaspar030 needs to update Murdock first so CI will not fail.

Indeed, tests/malloc failed 😟 I didn't think that tests/malloc would recognize that the chunk allocation with the old fat malloc is wrong.

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

benpicco commented Apr 3, 2020

My suspicion is correct - tests/malloc works now, so the toolchain was upgraded - pleas squash btw.

@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 Apr 4, 2020
@benpicco benpicco removed the State: waiting for CI update State: The PR requires an Update to CI to be performed first label May 20, 2020
@benpicco
Copy link
Contributor

The update has finally reached every CI worker I think - this needs another rebase though :)

@benpicco
Copy link
Contributor

benpicco commented Jul 9, 2020

ping @gschorcht 😉

@gschorcht gschorcht force-pushed the cpu/esp32/multiheap branch from 6fddd57 to f42d6bb Compare July 10, 2020 06:09
@gschorcht gschorcht added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jul 10, 2020
@benpicco
Copy link
Contributor

Please squash

Several unsused DRAM sections are added to the heap.
@gschorcht gschorcht force-pushed the cpu/esp32/multiheap branch from f42d6bb to fb47f09 Compare July 10, 2020 06:42
@gschorcht
Copy link
Contributor Author

gschorcht commented Jul 10, 2020

Murdock failed because of the following tests that are not related to this PR (output).

    tests/progress_bar/nrf52dk:gnu
    examples/micropython/nrf52dk:gnu
    tests/float/nrf52dk:gnu

Even stranger is that Murdock only did some tests on the nrf52dk board, but none on esp32-wroom-32. I don't understand how tests to run are selected.

@gschorcht gschorcht added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before and removed CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before labels Jul 10, 2020
@gschorcht
Copy link
Contributor Author

gschorcht commented Jul 10, 2020

@benpicco Do you think we should trigger Murdock again with the label CI disable test cache to force all tests to run on ESP32? Or should we simply remove label CI run tests to pass Murdock compilation?

@benpicco benpicco added 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 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 Jul 10, 2020
@benpicco
Copy link
Contributor

The tests on nrf52dk are flaky, I don't know why - but looks like now everything worked.

@gschorcht
Copy link
Contributor Author

The tests on nrf52dk are flaky, I don't know why - but looks like now everything worked.

Yeah, but only because no test was run because of the test cache. Should we run the test again with the label CI clear test cache so we can be sure that the tests on the ESP32 will still run after the rebase?

@benpicco
Copy link
Contributor

I ran the test on a local esp32 and it showed no errors. But if you want, you can run Murdock again.

@gschorcht
Copy link
Contributor Author

🤔 It probably makes no sense to run all tests with static data again. The only test that probably uses dynamic memory is tests/malloc. So lets merge before the hard feature freeze.

@benpicco benpicco merged commit 6b9edb7 into RIOT-OS:master Jul 10, 2020
@gschorcht
Copy link
Contributor Author

@benpicco Thanks for reviewing, testing and merging.

@miri64 miri64 added this to the Release 2020.07 milestone Jul 10, 2020
@gschorcht gschorcht deleted the cpu/esp32/multiheap branch July 10, 2020 14:55
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: run tests If set, CI server will run tests on hardware for the labeled PR 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: 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.

3 participants