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

Completes #6048: let FunctionalInterrupt.h support only IRAM-safe scheduled ISRs #6055

Closed
wants to merge 3 commits into from

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented May 6, 2019

Makes for a much cleaner cores/esp8266/core_esp8266_wiring_digital.cpp:

  • no special "functional" predicate
  • no reverse "duplicate from functionalInterrupt.h keep in sync" copied-code dependency

@hreintke
Copy link
Contributor

hreintke commented May 9, 2019

@dok-net @d-a-v @devyte
disclaimer : I did not read all the remarks from the last days as I was on a short break.

Doesn't solve #5922 the issue that std::function<void(void)> cannot be used in attachInterrupt due to not being in IRAM ?

@devyte
Copy link
Collaborator

devyte commented May 9, 2019

it should. I've only taken a quick glance at the recent PRs, we're currently trying to get 2.5.1 out the door.

@dok-net
Copy link
Contributor Author

dok-net commented May 9, 2019

@devyte @hreintke Maybe it's just me, I don't understand the point of using functional if all that's supported are static functions. Please run, commenting / uncommenting the various cases, the attached MCVE. I think it's a bug, misleading users into attaching ISRs that are not in IRAM, the whole point of #5995 is voided, too.
AttachInterruptArg.ino.txt

@d-a-v
Copy link
Collaborator

d-a-v commented May 11, 2019

I don't understand the point of using functional if all that's supported are static functions

I agree.

You have lots of running PRs. Are they all related ? If so can you group them in one single PR ?
To be honest I'm quite lost :]

@hreintke
Copy link
Contributor

hreintke commented May 12, 2019

I tried to get std::function::target running with rtti option set to verify myself but I only got crashes.

If I have a construct like

#include "InterruptTest.h"
#include "Arduino.h"
#include "FunctionalInterrupt.h"
#include "Schedule.h"

class InterruptTest
{
public:
	InterruptTest(int pin) : _pin(pin)
	{
		Serial.begin(115200);
		attachInterrupt(pin,std::bind(&InterruptTest::isr_iram,this),FALLING);
	}

	void ICACHE_RAM_ATTR isr_iram()
	{
		schedule_function([this]() { Serial.printf("isr_iram from pin %d\r\n",_pin); });
	}

	volatile int _pin;
};

InterruptTest D5Handler(D5);
InterruptTest D4Handler(D4);

void setup()
{
}

void loop()
{
}

Would that be a valid use (together with #5922 ?)

@dok-net dok-net changed the title Follow-up to #6048 or #6038 : let FunctionalInterrupt.h support only IRAM-safe scheduled ISRs Completes #6048: let FunctionalInterrupt.h support only IRAM-safe scheduled ISRs May 12, 2019
@dok-net
Copy link
Contributor Author

dok-net commented May 13, 2019

@d-a-v This PR is more readable now (turn on diff settings: hide whitespace changes).
It is best reviewed as diff against https://github.com/dok-net/Arduino/tree/bugfix/attachInterrupt , or after PR #6048 was merged.

It should be less confusing now:

Bugfix first (#6048 ), then this PR.

OR:

As an alternative, that depends on the ICACHE_RAM dilemma surrounding the use of C++ functional solved in PR #5922, PR #6047 supersedes both #6048 AND this PR.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 26, 2019

Development of this branch stopped.
Cause: implementation based on std::function is more advanced, previous concerns involving IRAM-proofness have been addressed. Please go on with review/merge of #6047

@dok-net dok-net closed this Jun 26, 2019
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.

4 participants