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

Split IRAM into 2 linker sections to move std::fcn #5922

Merged
merged 8 commits into from
Apr 10, 2019

Conversation

earlephilhower
Copy link
Collaborator

Callbacks need to be placed in IRAM when being called from an IRQ (like
the SPISlave callbacks).

This can be done by hacking the std::functional header and making every
single specialization of the template into an IRAM section, which would
take a lot of space for no benefit in the majority of cases.

The alternate is to specify the single instantiation types/operators
required, but the problem is the flash segment matcher would match them
before the IRAM section was begun, and rules for them would just not be
applied.

Get around this by splitting the IRAM section definition into .text and
.text1. This is linker syntactic sugar and does not actually change the
on-chip layout. But it does allow us to put the exception vectors at
the required absolute addresses and add single functions to IRAM.

Fixes #5914 (maybe, may need to add other template types)

Callbacks need to be placed in IRAM when being called from an IRQ (like
the SPISlave callbacks).

This can be done by hacking the std::functional header and making every
single specialization of the template into an IRAM section, which would
take a lot of space for no benefit in the majority of cases.

The alternate is to specify the single instantiation types/operators
required, but the problem is the flash segment matcher would match them
before the IRAM section was begun, and rules for them would just not be
applied.

Get around this by splitting the IRAM section definition into .text and
.text1.  This is linker syntactic sugar and does not actually change the
on-chip layout.  But it does allow us to put the exception vectors at
the required absolute addresses and add single functions to IRAM.
@dumarjo
Copy link
Contributor

dumarjo commented Mar 29, 2019

@earlephilhower You will need all the types in RAM.

typedef std::function<void(uint8_t *data, size_t len)> SpiSlaveDataHandler;
typedef std::function<void(uint32_t status)> SpiSlaveStatusHandler;
typedef std::function<void(void)> SpiSlaveSentHandler;

@earlephilhower earlephilhower changed the title Split IRAM into 2 linker sections to move std::fcn WIP: Split IRAM into 2 linker sections to move std::fcn Apr 9, 2019
@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Apr 9, 2019

I just tried the PR on a very simple example and got into an infinite boot loop. Must've gotten lucky the first time I tried things.

Basically the elf is fine, but the BIN is not pulling in the .text1 segment for actual image creation.

Need to update elf2bin.py...

The extra segment name needs to be placed into the output binary as
well, or else only the init code gets stored and none of the real app is
present.  This leads to an infinite boot loop.

This change adds in the segment to the generated image.
@earlephilhower earlephilhower changed the title WIP: Split IRAM into 2 linker sections to move std::fcn Split IRAM into 2 linker sections to move std::fcn Apr 9, 2019
@earlephilhower
Copy link
Collaborator Author

Latest push actually uploads the app main code and runs on tests I've thrown at it.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

LGTM!

@devyte
Copy link
Collaborator

devyte commented Apr 9, 2019

A warning here: only std::function::operator()() is moved to IRAM. The constructor is not. That means that re-attaching via the functional api from an ISR can still crash, because that will construct a temp std::functional and pass it in, and the constructor could potentially not be in IRAM.
I think it's a case rare enough to not warrant the price in IRAM.

@hreintke
Copy link
Contributor

@earlephilhower :
Don't know whether it is intentional and/or it has any impact but when I was looking into the objdump output checking for functional interrupt I noticed.

From he title of the PR I expected to see both .text and .text1 segments. .text being all as before, .text1 for the specific std::functions.
But it looks like all gets into .text1.

part of objdump before the commit :

40238048 g     F .irom0.text	00000038 periodic_cal
4021dc3c g     F .irom0.text	00000068 ieee80211_pwrsave
40105ff8 g     F .text	00000067 phy_get_bb_freqoffset
401062ec g     F .text	0000002b clockgate_watchdog
40102f28 g     F .text	0000000f lmacIsActive
40101180 g     F .text	00000022 pp_post2
3ffedd54 g     O .bss	00000004 timer_list
40204028 g     F .irom0.text	00000041 _ZN12UpdaterClassC1Ev
40103e34 g     F .text	00000026 lmacMSDUAged
00000220 g       *ABS*	00000000 _memmap_cacheattr_bp_base
402205d8 g     F .irom0.text	00000049 cnx_sta_connect_led_timer_cb
40105e70 g     F .text	00000151 tx_pwr_backoff_old
4022cd3c g     F .irom0.text	00000008 fpm_get_slp_type
40204374  w    F .irom0.text	0000009e _ZN8conststr15parseNthIntegerILj12EEEiRAT__Kcj
402391c8 g     F .irom0.text	0000000b phy_set_powerup_option

Same part after the commit

40238048 g     F .irom0.text	00000038 periodic_cal
4021dc3c g     F .irom0.text	00000068 ieee80211_pwrsave
40105ff8 g     F .text1	00000067 phy_get_bb_freqoffset
401062ec g     F .text1	0000002b clockgate_watchdog
40102f28 g     F .text1	0000000f lmacIsActive
40101180 g     F .text1	00000022 pp_post2
3ffedd54 g     O .bss	00000004 timer_list
40204028 g     F .irom0.text	00000041 _ZN12UpdaterClassC1Ev
40103e34 g     F .text1	00000026 lmacMSDUAged
00000220 g       *ABS*	00000000 _memmap_cacheattr_bp_base
402205d8 g     F .irom0.text	00000049 cnx_sta_connect_led_timer_cb
40105e70 g     F .text1	00000151 tx_pwr_backoff_old
4022cd3c g     F .irom0.text	00000008 fpm_get_slp_type
40204374  w    F .irom0.text	0000009e _ZN8conststr15parseNthIntegerILj12EEEiRAT__Kcj
402391c8 g     F .irom0.text	0000000b phy_set_powerup_option

mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 11, 2019
…ipt for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 12, 2019
…ipt for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 12, 2019
…ipt for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 13, 2019
…ipt for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 13, 2019
…ipt for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
slaff pushed a commit to SmingHub/Sming that referenced this pull request Sep 15, 2019
* Add `CallbackTimer` class template to support HardwareTimer, SimpleTimer and Timer.

- Contains common logic and checks for all callback timer types
- Templated for best performance (no VMT)
- Added `TimerCallback` supporting void* arg parameter (in addition to existing `InterruptCallback`)
- Templated methods added with compile-time checks on interval (so code won't compile if out of range)
- Added methods to support setting/checking/querying intervals directly in timer ticksn
- Timer intervals stored internally in timer ticks, so querying the value confirms the actual time in use accounting for rounding, etc.

Note that delegate callbacks are supported only by the Timer class, which now also has an AutoDelete variant.

Host timer queue implementation improved, handles multiple timers properly (fixes bug where all timers get cancelled instead of just one).

Add `os_timer_arm_ticks()` function so software timers are programmed in ticks instead of microseconds or milliseconds

- Used instead of `os_timer_arm_us` provides simpler and more flexible timer interface.
- Avoids un-necessary (and potentially inaccurate) time conversions
- Timers can be used with `USE_US_TIMER=0` for 3.2us tick resolution, providing basic range of 1'54" (with SimpleTimer)
- Default is still `USE_US_TIMER=1` for 0.2us tick resolution and reduced 0'7"9s range (without using Timer class)

* Update samples to use improved timer API and add timers module to HostTests

* Templates ignore section attributes so requires entries in linker script for IRAM code.

In the source code you'll see quite a bit of `__forceinline` as well as `IRAM_ATTR`, which is mainly for safety
and to indicate functions/methods may be used from interrupt context.

Unfortunately, marking templated code with `IRAM_ATTR` is not sufficient to get it into IRAM:

https://stackoverflow.com/questions/36279162/section-attribute-of-a-function-template-is-silently-ignored-in-gcc

So the linker script needs to be updated to catch all such instances. To do this requires splitting the `.text` output
segment into two parts, called `.text` and `.text1`. The rBoot script also needs to know about both segments
so they both end up in the ROM image it creates.

Using `CallbackTimer` as an example, most of the code gets inlined anyway and as it's called from task context
this actually uses very little IRAM.

More details here:

esp8266/Arduino#5922
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hspi in slave mode with Wifi enabled produce crash dump
5 participants