Use safer & faster min/max functions #120
Conversation
Two issues here. First, the default min/max that this is dependening on uses min/max as a macro. That means the `min` call was actually evaluating `sqrt` and float cast twice, with a minor perf penalty (unless the optimizer catches it). Second, not all environments have min/max available in the global namespace (particularly platformio's native env). This avoids that implicit dependency. Therefore this fixes: ``` <path_here>/StepperDriver/src/BasicStepperDriver.cpp:141:25: error: use of undeclared identifier 'min'; did you mean 'fmin'? ``` The original purpose of this diff was the second issue, but researching other people running into this made me realize the first issue. So I went down a different fix. Another fix is just use native c++'s std::min and std::max, but that gets into various arduino-cli related compiler complications.
richievos
changed the title
|
Sorry for the multiple force-pushes, took me a couple tries to figure out how to run this locally and on my own PR fork. I updated the commits to get the build passing (ran them on my fork richievos#2), and I also found there's a minor potential perf boost to an alternative solution, so I did that too. |
richievos
changed the title
|
|
||
| /* | ||
| * Min/Max functions which avoid evaluating the arguments multiple times. | ||
| * See also https://github.com/arduino/Arduino/issues/2069 |
There was a problem hiding this comment.
Thank you for this link explaining the issue
laurb9
left a comment
laurb9
left a comment
There was a problem hiding this comment.
Thank you for this fix. I am curious about the use case for native, I imagine this code runs choppy on a Pi or similar
You're welcome. I'm using native mode to run my unit tests without having to have my ESP32/Arduino/... connected. I am not invoking the StepperDriver code in that setup, but I currently do have it as a library dependency to successfully build my tests. |
2 participants
Two issues here. First, the default min/max that this is dependening on uses
min/max as a macro. That means the
mincall was actually evaluatingsqrtand float cast twice, with a minor perf penalty (unless the optimizer catches
it).
Second, not all environments have min/max available in the
global namespace (particularly platformio's native env). This avoids that
implicit dependency. Therefore this fixes:
The original purpose of this diff was the second issue, but researching other
people running into this made me realize the first issue. So I went down a
different fix.
Another fix is just use native c++'s std::min and std::max, but that gets into
various arduino-cli related compiler complications.