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

Pluggable Scheduler: Make entry points into sequential cont/user scheduler weak functions #6182

Closed
wants to merge 14 commits into from

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented Jun 5, 2019

For sketches that don't use libraries depending on a user scheduler, this PR helps eliminate the Scheduler from the binaries, saving memory. To complete this, the Scheduler would have to be moved from core to the libraries.
Also, plugging different schedulers becomes more obvious to do.

Questions:

  • is there a way to make the concrete Scheduler used in Ticker or LEAmDNS easily pluggable?
  • are there different preferences for naming loop_completed() and yield_completed()?

@dok-net
Copy link
Contributor Author

dok-net commented Jun 5, 2019

Blink.ino before:
Program size: 257,740 bytes (used 25% of a 1,044,464 byte maximum) (1.99 secs)
Minimum Memory Usage: 26548 bytes (32% of a 81920 byte maximum)

After:
Program size: 257,356 bytes (used 25% of a 1,044,464 byte maximum) (22.41 secs)
Minimum Memory Usage: 26532 bytes (32% of a 81920 byte maximum)

@d-a-v @devyte Please review?

@dok-net
Copy link
Contributor Author

dok-net commented Jun 5, 2019

@hreintke What do you think? Could you please check the FunctionalInterrupt/library.properties?

@dok-net dok-net changed the title Make entry points into sequential cont/user scheduler weak functions Pluggable Scheduler: Make entry points into sequential cont/user scheduler weak functions Jun 6, 2019
@dok-net dok-net force-pushed the pluggable_sched branch 2 times, most recently from a6e61f3 to 9f22320 Compare June 10, 2019 22:48
@dok-net dok-net force-pushed the pluggable_sched branch 2 times, most recently from d3943cb to e7daf1e Compare June 21, 2019 11:47
@dok-net dok-net force-pushed the pluggable_sched branch 5 times, most recently from 97d82df to 56975b3 Compare June 25, 2019 09:38
@dok-net
Copy link
Contributor Author

dok-net commented Jun 25, 2019

@d-a-v @earlephilhower @devyte This is 3 weeks old, and I am sorry, had gotten out of control somehow, being a PR containing a lot of unrelated changes.
I've cleaned up now to the minimum changeset, and I would like to ask you again to review and consider merging, possibly ahead of #6214 , which I personally am having a bit of a hard time figuring out what the rationale is, nevermind.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 25, 2019

@d-a-v #6214 , why o' why...
The explanation isn't quite comprehensive yet. I disagree with the wording "long running operations or yield() or delay() are NOT WISE in the lambda." - should be either a yes or a no, nothing indeterminate.
I think it's a mistake not allowing one-shot scheduled functions to run on yield now - an async programming style produces much slower or un-responsive sketches this way. And this just because yields are not allowed? One-shot functions don't have to be slower than recurrent ones to contain yields - are the scheduled interrupts not hampered now for no good reason at all?

But of course my biggest gripe is that all my PRs need intensive care now 👎

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 25, 2019

I think it's a mistake not allowing one-shot scheduled functions to run on yield now - an async programming style produces much slower or un-responsive sketches this way.

I agree. I did what the majority told me.

In some other comment it was clearly stated that scheduled functions should be allowed to run delay() and yield(), plus the fifo semantic, plus once per loop. I did my homework. This doesn't fit my (ethernet) needs so I added the recurrent version.

You can however use recurrent scheduled function like you do with scheduled function with

  • return false
  • ::alwaysExpired polledTimeout parameter.
  • without fifo semantics

@dok-net
Copy link
Contributor Author

dok-net commented Jun 25, 2019

@d-a-v I'll see what I can do to the circular-queue based scheduler. First, I'd like to see the EspSoftwareSerial issue (#6227) through, the attachInterrupt fix/improvement is important (#6047 does away with #6055 and #6048 in one step), and getting this here pluggability into core would be awesome.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 25, 2019

@d-a-v

You can however use recurrent scheduled function like you do with scheduled function with

return false
::alwaysExpires polledTimeout parameter.
without fifo semantic

Oh wait a minute - is this a suggestion for scheduled ISRs? Doing those without FIFO semantics is completely wrong.

d-a-v added a commit to d-a-v/Arduino that referenced this pull request Jun 25, 2019
@d-a-v
Copy link
Collaborator

d-a-v commented Jun 25, 2019

It's not a suggestion for scheduled ISR, there are conditions if you wish to do that, and one of them is "without fifo semantics".

Oh wait a minute - is this a suggestion for scheduled ISRs? Doing those without FIFO semantics is completely wrong.

Now if you think scheduled ISR should be called also from yield, fifo semantics needs to be added to recurrent scheduled functions, which will increase overhead, which was an argument against the previous implementation.

(see? I do my homework)

@dok-net dok-net force-pushed the pluggable_sched branch 3 times, most recently from dcfc9a0 to f3d5c10 Compare June 26, 2019 06:51
@dok-net
Copy link
Contributor Author

dok-net commented Jun 26, 2019

@d-a-v What's in master now doesn't work, I am sorry. Regression test using f5a882d is OK.

ets Jan  8 2013,rst cause:4, boot mode:(3,6)

wdt reset
load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
vb20cfcf5
~ld

Just running ScheduledFunctional.ino , then clicking button 2:

#include <FunctionalInterrupt.h>

#ifndef IRAM_ATTR
#define IRAM_ATTR ICACHE_RAM_ATTR
#endif

#if defined(ESP32)
#define BUTTON1 16
#define BUTTON2 17
#elif defined(ARDUINO_ESP8266_WEMOS_D1MINI)
#define BUTTON1 D4
#define BUTTON2 D3
#else
#define BUTTON1 2
#define BUTTON2 0
#endif

class Button {
  public:
    Button(uint8_t reqPin) : PIN(reqPin) {
      pinMode(PIN, INPUT_PULLUP);
      attachScheduledInterrupt(PIN, [this](const InterruptInfo & ii) {
        Serial.print("Pin ");
        Serial.println(ii.pin);
        buttonIsr();
      }, FALLING); // works on ESP8266
    };
    ~Button() {
      detachInterrupt(PIN);
    }

    void IRAM_ATTR buttonIsr() {
      numberKeyPresses += 1;
      pressed = true;
    }

    static void IRAM_ATTR buttonIsr_static(Button* const self) {
      self->buttonIsr();
    }

    uint32_t checkPressed() {
      if (pressed) {
        Serial.printf("Button on pin %u has been pressed %u times\n", PIN, numberKeyPresses);
        pressed = false;
      }
      return numberKeyPresses;
    }

  private:
    const uint8_t PIN;
    volatile uint32_t numberKeyPresses = 0;
    volatile bool pressed = false;
};

Button* button1;
Button* button2;


void setup() {
  Serial.begin(115200);
  Serial.println("FunctionalInterrupt test/example");

  button1 = new Button(BUTTON1);
  button2 = new Button(BUTTON2);

  Serial.println("setup() complete");
}

void loop() {
  button1->checkPressed();
  if (nullptr != button2 && 10 < button2->checkPressed()) {
    delete button2;
    button2 = nullptr;
  }
}

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 26, 2019

What's in master now doesn't work, I am sorry.

That one was hairy. It will be fixed by #6233 (I hadn't retested after change from malloc to new to malloc).

@dok-net
Copy link
Contributor Author

dok-net commented Jun 26, 2019

@d-a-v Can you help me? Where's the discussion that led to implementing two separate lists for immediate and timed scheduled functions? Was there any proof of the efficiency of this? Is anybody in any way looking at my work with the lock-free data structure and using the timer for longer delays at all right now?

@dok-net dok-net force-pushed the pluggable_sched branch 5 times, most recently from 5712e7c to d937f46 Compare July 26, 2019 08:45
@dok-net dok-net force-pushed the pluggable_sched branch 3 times, most recently from c760644 to 669502b Compare August 6, 2019 22:08
@dok-net dok-net force-pushed the pluggable_sched branch 2 times, most recently from f7efedf to 71bab7c Compare September 4, 2019 10:30
@dok-net dok-net force-pushed the pluggable_sched branch 2 times, most recently from 9d6bc10 to bba72cc Compare September 14, 2019 07:35
@dok-net
Copy link
Contributor Author

dok-net commented Nov 15, 2019

Withdrawing this PR - existing Scheduler in core has received patches, works nicely, belongs in core.

@dok-net dok-net closed this Nov 15, 2019
@dok-net dok-net deleted the pluggable_sched branch November 15, 2019 15:12
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.

2 participants