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

avoid circular #include dependence for PolledTimeout #7356

Merged
merged 19 commits into from
Aug 15, 2020

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Jun 7, 2020

edit / change:
This PR removes #include <Arduino.h> from PolledTimeout.h thus avoid circular dependencies in some cases.
(cf. last line from OP below)
Title is changed accordingly.

OP:
Using delay() in the main loop is not efficient in the esp8266 arduino core.
PolledTimeout has beed added a while ago to help users avoiding it.

PolledTimeout is not included by default and being hidden in a deep namespace hierarchy makes its access difficult to the average arduino user.

This PR brings it at top level. It is now easier for a user to understand and remember how to transform

void loop ()
{
   handleWebServer();
   doSomething();
   //delay(1000); // breaks webserver!  <= wrong
   delay(1000); // reduces webserver performances!
}

to

void loop ()
{
    handleWebServer();

    static periodicMs doItNow(1000);
    if (doItNow) {
        doSomething();
    }
}

or

periodicMs doItNow(1000);

void loop ()
{
    handleWebServer();
    if (doItNow) {
        doSomething();
    }
}

All other useful classes in PolledTimeout are also available.
Documentation for PolledTimeoud is in an example (updated in this PR)

Also adding ::resetAndSetExpired([period]) to mark a periodic to be triggered at next check, and optionally change its period.

All other changes are due to circular inclusions with Arduino.h.

@d-a-v d-a-v added this to the 3.0.0 milestone Jun 7, 2020
@d-a-v d-a-v changed the title Brings PolledTimeout globally next to millis() Bringing PolledTimeout globally next to millis() Jun 10, 2020
@dok-net
Copy link
Contributor

dok-net commented Jun 12, 2020

I see multiple issues with this PR, in that it works against PRs from me - either those still pending, or by undoing performance improvements in already merges PRs.
Simply speaking, this PR suffers a separation-of-concern failure, that makes it kind of hard to address in short time.

@dok-net
Copy link
Contributor

dok-net commented Jun 12, 2020

To start off, though, PolledTimeout is good for stackless coroutine programming, but replacing delay(), intended in Arduino to be shimmed by multitasking libraries, prominently here from my POV the CoopTask implementation, breaks that cooperative multitasking model of stackful coroutines.
That said, running with CoopTask, the code sample

void loop ()
{
   handleWebServer();
   doSomething();
   delay(1000); // breaks webserver!
}

incorrectly claims that delay() breaks the webserver, because the webserver runs just fine as a CoopTask task.
Please also remember that the work done for Scheduler.h|cpp allows coroutines to run during delay() suspensions.

@dok-net
Copy link
Contributor

dok-net commented Jun 12, 2020

The change from clockCyclesPerMicrosecond() to esp_get_cpu_freq_mhz(), OTTOH, disables the compile-time constexpr work I've done. I think this is merged already. Likewise, the relocating of millis() etc. may be a serious merge obstacle to long standing pending PRs.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jun 12, 2020

Dear dok-net, I'm sorry to know that you see so many issues in this trivial PR.

it works against PRs from me by undoing performance improvements in already merged PRs

Can you name them and show how performances are degraded ?

[the example] incorrectly claims that delay() breaks the webserver, because the webserver runs just fine as a CoopTask task.

Lots of snippets seen in issues of this repository use that kind of delay in the main loop, including examples (that also need to be fixed). The delay() as shown above reduces webserver performances (which are already low) when cooptask library is not used, which is the default. This is then not an incorrect claim - I can change to "breaks webserver peformances!" if it can help with your interpretation.

The change from clockCyclesPerMicrosecond() to esp_get_cpu_freq_mhz()

That is a rename I can make in this PR, not a problem.

Likewise, the relocating of millis() etc. may be a serious merge obstacle to long standing pending PRs.

This is a supposition from your part. millis() was in Arduino.h, now in features.h included by PolledTimeout.h included by Esp.h included by Arduino.h.
I'm using that change from a long time in my work, including by using third-party libraries. Not saying it is perfect though. And if millis() not directly in Arduino.h is really an issue, that can be easily solved.

This PR suffers a separation-of-concern failure that makes it kind of hard to address in short time.

It is targetted for version 3.0.0 so there is plenty of time. You seem to see a general failure or much complexity where this PR simply adds an include file and imports a global conflictless namespace. I'm sorry to admit that I have a general failure in trying to understand what "separation-of-concern failure" means here.

You are free to explain to not use polledTimeout in your library's documentation. But you cannot claim that polledTimeout is counter-productive nor not needed in the provided example in OP. It was also originally made for that purpose.

@dok-net
Copy link
Contributor

dok-net commented Jun 12, 2020

Dear @d-a-v, I have no strong feelings about the central change underlying this PR:

using namespace esp8266::polledTimeout;

What does cause me some concern, though, is that there are a handful of changes in this PR, that are unrelated to that namespace matter. That is what I refer to as separation of concern issue. If you could split it into individual PRs, it would make it easier to discuss the issues.

To get you started on an obvious problem, I think there's an undetected compile-time failure caused by one of the changes. Where you add an implementation of uint8_t getCpuFreqMHz() in cores/esp8266/Esp.h, you're ignoring the existing implementation in Esp.cpp.

@dok-net
Copy link
Contributor

dok-net commented Jun 13, 2020

Furthermore, the example code change you give in this PR, labelled as "transformation", is in fact a completely different style of programming for all except the most trivial "Hello world" program code. I'm in no way passing judgement on whether commonplace AVR Arduino-like code using delay(), stateful/stackful coroutines using delay() in multiple tasks (loop-like functions) running off an AVR Arduino-compatible MT extension, or stackless coroutines running either off PolledTimeout or Schedule.h are "better suited" in any conceivable use case. It's just that this cannot be described as a transformation to unsuspecting users, and it's also a portability issue.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jun 13, 2020

@dok-net I'm sure I have seen this or even done this on AVR before, not you ?

    // declaration: unsigned long for lastTime and intervalMs
    if (millis() - lastTime > intervalMs) {
        lastTime += intervalMs;
        digitalWrite(LED_BUILTIN, 1 - digitalRead(LED_BUILTIN));
    }

That makes me think that we could port the polledTimeout library to be arduino compatible on all architectures, so we could on a pro mini

    // declaration: periodicMs doItNow(intervalMs);
    if (doItNow) {
        digitalWrite(LED_BUILTIN, 1 - digitalRead(LED_BUILTIN));
    }

What do you think ?
It allows to easily make two leds blink at different frequencies compared to the usage of delay(), and with only one stack. Nice no ?

@dok-net
Copy link
Contributor

dok-net commented Jun 13, 2020

@d-a-v No objections, just note that this never had anything to do with delay in the first place. It's a different programming paradigm.

@TD-er
Copy link
Contributor

TD-er commented Jul 1, 2020

What's striking me in this PR is that it is calling using namespace in a .h file.
I get that's more or less the intent of this PR, but in general calling using namespace in a .h file makes the success of compiling depending on the order of includes which in general is a bad idea.
What's wrong with adding the namespace as a prefix to the objects you try to create? If it is too much of a hassle, then call using namespace in the .cpp file or for that matter in the .ino

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jul 1, 2020

polledTimeout is a helper, in that sense it is not strictly necessary.
Its namespace is unnecessary long and difficult to remember on a everyday use basis.

The goal of this PR is to make it easy to use, as presented in the OP.

@TD-er
Copy link
Contributor

TD-er commented Jul 1, 2020

I can understand why it may be a good thing, so I won't object as long as you do consider the implications of calling using namespace in a .h file.
It will be included in Arduino.h so it is somewhat less likely it will cause build issues, but it might.

Still there may be name conflicts if some function has the same name (or function signature) in existing projects which also occur in this name space.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jul 1, 2020

Yes, there is a risk of a name collision with {periodic,oneShot}{Ms,Fast{Ms,Us,Ns}}, but I'm confident.

If the using this-quite-unique-namespace is too much bothering, the PR can be changed to bring the above class names to global, and to not break backward compatibility the namespace can be recreated when #including PolledTImeout.h.

@devyte devyte self-requested a review July 1, 2020 13:25
@d-a-v d-a-v added the alpha included in alpha release label Jul 16, 2020
@d-a-v d-a-v changed the title Bringing PolledTimeout globally next to millis() avoid circular #include dependence for PolledTimeout Aug 10, 2020
cores/esp8266/Esp.h Outdated Show resolved Hide resolved
cores/esp8266/PolledTimeout.h Show resolved Hide resolved
cores/esp8266/PolledTimeout.h Show resolved Hide resolved
cores/esp8266/core_esp8266_features.h Show resolved Hide resolved
cores/esp8266/PolledTimeout.h Show resolved Hide resolved
@d-a-v
Copy link
Collaborator Author

d-a-v commented Aug 15, 2020

@devyte I'll let you verify changes following your review.
I didn't split core_esp8266_features.h into another core_esp8266_timing.h, I let that for another distinct commit.

@devyte devyte merged commit e0fedc5 into esp8266:master Aug 15, 2020
@d-a-v d-a-v removed the alpha included in alpha release label Aug 27, 2020
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