Coding Standard refactoring about marlindocumentation HOT 9 CLOSED

Here’s the video I was thinking of, apropos to this topic, that every Marlin developer should watch. Using C++17 (and in some cases 14 and 11j to write code that compiles with the least possible overhead, even when using the stdlib. The example used is a Pong game on the Commodore 64, with intermediate compilation to X86 Assembler. This is the video that got me using const to an obsessive degree, certainly more than is necessary, to help the compiler optimize wherever it can. In particular the use of a const std::array in the video has a dramatic effect compared to a non-const instance. (And with C++17 a constexpr Instance will also be possible.)

https://youtu.be/zBkNBP00wJE

from marlindocumentation.

CamelCase has a lot of fans... Myself included. And it would be nice to get rid of floating point just because of the speed issues associated with its use. But we need it for both the precision and the dynamic range of the numbers.

from marlindocumentation.

Macros are bad

I'd run while you still can before thinkyhead sees this, joking aside we need Macros and defines in Marlin for the level of control over the build and what code is even parsed by the compiler, in the future constexpr ifs(c++17) could do a much better job (and multiline constexpr function from c++14) but at the moment we are stuck with the evil that is the preprocessor.

from marlindocumentation.

While it's certainly important to hear differing opinions on how Marlin should have been coded in the past and how it should evolve going forward, it's always better to have the actual code changes submitted for evaluation as pull requests so we can see the results and whether they keep the code lean and fast for the sake of 128K MCUs. Simply saying "this and that could be better" is unlikely to get us to stop the presses and rewrite the existing code when we have so much on our plates. If you have good ideas, join the project and help make the needed changes.

from marlindocumentation.

CamelCase has a lot of fans... Myself included. And it would be nice to get rid of floating point just because of the speed issues associated with its use. But we need it for both the precision and the dynamic range of the numbers.

• i understand the need for dynamic ranges but the where do we need this? bigger printers need more steps? that is known at compiletime and then we must use something ugly like a 64 or 128bit integer. That should be still fast enough compared to Floats.
• on the other side we dont have that precision on floats. [source] (its easier to copy this proposal with its explaination on this topic)

I'd run while you still can before thinkyhead sees this, joking aside we need Macros and defines in Marlin for the level of control over the build and what code is even parsed by the compiler, in the future constexpr ifs(c++17) could do a much better job (and multiline constexpr function from c++14) but at the moment we are stuck with the evil that is the preprocessor.

I don't understand why if constexpr()is the solution here. Its only helping if only called elements exists. Because i can't explain it that well here some code:

extern int x; // no definition of x required
int f() {
if constexpr (true)
    return 0;
else if (x)
    return x;
else
    return -x;
}

[SRC]
constexpr variables and optimization like -O1 are enough. (Arduino IDE uses -Os)
proof in Godbolts Compiler Explorer

While it's certainly important to hear differing opinions on how Marlin should have been coded in the past and how it should evolve going forward, it's always better to have the actual code changes submitted for evaluation as pull requests so we can see the results and whether they keep the code lean and fast for the sake of 128K MCUs. Simply saying "this and that could be better" is unlikely to get us to stop the presses and rewrite the existing code when we have so much on our plates. If you have good ideas, join the project and help make the needed changes

My intention here was to see the feedback on this topic. If people have complete different opinions on solutions and problems, they cant work together. I just don't wanted to hear "nah we dont want it your way. We love the old one" after working for weeks on changes and my code will
definitely break the coding standard of this project.

from marlindocumentation.

I don't understand why if constexpr()is the solution here. Its only helping if only called elements exists. Because i can't explain it that well here some code:

It's a solution to having preprocessor like capabilities in the compiler, constexpr ifs can be a 'replacement' for preprocessor ifdefs, they allow the same precision in what code is actually compiled allowing for the architecture and feature specific code paths that Marlin needs while being able to use constants rather than definitions.

Marlin is very reliant on the preprocessor at this point, I don't like that, but I understand why it was designed that way at the time, and how fundamentally intertwined it currently is but if you could remove the preprocessor usage from Marlin and maintain binary size and performance across all architectures and configurations while allowing us to do runtime configuration on the more powerful boards I'm pretty sure we would all be very happy.

from marlindocumentation.

If people have complete different opinions on solutions and problems, they cant work together.

Everything is mutable. I wrote up the coding standards because there weren't any, and the code style was all over the place. The intent was to formalize the de facto standards which were already prevalent in the codebase and to have something we could refer contributors to instead of having to reiterate over and over.

Since we actually work with the codebase every day, we're hugely aware of its shortcomings, but there are only so many hours in the day, and we have a lot of things on our plates just patching up existing issues, implementing feature requests, and trying to move on from the 1.1.x codebase and get rolling with the unified 2.0.x codebase.

Much of what you suggest is already very much on our minds. We've had a lot of debate and discussion around these things, but as it takes time away from the work we really need to get done right now, I will keep insisting on less talk and more action.

That said, let me briefly go over your points with some first impressions…

• .h versus .hh or .hpp. I'm pretty "meh" about this, since I would prefer all the code within the Marlin folder to be under C++ semantics, but if it helps the compiler then I have no problem with it.
• Capitalization: I'm really not a fan of camelCase. I much prefer lowercase with underscores, as I just find it more readable. Currently this is the most prevalent style in Marlin, so I think we should just stick with it.
• Language features: The 1.1.x branch is meant to be the last version usable with Arduino 1.6.8, and until we retire the 1.1.x branch with the release of 1.1.9 we're not going to go beyond C++11 features. However, for the 2.0.x codebase it would be great to be able to use whatever C++14 (etc.) features we can that make the code better.
• STL: Marlin was originally built in C, not C++, and we have been evolving it slowly to be more C++-centric over the last few years. That said, we still want to be considerate of 128K boards and keep Marlin as small and performant as possible. That has meant leaving out libraries that may bloat the code and expand SRAM-based data structures. If STL can be used without bloating the code and data, that would be preferable. We don't care about objects, per se, we don't care about instances, and we cannot generally use new or malloc.
• Floating-point: We'd like to use fixed-point for some stuff, but sometimes it's better to use float. Fixed-point has rounding errors too. Sometimes we want the float used as input to be printed and give back the same value as the input. Depending on the mantissa, fixed-point can transmute the value even worse than float rounding. Anyway, converting everything away from float is a huge pile of work that simply no one has volunteered for.
• Includes and #pragma once: Sure, now that we can use #pragma once, great, let's use it everywhere we can! We also like to not-include headers (by feature) and in many cases not-compile whole .cpp files (by feature). One area that needs much help is the HAL, which has a lot of redundancies and some confusion about both internal and external dependencies. Up to now I've been staying out of the way so that the HAL could just get to a working state, and then I was planning to get into a proper cleanup after 1.1.9 is released, when we're focusing all attention on 2.0.x.
• Replacing macros: Where macros can be replaced with constexpr, const, etc., that would be great. Sometimes this is possible, and sometimes this is not so simple. Again, if someone wants to get all up in the style thing and truly improve the codebase, we would welcome such contributions.
• Avoid #define: We're not going to change all the configuration files to use constexpr in place of #define at this stage. We rely on the preprocessor to only compile the code we want and keep everything else out. There's a lot that the compiler can do to optimize things out, but #ifdef can keep the compiler from wasting time on it in the first place and speed up compile time.

So anyway, you're not the first (and I'm sure won't be the last) to point out all the things we wish we had done yesterday but (due to the linear nature of time) have not yet gotten around to doing. If you look back on Marlin 1.0.0 and follow its evolution over the last 5 years you will see that a huge amount of work has gone into establishing some basic standards, cleaning up the code, trying to make its patterns more evident, and waiting for Arduino IDE to catch up and allow the use of C++11 features. Right now we are at the very tail end of a two year process of molding the 1.1.x codebase into a final form so that we can move on and get into architectural improvements. I've personally wanted to rewrite all the LCD code for over a year, for example, but simply haven't had the time and space to delve into it.

If, as it appears, you would like to help us to modernize the code-base while also keeping it lean and performant, please get on board and start making iterative improvements to the bugfix-2.0.x branch. We will evaluate them on their own merits and accept any and all that make clear improvements. Just, please, don't be one of those people who is always pointing out how stupid things (and by extension, we) are.

from marlindocumentation.

.h versus .hh or .hpp. I'm pretty "meh" about this, since I would prefer all the code within the Marlin folder to be under C++ semantics, but if it helps the compiler

.h is fine for both C and C++. There is no reason to do anything different.

Floating-point: We'd like to use fixed-point for some stuff, but sometimes it's better to use float. Anyway, converting everything away from float is a huge pile of work that simply no one has volunteered for.

Soon, with 32-bit platforms we can just declare everything as double. I'll take that over fixed point.

Avoid #define: We're not going to change all the configuration files to use constexpr in place of #define at this stage.

Agreed...

Includes and #pragma once: Sure, now that we can use #pragma once, great, let's use it everywhere we can! We also like to not-include headers (by feature) and in many cases not-compile whole .cpp files (by feature).

If, as it appears, you would like to help us to modernize the code-base while also keeping it lean and performant, please get on board and start making iterative improvements to the bugfix-2.0.x branch. We will evaluate them on their own merits and accept any and all that make clear improvements. Just, please, don't be one of those people who is always pointing out how stupid things (and by extension, we) are.

crickets???

from marlindocumentation.

Soon, with 32-bit platforms we can just declare everything as double. I'll take that over fixed point.

With many (if not all) of the ARM chips, interrupts cannot use the FPU. So, fixed-point would still be optimal within the stepper ISR and other interrupt contexts.

from marlindocumentation.