ellensp
commented
3 years ago Please attach your configuration files. Also any other relevant modified files
Open deeplymbedded opened 3 years ago
ellensp
commented
3 years ago Please attach your configuration files. Also any other relevant modified files
deeplymbedded
commented
3 years ago Configuration.txt Configuration_adv.txt pins_RAMPS.h.txt
Files attached
ellensp
commented
3 years ago Your motherboard BOARD_RAMPS4DUE_EFB uses pins_RAMPS4DUE.h not pins_RAMPS.h
Compiles fine on platformio with default pins_RAMPS4DUE.h
deeplymbedded
commented
3 years ago Yes but pins_RAMPS.h is included in pins_RAMPS4DUE.h
I use Arduino IDE 1.8.5 where the problem occurs.
ellensp
commented
3 years ago using arduino ide 1.8.13 does produce same error (without all the extra warnings)
In file included from /tmp/arduino_build_429719/sketch/src/HAL/DUE/HAL_SPI.cpp:41:0:
/tmp/arduino_build_429719/sketch/src/HAL/DUE/HAL_SPI.cpp: In function 'uint8_t spiTransferX(uint8_t)':
/tmp/arduino_build_429719/sketch/src/HAL/DUE/../shared/Delay.h:93:83: error: the value of 'spiDelayCyclesX4' is not usable in a constant expression
#define DELAY_CYCLES(X) do { SmartDelay<IS_CONSTEXPR(X), IS_CONSTEXPR(X) ? X : 0> _smrtdly_X(X); } while(0)
^
/tmp/arduino_build_429719/sketch/src/HAL/DUE/HAL_SPI.cpp:252:7: note: in expansion of macro 'DELAY_CYCLES'
DELAY_CYCLES(spiDelayCyclesX4);
^
/tmp/arduino_build_429719/sketch/src/HAL/DUE/HAL_SPI.cpp:243:19: note: 'uint32_t spiDelayCyclesX4' is not const
static uint32_t spiDelayCyclesX4 = 4 * (F_CPU) / 1000000; // 4µs => 125khz
^
To get rid of the warnings change pins_RAMPS.h
//
// SPI for Max6675 or Max31855 Thermocouple
//
#ifndef MAX6675_SS_PIN
#define MAX6675_SS_PIN 66 // Don't use 53 if using Display/SD card (SDSS) or 49 (SD_DETECT_PIN)
#endif
to
//
// SPI for MAX Thermocouple
//
#ifndef TEMP_0_CS_PIN
#define TEMP_0_CS_PIN 66 // Don't use 53 if using Display/SD card (SDSS) or 49 (SD_DETECT_PIN)
#endif
slowbro
commented
2 years ago I don't see a reason why HAL_SPI in DUE should be using cycles - all of the calculations it's doing is for microseconds. Also, in spiInit it seems to be feeding US into DELAY_CYCLES anyways... I'll submit a PR with a fix.
thinkyhead
commented
2 years ago With the patch merged, DELAY_NS now gets a simple numerical value. The original issue stems from the additional complexity introduced in #20901 by @X-Ryl669, so those changes could use some review to make sure they are not causing other things.
ellensp
commented
2 years ago this has not solved the issue under arduino IDE the error is now
In file included from /tmp/arduino_build_559606/sketch/src/HAL/DUE/HAL_SPI.cpp:41:0:
/tmp/arduino_build_559606/sketch/src/HAL/DUE/HAL_SPI.cpp: In function 'uint8_t spiTransferX(uint8_t)':
/tmp/arduino_build_559606/sketch/src/HAL/DUE/../shared/Delay.h:93:83: error: the value of 'spiDelayNS' is not usable in a constant expression
#define DELAY_CYCLES(X) do { SmartDelay<IS_CONSTEXPR(X), IS_CONSTEXPR(X) ? X : 0> _smrtdly_X(X); } while(0)
^
/tmp/arduino_build_559606/sketch/src/HAL/DUE/../shared/Delay.h:207:23: note: in expansion of macro 'DELAY_CYCLES'
#define DELAY_NS(x) DELAY_CYCLES(((x) * ((F_CPU) / 1000000UL) + 999) / 1000UL) // "ceil"
^
/tmp/arduino_build_559606/sketch/src/HAL/DUE/HAL_SPI.cpp:252:7: note: in expansion of macro 'DELAY_NS'
DELAY_NS(spiDelayNS);
^
/tmp/arduino_build_559606/sketch/src/HAL/DUE/HAL_SPI.cpp:243:19: note: 'uint16_t spiDelayNS' is not const
static uint16_t spiDelayNS = 4000; // 4000ns => 125khz
^
arduino IDE
Ah, I missed that part. I'll give it another shot and check out #20901 as well; from what I saw DELAY_CYCLES should allow using a non-const (and seems to work ok w/ PIO) but maybe it can just all be simplified.
thinkyhead
commented
2 years ago …the error is now…
Makes sense. The thing about these tiny delays is that they really want to be inline static. If this delay can be done in micro-seconds then it will be okay to use a variable.
slowbro
commented
2 years ago I think the templating/specialization in Delay.h is over my head. I'm getting the gist of it, but I don't quite understand how the "fallback"/runtime version ever gets called. Seemingly, in Arduino IDE, it isn't...
Can someone help demystify this..? It seems like no matter what it's going to end up calling SmartDelay<bool, int> which matches the first template. In my messing around, too, I found it difficult to get templates to understand the difference between true/false (1/0) and an int..
rhapsodyv
commented
2 years ago What would be the drawback to have a simple and normal function without all those bizarre advanced programming techniques that only a few persons can change? If the cost is minimal, I think we should just simplify the code...
slowbro
commented
2 years ago Agreed, though I see why the complexity was added, in part. Since you can only use constexpr in template arguments, it makes the SmartDelay function also require a template. I was able to simplify it, and it compiles with PIO and Arduino, though I'm not 100% certain it works...
diff --git a/Marlin/src/HAL/shared/Delay.h b/Marlin/src/HAL/shared/Delay.h
index 3174968c1b..a291f9837e 100644
--- a/Marlin/src/HAL/shared/Delay.h
+++ b/Marlin/src/HAL/shared/Delay.h
@@ -73,24 +73,21 @@ void calibrate_delay_loop();
};
}
+
// Select a behavior based on the constexpr'ness of the parameter
// If called with a compile-time parameter, then write as many NOP as required to reach the asked cycle count
// (there is some tripping point here to start looping when it's more profitable than gruntly executing NOPs)
// If not called from a compile-time parameter, fallback to a runtime loop counting version instead
- template <bool compileTime, int Cycles>
- struct SmartDelay {
- FORCE_INLINE SmartDelay(int) {
+ FORCE_INLINE static void DELAY_CYCLES(int Cycles) {
+ DelayCycleFnc(Cycles);
+ }
+
+ template <int Cycles>
+ FORCE_INLINE static void DELAY_CYCLES() {
if (Cycles == 0) return;
Private::Helper<Cycles < TRIP_POINT_FOR_CALLING_FUNCTION, Cycles>::build();
- }
- };
- // Runtime version below. There is no way this would run under less than ~TRIP_POINT_FOR_CALLING_FUNCTION cycles
- template <int T>
- struct SmartDelay<false, T> {
- FORCE_INLINE SmartDelay(int v) { DelayCycleFnc(v); }
};
- #define DELAY_CYCLES(X) do { SmartDelay<IS_CONSTEXPR(X), IS_CONSTEXPR(X) ? X : 0> _smrtdly_X(X); } while(0)
// For delay in microseconds, no smart delay selection is required, directly call the delay function
// Teensy compiler is too old and does not accept smart delay compile-time / run-time selection correctlyI didn't look closely the code yet. But why it's so important to have a constexpr version of it? why not just a runtime version and just be happy? The runtime version gives too wrong results? Even more... if we need a compile-time and a runtime version, why not just define two functions? This is simple and easy.
slowbro
commented
2 years ago But why it's so important to have a constexpr version of it?
Optimization, I would assume. Why have the MCU do the math/additional ops at runtime if it can be pre-generated.
Even more... if we need a compile-time and a runtime version, why not just define two functions?
Personally, I would rather not have to remember to use DELAY_CYCLES() or DELAY_CYCLES_CONSTEXPR(). With the way that constants are checked (__builtin_constant_p) it can only be done at compile time, after the pre-processor, so it makes switching the function out in a macro difficult and/or impossible. I tried overloading DELAY_CYCLES with int and const int but that's not allowed; I also tried overloading with bool and int but that is ambiguous as noted above.
Honestly, though, I'm not sure how much the new NOP generator code is really needed.... I don't understand why this couldn't be sorted like AVR is; I suppose it was before, but was not accurate. I would think we could just adjust for that, no...?
@X-Ryl669 do you have any input here?
rhapsodyv
commented
2 years ago But why it's so important to have a constexpr version of it?
Optimization, I would assume. Why have the MCU do the math/additional ops at runtime if it can be pre-generated.
What is the difference to waste time with math than to waste time with nops? The only important thing is to waste the most accurate time that was asked.
Even more... if we need a compile-time and a runtime version, why not just define two functions?
Personally, I would rather not have to remember to use DELAY_CYCLES() or DELAY_CYCLES_CONSTEXPR().
For me, this is the best way. We always use DELAY_CYCLES, and optimize with DELAY_CYCLES_CONSTEXPR... Or name the opposite: DELAY_CYCLES_RUNTIME and DELAY_CYCLES, than DELAY_CYCLES would fail (we can do it?) if not used with a compile constant... and the dev knows that he need to use DELAY_CYCLES_RUNTIME.
Just to think about it: the original DELAY_CYCLES was wrong by over 50% more and it lived in Marlin for years with no or minimal complains, so why we need to put so much effort to make a perfect version of it? Almost all delays usages don't need to be so precise like that... and when we need the super precision, we can handle case by case, because that would be one or two cases only.
Simple and easy. And all this time and effort can be put in another tasks...
slowbro
commented
2 years ago For me, this is the best way. We always use DELAY_CYCLES, and optimize with DELAY_CYCLES_CONSTEXPR... Or name the opposite: DELAY_CYCLES_RUNTIME and DELAY_CYCLES, than DELAY_CYCLES would fail (we can do it?) if not used with a compile constant... and the dev knows that he need to use DELAY_CYCLES_RUNTIME.
I do like this idea. We can likely make it something that would fail "nicely" too versus the explosion of macro expansions.
Just to think about it: the original DELAY_CYCLES was wrong by over 50% more and it lived in Marlin for years with no or minimal complains, so why we need to put so much effort to make a perfect version of it?
And all this time and effort can be put in another tasks...
Agreed, there are a lot more interesting things I'd personally rather be spending my time on :wink:
X-Ryl669
commented
2 years ago The initial code was not only 50% off, it was also impossible to reach small delays. For the explanation of the current code, see here.
Basically, it's impossible to reach nanosecond delays with a dynamic variable because the simple fact of converting from a nanosecond to the CPU's time unit takes more than the waiting time (off by a order of magnitude). Thus, the idea here is to have the compiler compute the CPU's delay by itself and emit as many nop as required to fit the requirement. This is absolutely required for Neopixels (for one).
On Arduino IDE, their compiler is so old it does not compile code more recent than ten years ago (C++ 2011).
Anyway, the initial code was doing something like:
switch(delay / CPU_CLOCK)
{
...
case 2: nop;
case 1: nop;
case 0: nop;
....
}
// No break anywhereThus, the code was limited to only a few cases (self unrolled), and reaching higher delay because of different architecture means higher value. Also, due to different processor pipeline, the code above is using comparison that are not CPU clock constant (between ARM M3 and M0) so you can't have a true delay in cycles unless you put #ifdef everywhere.
slowbro
commented
2 years ago @X-Ryl669 could you take a look at the bug in this topic and see why it's not compiling with the Arduino IDE?
X-Ryl669
commented
2 years ago I'll take a look, but I won't promise I'll be able to solve it, since I don't have/use the Arduino IDE. As far as I understand, your proposition is probably the right solution here. Instead of using the compiler to find out if an expression is constant at compile time, let the developer specify it (so the code that read:
template <bool compileTime, int Cycles>
struct SmartDelay {
FORCE_INLINE SmartDelay(int) {
if (Cycles == 0) return;
Private::Helper<Cycles < TRIP_POINT_FOR_CALLING_FUNCTION, Cycles>::build();
}
};
// Runtime version below. There is no way this would run under less than ~TRIP_POINT_FOR_CALLING_FUNCTION cycles
template <int T>
struct SmartDelay<false, T> {
FORCE_INLINE SmartDelay(int v) { DelayCycleFnc(v); }
};
#define DELAY_CYCLES(X) do { SmartDelay<IS_CONSTEXPR(X), IS_CONSTEXPR(X) ? X : 0> _smrtdly_X(X); } while(0) could read:
template <int Cycles>
struct SmartDelay {
FORCE_INLINE SmartDelay(int) {
if (Cycles == 0) return;
Private::Helper<Cycles < TRIP_POINT_FOR_CALLING_FUNCTION, Cycles>::build();
}
};
#define DELAY_CYCLES(X) do { SmartDelay<X> _smrtdly_X(X); } while(0)
#define DELAY_CYCLES_RUNTIME(X) DelayCycleFnc(X)
For your interest, the template <bool, int> and template <int> stuff is simple to understand once explained. The former says the following structure depends on a boolean (that can only be true or false) and an integer (the number of cycles).
The latter is a specialization of the former where the boolean value is false (see SmartDelay<false> part). So, if the boolean is true, the "general" version is used, and if it's false, the specialized version is used (which, in my code is when the variable is not a constexpr).
The int part is a recursive counter, SmartDelay<3> will instantiate SmartDelay<2> and so on until SmartDelay<0> where the recursion stops. Each instantiation contains a nop, that is, a cycle. Thus, SmartDelaynop (and burn X cycles) which is exactly what we are looking for...
thinkyhead
commented
2 years ago I look forward to seeing if those tweaks do the trick! I don't use Arduino IDE either, but I will see if I can dig up a recent version and give this a test at some point soon.
slowbro
commented
2 years ago I do agree there is simplicity and sanity to the _RUNTIME addition, but for very little additional complexity, we can have a DELAY_CYCLES that does the selection automatically. If we already have to have the template ... SmartDelay then there's not much difference here.
FORCE_INLINE static void DELAY_CYCLES(int Cycles) {
DelayCycleFnc(Cycles);
}
template <int Cycles>
FORCE_INLINE static void DELAY_CYCLES() {
if (Cycles == 0) return;
Private::Helper<Cycles < TRIP_POINT_FOR_CALLING_FUNCTION, Cycles>::build();
};
As far as I can tell type specialization is working fine with our compilers. So we just need some equivalent for this that works with C++11 or some guarantee that our tools are all compiling C++14 properly…
// Only valid solution with C++14. Should use std::is_constant_evaluated() in C++20 instead
#define IS_CONSTEXPR(...) __builtin_constant_p(__VA_ARGS__)If some tools are already leaping to C++20 then we may need to add a condition and switch to is_constant_evaluated.
Did you test the latest
bugfix-2.0.xcode?Yes, and the problem still exists.
Bug Description
When compiling Marlin from both the 2.0.x and the bugfix-2-0-x branch the compilation fails with the following error:
Log Output
``` In file included from sketch/src/inc/MarlinConfig.h:49:0, from sketch/src/MarlinCore.h:24, from sketch/src/MarlinCore.cpp:31: sketch/src/inc/SanityCheck.h:423:4: warning: #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." [-Wcpp] #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." ^ In file included from sketch/src/module/printcounter.h:25:0, from sketch/src/MarlinCore.cpp:53: sketch/src/module/../libs/duration_t.h:111:36: warning: unknown option after '#pragma GCC diagnostic' kind [-Wpragmas] #pragma GCC diagnostic ignored "-Wformat-overflow" ^ In file included from sketch/src/HAL/DUE/../../inc/MarlinConfig.h:49:0, from sketch/src/HAL/DUE/HAL.cpp:27: sketch/src/HAL/DUE/../../inc/SanityCheck.h:423:4: warning: #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." [-Wcpp] #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." ^ In file included from sketch/src/HAL/DUE/../../inc/MarlinConfig.h:49:0, from sketch/src/HAL/DUE/HAL_SPI.cpp:40: sketch/src/HAL/DUE/../../inc/SanityCheck.h:423:4: warning: #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." [-Wcpp] #warning "MAX*_SS_PIN, MAX*_SS2_PIN, MAX*_CS_PIN, and MAX*_CS2_PIN are deprecated and will be removed in a future version. Please use TEMP_0_CS_PIN/TEMP_1_CS_PIN instead." ^ In file included from sketch/src/HAL/DUE/HAL_SPI.cpp:41:0: sketch/src/HAL/DUE/HAL_SPI.cpp: In function 'uint8_t spiTransferX(uint8_t)': sketch/src/HAL/DUE/../shared/Delay.h:93:83: error: the value of 'spiDelayCyclesX4' is not usable in a constant expression #define DELAY_CYCLES(X) do { SmartDelayAfter digging into the code I found out that the problem is the usage of the MACRO
define DELAY_CYCLES(X) do { SmartDelay<IS_CONSTEXPR(X), IS_CONSTEXPR(X) ? X : 0> _smrtdly_X(X); } while(0)
in the file Delay.h in conjunction with the bad definition of static uint32_t spiDelayCyclesX4 = 4 * (F_CPU) / 1000000; // 4µs => 125khz in HAL_SPI.cpp
Changing the variable definition to static const uint32_t spiDelayCyclesX4 = 4 * (F_CPU) / 1000000; // 4µs => 125khz does solve above error but gives a new one because spiDelayCyclesX4 is tryed to by overridden in void spiInit(uint8_t spiRate) in the default case of switch/case instuction. Commenting this line out spiDelayCyclesX4 = ((F_CPU) / 1000000) >> (6 - spiRate) << 2; // spiRate of 2 gives the maximum error with current CPU
makes the build finally go through successfully.
Bug Timeline
New
Expected behavior
Build shall work flawless for Arduino DUE board
Actual behavior
Build is breaking, see description above
Steps to Reproduce
Version of Marlin Firmware
2.0.x, bugfix-2.0.x
Printer model
CoreXY
Electronics
Arduino DUE with RAMPS
Add-ons
No response
Your Slicer
Simplify3D
Host Software
SD Card (headless)
Additional information & file uploads
No response