Comments (5)
I welcome suggestions and discussions for improvements. Unfortunately, the one example you have suggested is not a fix or improvement. The reason it is preferable to pass large objects by reference is because for a normal function call, the whole object would need to be copied to the stack. If the object were instead passed by reference then then only a pointer would need to be copied to the stack. This would consume less of the stack and achieve faster execution.
The reason this is not a fix or improvement here is because the function is declared static inline
within the header file. The function will therefore be implemented inline for each call so that no data is copied to the stack. Furthermore, the inline implementation used here and for all functions in FusionMath.h enables significant efficiencies to be achieved by the optimiser.
For example, consider the following function call. The optimiser will reduce this to 3 floating-point operations. If the function was not inline and instead received the values by reference then it would compile to 18 floating-point operations, most of them being multiplications by zero or one.
sensor = FusionCalibrationInertial(sensor, FUSION_IDENTITY_MATRIX, FUSION_VECTOR_ONES, offset);
These optimisations have been verified through profiling. The above line compiles to 25 instructions for a MIPS processor with FPU. This increases to 67 instructions when the function is not inline. That's a 270% improvement in execution speed for a line of code that may be called thousands of times a second in perpetuity on an embedded system.
A reduction in code efficiency is often preferable if it means enhancing readability, conforming to conventions, etc. However, in this case, I believe the SonarCloud suggestion is essentially bad advice that results from the analysis being not quite intelligent enough. I suggest that you implement rule exceptions if possible and where appropriate.
Are there other SonarCloud suggestions that you believe may be improvements to Fusion?
from fusion.
Thanks a lot for the detailed feedback.
Are there other SonarCloud suggestions that you believe may be improvements to Fusion?
You should be able to see them here, but after reviewing them, they don't all seem relevant based on your previous explanation.
The reason this is not a fix or improvement here is because the function is declared static inline within the header file. The function will therefore be implemented inline for each call so that no data is copied to the stack.
I don't know about C, but in C++, inline
is considered a hint at the compiler to inline the function, but the behavior depends on optimization and the compiler is always free to do as it wants. Using __attribute__((always_inline))
on the other hand will force inlining.
How can I make sure that on my hardware, STM32F769 (ARM Cortex M7), the functions are inlined and that I can benefit from the optimization you're talking about?
How would I benchmark forced inline
vs maybe inlined
?
from fusion.
Your description of inline
applies to both C and C++. __attribute__((always_inline))
was used in older versions of the library. However, this was converted to inline
because profiling indicated identical performance, and it is generally preferable to guide the compiler towards optimisations rather than force it.
How can I make sure that on my hardware, STM32F769 (ARM Cortex M7), the functions are inlined and that I can benefit from the optimization you're talking about? How would I benchmark
forced inline
vsmaybe inlined
?
I suggest that when using a mature, third-party library, best practise is to trust the authors and use the library as is. The library code should remain identical to the source (including white space) after integration to your project. This allows updates to be quickly reviewed as diffs, simplifies pull requests, and allows any reader of the code (including yourself in the future) to easily verify the library against the source.
I have created the temporary branch, no-inline that removes the static inline
optimisations from FusionMath.h. I suggest you benchmark the performance of this branch against main by profiling the function FusionAhrsUpdateNoMagnetometer
. It is important that the function is called using real sensor data and not constant values, and that the result is averaged over several seconds. I have just done this for the PIC32MZ2048EFG100 and found main executes in 4.22 us, and no-inline executes in 7.76 us. In this case, the optimisations allow the algorithm to execute almost twice as fast. I would be interested to hear your results.
Thank you for sharing the SonarCloud issues. Most of the suggested changes are not applicable. For example, using
, the reference declarator (&
), enum class
, and std::varient
do not exist in C. I believe that "make the type of this parameter a pointer-to-const" refers to lines 97 and 318. This has been fixed in 4b0bd28.
I disagree with the suggestion to add a default case to the switch statement. A switch statement operating on an enum and implementing all values should not include a default case so that the compiler is able to provide the "enumeration value not handled in switch" warning. Adding a default case would inhibit this warning and prevent the compiler from detecting errors.
I do not have access to see which lines of code the following issues refer to. Please can you confirm.
from fusion.
Your description of inline applies to both C and C++. attribute((always_inline)) was used in older versions of the library. [...]
Makes perfect sense.
I suggest that when using a mature, third-party library, best practice is to trust the authors and use the library as is. [...]
This is exactly our workflow for the different external libraries that we use, including yours. And I can say from experience that it has saved us a lot of time and headache!
I have created the temporary branch, no-inline that removes the static inline optimisations from FusionMath.h. I suggest you benchmark the performance of this branch against main by profiling the function [...]
I would be interested to hear your results.
I'll try to do the benchmark tomorrow or the day after, it's something we wanted to do anyway to measure the impact on our whole system.
Thank you for sharing the SonarCloud issues. Most of the suggested changes are not applicable. [...]
Yes, our project is written in C++ with a few parts in C, so Sonarcloud is configured that way.
If you were to use Sonarcloud for Fusion, you could set it up to only do C, avoiding all the C++ suggestions.
I disagree with the suggestion to add a default case to the switch statement. [...]
Funny you say that, I had the exact same reasoning during a code review last week. I've actually changed the parameters of Sonarcloud to say that but forgot turn off the previous one, hence the "wrong" suggestion.
I do not have access to see which lines of code the following issues refer to
Those were for our codebase and have been fixed.
Thanks again for the time you take answering all my questions. I've learned a lot and it pushed me to dive deeper into your code and the math behind it. I cannot say I'd be able to do it on my own from the top of my head, but it gave me a better understanding of the whole subject and in the end made me a little better developer than I was before.
I'll keep this open till I can do the benchmark and report the results here.
from fusion.
The temporary branch, no-inline has now been removed. Thank you for your comments. The project is improved by discussions like this, even if the code is not changed.
from fusion.
Related Issues (20)
- How to use the magnetometer data with different sampling frequency from the inertial sensors? HOT 1
- Gyroscope Offset Algorithm HOT 7
- Link to dissertation is broken HOT 1
- Q: Influence of motion and external acceleration HOT 3
- Very noisy Roll Pitch Yaw data HOT 5
- Recommended sampling rate? HOT 2
- yaw value changes while rotating on only roll or pitch axis. HOT 3
- 5 seconds delay in real-time operation (python) HOT 2
- Need some help setting up the algorithm. HOT 14
- Application Questions HOT 25
- Integration of GPS data to estimate position and velocity HOT 1
- Proper way of calling FusionAhrsUpdate with accel, gyro and mag all having different update rates HOT 2
- What is the difference between this algorithm and madwick gradient algorithm? Can it still be called gradient descent algorithm? HOT 7
- 做过这块IMU融合的加一下我微信 HOT 1
- data advice request HOT 1
- Anomaly quaternion when applying realtime calculating by sensor data from UDP connection HOT 5
- Overshooting and slow stabilisation pitch value HOT 7
- Thesis availability HOT 3
- How to get the Gravity data HOT 8
- [Question] Can I set an initial orientation? HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from fusion.