-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Make F_CPU a compile time constant (and not a runtime constant). #21051
Make F_CPU a compile time constant (and not a runtime constant). #21051
Conversation
… fixes DELAY_NS for small delays
d7106c7
to
5c6b272
Compare
It’s better to use -post.py and projenv. The pre script runs before the platform script and the post run after. So, to be safe and better, you should move the code as I said originally. |
It does not work for dependencies (typically: NeoPixels). The projenv is ignored for building dependencies. I have to change the buildflags for both marlin's code and dependencies. |
As a side note, I'm not aware of any other dependency that's back including Marlin's code like Neopixels. And Neopixels code is just a bunch of spaghetti mess, so if we could get rid of this sh.t, I'd be more than happy to move to a |
So add it to env and projenv. But the post is the better/safer place to run it. It may work on pre, but I already had problem with some platform replacing our changes. |
Will try... |
It does not work:
Building Marlin's code is working:
but it's completely ignored for Neopixels:
I don't know platformio internal well enough to figure out if the common-dependencies-post is even used for external libraries |
Ok, don’t worry. If pre is working, keep it. With time I will take a look. Using build_flags may work on pre without issues. Only CPPDEFINES could be a issue. |
#ifdef BOARD_F_CPU | ||
#undef F_CPU | ||
#define F_CPU BOARD_F_CPU | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this matters, but this hack will cause any external headers that use F_CPU
and follow the include of MarlinConfigPre.h
to have a different value of F_CPU
than their corresponding .cpp
files. This could have hard-to-diagnose side effects in current or future external libraries used by Marlin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly a side effect I'm looking for. External libraries will be redirected to a compile time version of the frequency, instead of a runtime version. That's very important to reach the last few level of optimization (for DELAY_NS typically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
External libraries won't see the F_CPU
value set here. They will only see the F_CPU
value set in the headers that they include. Do we expect all external libraries to use BOARD_F_CPU
instead of F_CPU
when BOARD_F_CPU
is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one of the most painful external library is NeoPixel and it includes Delay.h from Marlin. This, in turns, includes Marlin_pre_config.h which will swap F_CPU for the compile time version. I would expect that, either the library does not make use of very small delay and in that case, using either F_CPU will work. Either the library requires small delay, and it'll likely need the declaration of DELAY_NS (via Delay.h) and in that case, F_CPU will be swapped to the compile time version and the DELAY_NS macro to convert to cycles will be compile time computed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one of the most painful external library is NeoPixel and it includes
Delay.h
from Marlin.
When did that happen? If a library really needs some custom code to be optimally compiled inline without requiring any function calls, it should be provided as a template argument during instantiation. If the NeoPixel library is checking for __MARLIN_FIRMWARE__
and including code from Marlin, that's quite a brittle dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was already discussed somewhere. @sjasonsmith may remember about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that is in SAMD51 framework...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the best idea is to just include our own NeoPixel driver. Let's make it so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. I think this block can probably go into pins_postprocess.h
since it pertains to a setting expected to come from one of the pins files (if not a compile-time define).
It seems to work with |
Description
On STSTM32 framework, F_CPU is defined to a runtime value that's modified on boot. Because of this, the
DELAY_NS
code that's converting the delay argument in nanosecond is not computed at compile time but at runtime, resulting in 2 divide plus a multiply being emitted per call toDELAY_NS
.For small delay (below 300ns), this overhead completely breaks the expected delay duration. For larger delay, it add a large overhead to the delay duration.
So, since we have the information of the expected CPU frequency in board's JSON file, while building Marlin, let's pass this information down to the compile unit and redefine the
F_CPU
macro to a compile time constant.Requirements
STM32 CPU and building with ststm32 framework (not libmaple, it works directly in libmaple)
Benefits
Delay down to a single cycle are now possible on STSTM32 framework, so this should fix the library that makes use of this code, like NeoPixels.
Configurations
N/A
Related Issues
#20982 #21048