Fixes and implementation to expose attachInterruptArg in Arduino.h

[dok-net]

• attachInterrupt and detachInterrupt don't require ICACHE_RAM_ATTR.
• fix incorrect handling of non-"functional" interrupt handlers.
• complete and expose attachInterruptArg().

[dok-net]

@earlephilhower In order to go ahead with an efficient small footprint EspSoftwareSerial, I've ported parts of the attachInterruptArg implementation from ESP32 Arduino. Keeping double tables in libraries using interrupts to account for the until now missing argument to ISRs, whereas the interrupt attachment code already keeps such tables, is now obviated.

Also, I found that the use of ICACHE_RAM_ATTR on attachInterrupt and detachInterrupt is strictly redundant, consuming the constrained resource while the part of attaching/detaching interrupts has no real-time requirements AFAIK.

Once this PR is merged, I have the matching EspSoftwareSerial PR right at hand :-)

[hreintke]

@dok-net : Been busy with other stuff, I just noticed this thread.
I am the original author of functional interrupt code, both on esp8266 and esp32.

The difference you noticed in implementation happened because of the sequence in patches on both platforms.
ESP8266 : functional interrupts added when interruptArg was not exposed -> no need to get this exposed as it is a specialization on the generic interrupt. This can be achieved using std:bind.
ESP32 : First a patch was introduced which exposed interruptArg -> need to keep backward compatibility. There are now two ways to achieve the same result.

Unless I missed some specific items/issues I expect you can get ESP8266 and ESP32 code compatibility without this update

[dok-net]

To split it up:
1 - ICACHE_RAM_ATTR - do you agree that it's right to drop the ones my PR removes?
2 - publicly exposing attachInterruptArg() like attachInterrupt() is nice-to-have
3 - there is also the misconception out in the field that __attachInterruptArg() works the same (#6002 (comment))
4 - but it's broken for non-functional ISRs without the checks this PR adds (crashes)
5 - none of the benefits of the ArgStructure, prefabricated pin, value, and timestamp fields, that are the core of the ISR in EspSoftwareSerial and would be of benefit, are there in ESP32, so no portability for that is given.
I think the overhead by the patch is minimal, maybe even less than using std::bind etc al, I haven't checked.
Please merge the PR?

[hreintke]

1 - ICACHE_RAM_ATTR - do you agree that it's right to drop the ones my PR removes?

Yes, (most) use of attach/detach will be outside the ISR. However I do have a homeautomation application where I change attach to/from CHANGE/RISE and detach in ISR. For me it is "just another" local patch to add.
@d-a-v : Do you know a way to add an ICACHE_RAM attribute to a core/library function without local patch ?

2 - publicly exposing attachInterruptArg() like attachInterrupt() is nice-to-have

Up to @d-a-v whether to have nice-to-haves in the core

5 - none of the benefits of the ArgStructure, prefabricated pin, value, and timestamp fields are there in ESP32,

That is scheduled interrupt functionality. There are no (t yet) scheduled functions in ESP32.
One of my ESP32 local patches :)

[d-a-v]

2 - publicly exposing attachInterruptArg() like attachInterrupt() is nice-to-have

Up to @d-a-v whether to have nice-to-haves in the core

Nothing against that myself, especially if it goes towards esp8266/esp32 unofficial arduino api compatibility.

Do you know a way to add an ICACHE_RAM attribute to a core/library function without local patch ?

About the ability to attach/detach interrupts from an ISR, there is in fact a solution. It is quite fair, as you say, to do that in some case. @earlephilhower mentionned it too, few days ago. What is feasible is the following:

#if ALLOW_SETTING_ISR_IN_ISR
#define ICACHE_RAM_ATTR_ISR ICACHE_RAM_ATTR
#else 
#define ICACHE_RAM_ATTR_ISR
#endif

(I think this would go into tools/sdk/include/c_types.h next to ICACHE_RAM_ATTR definition)

Where relevant, change ICACHE_RAM_ATTR to ICACHE_RAM_ATTR_ISR like:

extern "C" void ICACHE_RAM_ATTR_ISR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void*fp , int mode);

Now when a projects needs that, add in platform.local.txt (or equivalent in PIO or sloeber) the following:

compiler.c.extra_flags=-DSTASSID="xxxx" -DSTAPSK="yyyy" -DPUYA_SUPPORT=1 -DALLOW_SETTING_ISR_IN_ISR=1

and in the sketch:

#if !ALLOW_SETTING_ISR_IN_ISR
#error ALLOW_SETTING_ISR_IN_ISR=1 is needed for this sketch
#endif 

(this is not yatmo - Yet Another Tools Menu Option ;)

[d-a-v]

After reading several times, I don't understand why it is needed to have this bool functional.
It is always checked along with the not-nullptr arg. Why is arg not sufficient ?

[hreintke]

I will do a detailed check (maybe thursday. otherwise friday).
I needed it in ESP32 to distinguish the use of attachinterruptArg from user exposed and usage for functional.

[dok-net]

@d-a-v The bool functional prevents undefined behavior of type-casting void* arg to ArgStructure* and dereferencing in all those cases that the whole exercise of exposing
void attachInterruptArg(uint8_t pin, void (*)(void*), void * arg, int mode)
is done for: whenever it's not an ArgStructure*, not null either, but for instance some this pointer or whatever else fits into void* (32bit integers).
I've revisited the code and I am quite sure it's needed where it is.

[dok-net]

FYI: I've tested using microsecond resolution instead of CPU cycles for the Software Serial bit timing. The ESP8266 shows no difference, but on the ESP32, at high bit rate (57600cps), the error rate increases measurably.

[d-a-v]

Given the reduction of the complexity of the generated code, I always try to cpu cycle (if measured durations are less than 26 seconds (esp8266@160MHz) or if overflow is managed)

[dok-net]

But here (SW serial) it impacts basic usefulness, each worsening in error rate renders it useless in more cases.
My point of view is that unconditionally requiring C++ functional objects in the attachInterruptArg interface through an API that looks like plain C attracts confusion. The compiler can't catch any of it. I have to admit, that I am not entirely happy with the whole construct between FunctionalInterrupt and core_esp8266_wiring_digital, exemplified by the need to copy&paste type definitions
//duplicate from functionalInterrupt.h keep in sync
typedef struct InterruptInfo
because the relationship is bit forced as it is right now.
But anyway, in ESP32 Arduino, it's just the same, so future patches just might take advantage or a more in similarity.

[dok-net]

In this spot, discerning only null from non-null causes undefined behavior in all non-"functional" uses due to accessing arg->interruptInfo.
Therefore, the check for functional == true is correct and necessary.

[d-a-v]

Right, I missed that, thanks for the explanation

[d-a-v]

Why is it closed ?

[dok-net]

Entirely not on purpose!!! Must be a side effect of me trying to avoid ugly merge history by local rebase followed by force push to my git repo. Dammit :-)

[dok-net]

Re-pushed (ugly merge commits, but opinions on merge vs. rebase differ wildly anyhow).

[dok-net]

Please observe #6048