::onReceive() implementation & calling functions not in IRAM

[mcspr]

ref. esp8266/Arduino#8830, user reports frequent crashes when using 7.0.0
most likely cause being queue's available(), and the delegate check & call are not in IRAM

suppose one way to solve this is from our side and force things into IRAM manually, but I'd think esp32 would have similar issue when running out of cache
esp8266/Arduino#8834

[dok-net]

@mcspr I've pushed changes that should fix what can be done on this side to master.
Beyond that, it looks to me that the original reporter has a lib that didn't fully understand the implications of the breaking change in 7.0.0. They might just be doing the same things in their onReceive handler as before, but now it's in interrrupt context, which means they can only set a flag and must return ASAP.

[mcspr]

Original report crashed in ISR without even touching onReceive callback, stopping on queue::available()

I switched the commit in the local copy and rebuilt my test project, but still see at least one delegate::operator bool()

(gdb) pipe disassemble 'SoftwareSerial::rxBitISR' | grep -B2 call
   0x40100500 <+20>:    l32i.n  a15, a12, 28
   0x40100502 <+22>:    s32i.n  a0, a1, 44
   0x40100504 <+24>:    call0   0x40100740 <micros()>
--
   0x4010053f <+83>:    s32i    a6, a1, 0
   0x40100542 <+86>:    l32r    a0, 0x40100374
   0x40100545 <+89>:    callx0  a0
--
   0x40100590 <+164>:   mov.n   a2, a12
   0x40100592 <+166>:   l32r    a0, 0x40100378
   0x40100595 <+169>:   callx0  a0
(gdb) x *0x40100374
0x4000e268:     Cannot access memory at address 0x4000e268 <<< .ld has PROVIDE ( __umodsi3 = 0x4000e268 );, guess it is ok?
(gdb) x *0x40100378
0x4020bb98 <delegate::detail::DelegateImpl<void*, void>::operator bool() const>:        0x050c4248
(gdb)

[dok-net]

@mcspr You have the source code, which one could that possibly be, I have looked over and over now, every operator bool() has the IRAM_ATTR annotation. Unless inlining somehow is causing this to be ignored, I'm at a loss how to explain your result but that you are not actually compiling the updated Delegate.h?

[mcspr]

Sources are in order, building latest esp8266 git with forcibly reset submodule

~/.p/p/f (master)> git log --oneline -1
d7da591e (HEAD -> master, origin/master, origin/HEAD) Revert ESPSoftwareSerial from v7 to v6
~/.p/p/f (master)> git -C libraries/SoftwareSerial log --oneline -1
254bf85 (HEAD, origin/main) move with rvalue-refs

We are just lucky to hit a very special condition under which attributes don't work (in GCC) - templates.
espressif/esp-idf#4542
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70435
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88061

So, things go to flash like they would by default.

[dok-net]

@mcspr If you could, please check commit c6e2001. I'm forcibly inlining all pertaining functions now. Maybe have an eye on the binary size, too?

[mcspr]

Having thought about the issue...
https://godbolt.org/z/dfd4xWa8e

Before any call to the specific template, it can be manually instantiated like this

template something<Type>();

Attribute seems to be working under these conditions (see readelf output on the right, there is .handler section!). Would be interesting to see whether this works for this code.

Regarding the commit, delegate mentions are completely gone from irom. Binary size goes up, as expected.

~/d/swserial270> nm -S -C --radix=d 254bf852.elf | grep 'rxBit.*SoftwareSerial'
1074791296 00000363 T SoftwareSerial::rxBitSyncISR(SoftwareSerial*)
1074791660 00000211 T SoftwareSerial::rxBitISR(SoftwareSerial*)
~/d/swserial270> nm -S -C --radix=d c6e20015.elf | grep 'rxBit.*SoftwareSerial'
1074791320 00000420 T SoftwareSerial::rxBitSyncISR(SoftwareSerial*)
1074791740 00000268 T SoftwareSerial::rxBitISR(SoftwareSerial*)

[dok-net]

@mcspr Latest commit, this would be it from my side. Haven't investigated explicit specialization, interesting to know, more trouble than it seems worth in the case at hand (?). If you are OK with the results - the 3rd party issue of possibly needing to fix their onReceive handler non-withstanding - I would push a release.

[mcspr]

Looks fine to me.

Also, https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html
Inlining still works, but depends on visibility (anonymous namespaces or static).

---

BTW, there is also std::_Function_handler<void(), *>::_M_invoke(std::_Any_data const&) that is in flash for callable objects or lambdas we are going to call in std function operator().
Shouldn't the callback be a simple pointer since it (seemingly) can't call complex objects without more .ld tweaks?

[dok-net]

@mcspr Thanks for all the input. It's not quite intuitive, finding the right class template in Delegate.h, but I am confident I have managed it and now the required bits are neither inlined nor placed outside the IRAM cache address range.
Would you mind verifying? I notice that EspSoftwareSerial was reverted in ESP82266 Arduino core - given this latest fix in EspSoftwareSerial main, are you OK if I prepare a new release and submit the PR?

[mcspr]

lgtm based on repeating tests above, thx
(I will try to test with some real hw some time next week)

Main breaking concern v6 -> v7 is onReceive context change, but I am of two minds about it.
Signature change is pretty apparent change from my pov, but I wonder if method as well as name would be more clear on what it does with a name change?

Meaning, keep the void onReceive(T) { /* do nothing */ } and prepend it with [[deprecated(...why...)]] // [[gnu::unavailable(...)]] // [[gnu::error(...)]]; not template error, but a clear warning or a failure state

[[deprecated("Don't use this one")]]
void onReceive(Func<void(int)>);

void onReceiveISR(Func<void()>);

[dok-net]

I wasn't aware that anyone had considered using the onReceive() function, let alone implemented its use. I think I stand by this breaking change, which IIRC is clearly mentioned in the descriptions. I would not name the set-handler function onReceiveISR(). It's for expert use anyway, given the requirements of running in ISR context, plus it's probably only useful for a few programs that would not be constantly polling the serial inputs anyway.

Once I am satisfied with the work I am doing on giving users more freedom with strapping pins, I will submit another PR. If you can totally not agree with me opinion, of course, let me know, I'm not completely ideological.

[mcspr]

Just a concern about versioning & deprecation like it is described by semver, since we bundle it with esp8266 Core vs. manual upgrade from library manager.
https://semver.org/#how-should-i-handle-deprecating-functionality
(and /deprec/ across the document)
It is a speculation, that I agree upon :> We don't know of any users of the original API, if it was ever used to begin with. My first hunch was to add some notices in code directly.

[dok-net]

@mcspr 3dc3b23, what do you think? Still a hard breaking change between direct version successors, of this lib. Users of the ESP8266 / ESP32 cores oftentimes seem not to use every main branch revision or even every release of the cores, so they would miss a transitional minor or bug fix release of EspSoftwareSerial anyway...
Good (enough) to go?

[mcspr]

Any reason not to also have onReceive like above, to also avoid template expansion error for delegate?
Should be, 3.1.2 is pending

[dok-net]

It doesn't compile due to overload resolution ambiguity. I don't think it matters at all, though, because I expect nobody uses one without the other - without perform_work() there is zero point in using onReceive() before the changes since 7.0.0.
I should be able to finalize 7.0.1 during the upcoming weekend. Unless you are completely dissatisfied with my handling of the deprecation, expect no major changes - I am certain there is a way to put the new ISR-trigger receiver handler to good work, but I don't have any code yet because it requires a good deal more work. Until then, everone can just replace perform_work() by some own code looping over available(), the whole original idea never made things so much easier to begin with :-)
Maybe I should mention what I have in mind:
There are two scenarios - one, low power modes. There, one would set the RX pin to wakeup from low-power sleep. Probably no need to even use onReceive.
The other, multithreading (remember CoopTask; possible the fine ESP8266 Core built-in Schedule, too) - rescheduling a thread or a coroutine due to incoming data. The SDS011 particulate matter sensor library is my project where I usually apply these features, albeit not in the examples just yet.
By the way, would you have some spare time to review my solution to esp8266/Arduino#7055, viz. esp8266/Arduino#7979? I would really be deeply grieved if it keeps getting sidelined forever and even becomes unmergeable if someone else modifies some of the underlying code? I think I have seen more than one issue since, that would be helped by the cleaned-up API :-(

[dok-net]

Dear @mcspr , I have also added an example for the use of the RX callback now and measured the possible power savings this allows (aba98fc, esp8266/Arduino#8869). A shameless plug, if the above-mentioned power API PR were merged, I would try to implement the deep sleep with GPIO wakeup as well - strictly speaking this doesn't require the callback, I know - and give measurements. For simplicity, I'm deploying just a USB power meter, not measuring the current input for the pure ESP8266 module alone.
If you have some spare time, please review the example for any race condition?

[mcspr]

lgtm? I haven't considered the lowpower application, does sound like a pretty good example to have

race condition wise, only pretty minor thing that comes to mind is rxPending = avail > 0; and immediately checking its value. gcc seems to prefer the assigned value, but adding *reinterpret_cast<volatile bool*>(&rxPending) works around that

I think mentioned PRs can be part of 3.2.0, but I don't have any strong opinions yet so pls bear with me

[dok-net]

@mcspr Thanks for looking at everything!
W.r.t. rxPending = avail > 0;: You pointed me toward a race condition, it takes a little deeper inspection, right now I believe the code may perform reads despite available() == 0.

[dok-net]

For those reading here: the discussion has now completely shifted to the issue for merging with ESP8266 Arduino Core esp8266/Arduino#8869

[dok-net]

As I stated yesterday, follow and join the discussion at the ESP8266 core repository.