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.

Sign up for GitHub

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

Fixes and implementation to expose attachInterruptArg in Arduino.h #6003

Closed
wants to merge 24 commits into from
Closed

Fixes and implementation to expose attachInterruptArg in Arduino.h #6003

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5c8c68f
- attachInterrupt and detachInterrupt don't require ICACHE_RAM_ATTR.
dok-net Apr 20, 2019
669b9ce
Fix CI warnings.
dok-net Apr 21, 2019
3c8a851
Revert "Fix CI warnings."
dok-net Apr 21, 2019
c59a2c2
Fix CI warnings.
dok-net Apr 21, 2019
b7ca699
Done fixing CI warnings.
dok-net Apr 21, 2019
20ff987
Merge branch 'master' into master
dok-net Apr 23, 2019
7e77ba6
Merge branch 'master' into master
dok-net Apr 24, 2019
c376c97
Merge remote-tracking branch 'origin/master'
dok-net Apr 24, 2019
3594450
Merge remote-tracking branch 'dok-net/master'
dok-net Apr 25, 2019
d6190fd
Merge commit '359445077a6d7bc36743e4dede7794015956b725'
dok-net Apr 25, 2019
c353b51
Merge branch 'master' into master
dok-net Apr 27, 2019
efafcbd
- attachInterrupt and detachInterrupt don't require ICACHE_RAM_ATTR.
dok-net Apr 20, 2019
e04ac23
Fix CI warnings.
dok-net Apr 21, 2019
64a56c9
Revert "Fix CI warnings."
dok-net Apr 21, 2019
38ef0f1
Fix CI warnings.
dok-net Apr 21, 2019
254302b
Done fixing CI warnings.
dok-net Apr 21, 2019
78aa3e8
Track breaking API chance in EspSoftwareSerial.
dok-net Apr 25, 2019
1ecf11c
Merge remote-tracking branch 'dok-net/espswserialapi'
dok-net Apr 29, 2019
1b4077f
Update SoftwareSerial release only after PR containing interrupt hand…
dok-net Apr 29, 2019
f21db95
Revert "Track breaking API chance in EspSoftwareSerial."
dok-net Apr 29, 2019
3a206f2
Merge branch 'master' into master
dok-net May 1, 2019
4a78b1a
Code formatting cleaned up a bit.
dok-net May 1, 2019
b03e7c7
Merge branch 'master' into master
dok-net May 3, 2019
bbd9948
Merge branch 'master' into master
dok-net May 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cores/esp8266/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ void shiftOut(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder, uint8_t val);
uint8_t shiftIn(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder);

void attachInterrupt(uint8_t pin, void (*)(void), int mode);
void attachInterruptArg(uint8_t pin, void (*)(void*), void * arg, int mode);
void detachInterrupt(uint8_t pin);

void preinit(void);
Expand Down
6 changes: 3 additions & 3 deletions cores/esp8266/FunctionalInterrupt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ typedef void (*voidFuncPtr)(void);
typedef void (*voidFuncPtrArg)(void*);

// Helper functions for Functional interrupt routines
extern "C" void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void*fp , int mode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dok-net dok-net Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@dok-net dok-net Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

extern "C" void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtr userFunc, void*fp , int mode, bool functional);


void ICACHE_RAM_ATTR interruptFunctional(void* arg)
Expand Down Expand Up @@ -49,7 +49,7 @@ void attachInterrupt(uint8_t pin, std::function<void(void)> intRoutine, int mode
as->interruptInfo = ii;
as->functionInfo = fi;

__attachInterruptArg (pin, (voidFuncPtr)interruptFunctional, as, mode);
__attachInterruptFunctionalArg (pin, (voidFuncPtr)interruptFunctional, as, mode, true);
}

void attachScheduledInterrupt(uint8_t pin, std::function<void(InterruptInfo)> scheduledIntRoutine, int mode)
Expand All @@ -67,5 +67,5 @@ void attachScheduledInterrupt(uint8_t pin, std::function<void(InterruptInfo)> sc
as->interruptInfo = ii;
as->functionInfo = fi;

__attachInterruptArg (pin, (voidFuncPtr)interruptFunctional, as, mode);
__attachInterruptFunctionalArg (pin, (voidFuncPtr)interruptFunctional, as, mode, true);
}
49 changes: 29 additions & 20 deletions cores/esp8266/core_esp8266_wiring_digital.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@ typedef void (*voidFuncPtrArg)(void*);

typedef struct {
uint8_t mode;
void (*fn)(void);
void * arg;
voidFuncPtr fn;
void* arg;
bool functional;
} interrupt_handler_t;

//duplicate from functionalInterrupt.h keep in sync
Expand All @@ -125,11 +126,10 @@ typedef struct {
void* functionInfo;
} ArgStructure;

static interrupt_handler_t interrupt_handlers[16];
static interrupt_handler_t interrupt_handlers[16] = { {0, 0, 0, 0}, };
static uint32_t interrupt_reg = 0;

void ICACHE_RAM_ATTR interrupt_handler(void *arg) {
(void) arg;
void ICACHE_RAM_ATTR interrupt_handler(void *) {
uint32_t status = GPIE;
GPIEC = status;//clear them interrupts
uint32_t levels = GPI;
Expand All @@ -147,13 +147,16 @@ void ICACHE_RAM_ATTR interrupt_handler(void *arg) {
// to make ISR compatible to Arduino AVR model where interrupts are disabled
// we disable them before we call the client ISR
uint32_t savedPS = xt_rsil(15); // stop other interrupts
ArgStructure* localArg = (ArgStructure*)handler->arg;
if (localArg && localArg->interruptInfo)
{
localArg->interruptInfo->pin = i;
localArg->interruptInfo->value = __digitalRead(i);
localArg->interruptInfo->micro = micros();
}
if (handler->functional)
{
ArgStructure* localArg = (ArgStructure*)handler->arg;
if (localArg && localArg->interruptInfo)
Copy link
Contributor Author

@dok-net dok-net Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I missed that, thanks for the explanation

{
localArg->interruptInfo->pin = i;
localArg->interruptInfo->value = __digitalRead(i);
localArg->interruptInfo->micro = micros();
}
}
if (handler->arg)
{
((voidFuncPtrArg)handler->fn)(handler->arg);
Expand All @@ -170,8 +173,7 @@ void ICACHE_RAM_ATTR interrupt_handler(void *arg) {

extern void cleanupFunctional(void* arg);

extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void *arg, int mode) {

extern void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtrArg userFunc, void *arg, int mode, bool functional) {
// #5780
// https://github.com/esp8266/esp8266-wiki/wiki/Memory-Map
if ((uint32_t)userFunc >= 0x40200000)
Expand All @@ -185,12 +187,13 @@ extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFu
ETS_GPIO_INTR_DISABLE();
interrupt_handler_t *handler = &interrupt_handlers[pin];
handler->mode = mode;
handler->fn = userFunc;
if (handler->arg) // Clean when new attach without detach
handler->fn = (voidFuncPtr)userFunc;
if (handler->functional && handler->arg) // Clean when new attach without detach
{
cleanupFunctional(handler->arg);
}
handler->arg = arg;
handler->functional = functional;
interrupt_reg |= (1 << pin);
GPC(pin) &= ~(0xF << GPCI);//INT mode disabled
GPIEC = (1 << pin); //Clear Interrupt for this pin
Expand All @@ -200,12 +203,16 @@ extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFu
}
}

extern void ICACHE_RAM_ATTR __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode )
extern void __attachInterruptArg(uint8_t pin, voidFuncPtrArg userFunc, void * arg, int mode)
{
__attachInterruptArg (pin, userFunc, 0, mode);
__attachInterruptFunctionalArg(pin, userFunc, arg, mode, false);
}

extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode ) {
__attachInterruptFunctionalArg(pin, (voidFuncPtrArg)userFunc, 0, mode, false);
}

extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) {
extern void __detachInterrupt(uint8_t pin) {
if(pin < 16) {
ETS_GPIO_INTR_DISABLE();
GPC(pin) &= ~(0xF << GPCI);//INT mode disabled
Expand All @@ -214,11 +221,12 @@ extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) {
interrupt_handler_t *handler = &interrupt_handlers[pin];
handler->mode = 0;
handler->fn = 0;
if (handler->arg)
if (handler->functional && handler->arg)
{
cleanupFunctional(handler->arg);
}
handler->arg = 0;
handler->functional = false;
if (interrupt_reg)
ETS_GPIO_INTR_ENABLE();
}
Expand All @@ -243,6 +251,7 @@ extern void pinMode(uint8_t pin, uint8_t mode) __attribute__ ((weak, alias("__pi
extern void digitalWrite(uint8_t pin, uint8_t val) __attribute__ ((weak, alias("__digitalWrite")));
extern int digitalRead(uint8_t pin) __attribute__ ((weak, alias("__digitalRead")));
extern void attachInterrupt(uint8_t pin, voidFuncPtr handler, int mode) __attribute__ ((weak, alias("__attachInterrupt")));
extern void attachInterruptArg(uint8_t pin, voidFuncPtrArg handler, void * arg, int mode) __attribute__ ((weak, alias("__attachInterruptArg")));
extern void detachInterrupt(uint8_t pin) __attribute__ ((weak, alias("__detachInterrupt")));

};