Skip to content

Minimize header use, move Ticker function definitions into cpp file #6496

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

Merged
merged 10 commits into from
Nov 14, 2019

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented Sep 9, 2019

This is a corresponding port of ESP32 review results.

@dok-net dok-net force-pushed the ticker branch 3 times, most recently from d7051cd to 9b6eb9e Compare September 18, 2019 09:38
@dok-net dok-net force-pushed the ticker branch 3 times, most recently from e41e673 to c85b86f Compare October 1, 2019 21:06
@devyte devyte self-requested a review October 7, 2019 23:31
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

In the current code, all methods that take a "seconds" arg are routed through a single _attach_s() which is where the float multiplication is done. This is to have the multiplication in a single place, because on the ESP float arithmetic is done in sw.
In other words, the current code pays the price of an additional forwarder method to reduce the duplication of the generated code for the float multiplication.
On the other hand, you moved the methods out of the header.
Please check size of the bins for current code vs. this PR for a case that uses many of the methods.

@dok-net
Copy link
Contributor Author

dok-net commented Oct 11, 2019

@devyte Are you certain that float multiplication is even inlined?
I think the changes I've now made reduce ROM usage across the board - one ticker, two tickers, same mode, two different modes (attach_ms_scheduled, once).

@devyte
Copy link
Collaborator

devyte commented Oct 15, 2019

@dok-net I see you reverted the move from h to cpp. Did you compare the bin size lf your current proposal vs. unifying the float mult in a single attach method as is currently the case?

@dok-net
Copy link
Contributor Author

dok-net commented Oct 15, 2019

@devyte My comment in the ESP32 PR was more informative: better than 204 bytes saved by inlining in header

As to your precise question, I don't see the issue you're hinting at, as far as I can tell from objdump either, the multiplication is not unrolled/inlined, therefore it's rather insignificant in how many places a 1000UL * x or 1000000UL * x expression exists. This is from extensive trying out all sorts of combinations of inline vs. cpp impl, nested functions vs. copy&paste. The version I have in this PR is the only one that turned out well.

That said, I don't expect any single sketch to use all modes of the Ticker at once, YMMV if you do. But for using two Ticker instances, one with attach_ms, the other with once_scheduled, this is what I base my findings on and that's what this PR is quite good at.

@devyte
Copy link
Collaborator

devyte commented Oct 15, 2019

@dok-net the relevant code is in effect only when using attach() with a float arg.

@dok-net
Copy link
Contributor Author

dok-net commented Oct 15, 2019

@devyte Naturally, I've tested all sort of combinations, also on ESP32, where attach_ms() scales to µs (1000ULL *) :-)

@dok-net
Copy link
Contributor Author

dok-net commented Oct 15, 2019

@devyte CI is broken - not by me (?)

@earlephilhower
Copy link
Collaborator

The pleasures of DevOps and needing sources from 15 different projects. The Python version in the Win VM was changed, and installed into a different dir than before. Fixed now, hopefully, permanently.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Ticker.cpp was moved, so the diff is visible.
ticker.h was not moved, it was (D)eleted in one place and (A)dded in the new one. Therefore, the diff can't be seen here in the PR, and instead the file is fully red in the origin and fully green in the destination.
Please fix that, or redo the PR, so that I can review.

@dok-net
Copy link
Contributor Author

dok-net commented Nov 6, 2019

@devyte It's not on purpose, git doesn't recognize it as a move, the internal difference probably is to large. Could you do a local diff by hand, please?

@dok-net dok-net force-pushed the ticker branch 5 times, most recently from 0d0a604 to d227aaa Compare November 13, 2019 08:39
@devyte devyte self-assigned this Nov 14, 2019
@devyte devyte added this to the 2.7.0 milestone Nov 14, 2019
@devyte devyte modified the milestones: 2.7.0, 2.6.1 Nov 14, 2019
@devyte devyte merged commit 482516e into esp8266:master Nov 14, 2019
@dok-net dok-net deleted the ticker branch November 14, 2019 13:08
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