[Grbl_Esp32 Issue#504] Coding style, architecture, and a lot of little things

#1 – ggallant571 于 2020-08-02

If I were to apply a similar rewrite to the code based there would be
zero C++. This is embedded firmware with dedicated functionality. Does
not need a religious devotion to useless methodologies designed to help
the novice programmer. In fact I consider your efforts an insult to the
original authors. They had the skills and drive to develop an excellent
project. Yours sounds like a graduate student term project geared toward
appeasing the CS gods.

#2 – bdring 于 2020-08-02

@ggallant571

Please, only constructive or positive commentary is acceptable on this forum.

#3 – atlaste 于 2020-08-03

Just wanted to add that this is by no means meant as an insult; on the contrary. In fact, I am grateful for the help I got with creating my CNC board, so I decided to contribute as well. And while I’m still a novice on electronics, people do recognize me as an expert software engineer in C++. Still, I always feel my work should speak for itself.

Again, I stress this is still a WIP, and more time is needed from my part.

In any case: if my help is not appreciated by Bart and Mitch, that’s my loss, and mine alone. In that case, we’ll just close this ticket and leave it at that.

#4 – karoria 于 2020-08-03

@ggallant571
If you are so confident, port grbl to a controller of your choice and make your own name that way.
As a user, you or me don’t have right to criticise for the constructive work. Especially, developers are spending their precious time for a piece of work, you and me are using freely. There is always an option with you to buy professional cnc controllers and then criticise that company if you are not satisfied with their product. So, let’s talk in a positive way in this forum.

#5 – ggallant571 于 2020-08-03

I regret have responded much to harshly. Been in the industry a long
time (started with assembler) and have been bit multiple time by similar
statements.

I consider the GRBL code to be among the best products ever developed.
It just works. In fact it works fantastic considering for the
constraints it faces. I greatly admire the skill and fortitude of the
developers who persisted in producing the product. Perhaps so much that
suggesting another methodology was deemed an insult to them.

Going forward with more powerful processors probably warrants an
overhaul of the entire project. Especially at the user configuration
layer. Having someone willing volunteer is great.

As for porting GRBL myself, did that for STM32 long before there was a
public repository.

#6 – atlaste 于 2020-08-03

@ggallant571 From your comments I believe you incorrectly assumed that I introduced things like classes and (more general) C++ code. I did not. GRBLESP32 is not GRBL; it evolved, already had classes for spindles and motors and uses classes from external libraries. IMO this has been a good choice, and given the fact that the ESP32 is much more powerful than an Arduino Uno, it makes all the sense in the world to also shift more things to runtime configurable. In fact one of the recent features was the spindleselect call, which is a good example of this.

Obviously functionality and stability are key to CNC machines. There is absolutely no discussion about this (I ran a 800K line toolpath yesterday, imagine how I would have felt if it broke half way). In fact, I believe that if my work is done properly, it should be relatively easy to run a ‘simulated’ CNC machine on a PC, which enables much easier and faster testing, to ensure stability for future releases. But to be fair, this is currently only something on my wish list, because of the complexity of this. But… until such a facility is there, I also don’t believe it would be wise to overhaul the entire project, especially the logic. Yes, I admit I would love to split the gcode parser into all the individual G/M-codes (either like Marlin did or as functions) and unit-test them all separately – but in all likelyhood this would currently just break a lot of things.

This is why I started with the low hanging fruit: moving things and adding the obvious (missing ctors, virtual destructors, includes, namespaces, iso std naming, style, etc) without changing the logic. One step at a time.

#7 – MitchBradley 于 2020-08-03

I think this is a worthwhile path. Bart and I have been taking steps in this direction already, so you are definitely on a plausible track. I am a dyed-in-the-wool C programmer from way back, so migrating to C++ caused me some concerns, but my recent experience with the new Settings architecture convinced me that C++ can be very valuable if used tastefully. By “tastefully”, I mean “avoid the use of enormous external frameworks” and “try not to use so much abstraction and overloading that it becomes impossible for a new reader to figure out what is going on”.

Instead of a fork with an omnibus suite of changes, perhaps we can do it incrementally, as a series of PRs, presented one at a time with time for review in between. That makes testing and review much easier and permits bisecting to find problems. Here are my comments on individual points:

> Compile in vMicro. vMicro is the most popular Arduino Visual Studion addin that I know of for Arduino projects… and I’m using it, so it made sense to use this to build the code.

Bart and I both use platformio for most of our work now. It can be used either from the command line, or integrated within the VSCode IDE, or in a hybrid mode with VSCode integration but with an external editor. platformio compiles much faster than Arduino, perhaps due to the fact that it compiles individual files instead of doing the dubious Arduino trick of concatenating everything into one file and compiling the entire wad.

VSCode’s popularity seems to be growing by leaps and bounds, so it feels like a good choice for “primarily-supported IDE”.

I don’t know what vMicro’s characteristics are – but I do think that supporting two environments is hard enough, so adding a third seems like extra work without much benefit. If vMicro configuration is so equivalent to Arduino as to need no extra consideration, then it is a don’t care. We have to support Arduino due to its familiarity, but we really don’t like it very much – and the need to accommodate novices might go away when “one build to rule them all” lands (see below).

> Documentation. I’ve added some MD files for documentation.

Documentation is always very much appreciated! Separate PR.

> Coding style. I’ve noticed that the code base has grown organically since it was first ported from GRBL, with different coding styles across the code base. For me, it made it difficult to see what’s going on, so I added a .clang-format file and tuned it to what I saw as the major coding style. It’s not perfect, but at least it’s consistent.

That was on our list and Bart took some steps in that direction, but we have not been using it as systematically as we should. I think this would be an excellent first step. Perhaps you could ensure that it works well in the VSCode/platformio environment (VSCode does support clang-format), then teach us how to use it effectively. A global reformatting that changes only whitespace would be a good patch, along with some sort of git automation to check commits.

> C/C++ issues. There were a few C/C++ issues in the code base that I spotted. To name a few: _ as a prefix in the global namespace is reserved, and #include CPP files is generally accepted as a bad practice. Where I saw these, I eliminated them.

This would be a good second patch. I agree that including CPP is not good. I forget why we did it but it might have had something to do with Arduino IDE strangeness. If you can find a better solution that works with both plain Arduino and platformio, that would be a good second patch. The third would be to eliminate the _ prefix.

> Classes and filenames. A simple way to aid people with what’s where is to put classes in the file with the same name. Also, this helps to speed up compile times, because the compiler should do less work for each class.

This sounds good. The benefit to humans is obvious. Can you explain how it helps the compiler do less work? I didn’t realize that the C++ compiler was cognizant of the relationship between the filename and its contents.

> Includes. Most code just included grbl.h which basically includes everything. That means that incremental compilation will be broken, because for each header change, the compiler has to re-compile the lot. I changed the files to include just what is needed there.

Wow, I am really impressed that you pulled that off, considering the difficulty of sorting out what is used where and the dependency graph. I usually try to do it that way, and especially to structure the code so individual files depend on the fewest number of includes – but in a large project, maintaining that purity becomes difficult over time. I would very much like to see your solution.

> Namespaces. For me, it made sense to move code to where it belongs. So, spindles go into the Spindles namespace, motors go into the Motors namespace and so forth. This is still a WIP though, it’s hard to decypher where everything should go.

I’m not sure what you are suggesting here. We have the Spindles and Motors classes (which probably need improvement); do we also need namespaces for those concepts?

> Const versus define. Currently, there are a ton of #define’s in the code base that could also be class (const/constexpr) variables. The advantage of the latter is that you get compile-time checks (because they are strongly typed) and that they are scoped. I actually fixed a few implicit rounding bugs by making code const. (In terms of performance or memory usage it doesn’t matter).

Yes, definitely want to do that – and also use enums instead of #defines where that makes sense. Would love to have you help – but as small targeted patches instead of one giant omnibus.

> Mixing C and C++. The old GRBL code base is pure C, which is mixed with C++. I wanted to change the old GRBL code to classes as well – basically that’s also what was going on anyways with the prefixes, so I might as well give it a push. This is also currently the main reason why the code is not compiling. I’ve started moving things, but this still needs more work.

Seems like one of the later patches in the series.

> Settings and config. I think it would be best to separate compile-time and run-time settings better.

This is a good topic for discussion on slack. My goal is to get rid of compile-time settings altogether, enabling “one build to rule them all, configure by loading a text file”. That would not be feasible on an AVR, which is why classic GRBL – and to an even greater extent Marlin – have all the compile-time settings. But it should be entirely workable on 32-bit processors, considering the very low cost of SPI FLASH chips.

The settings rewrite was constrained by the need to maintain compatibility with classic GRBL – so senders don’t break – and with both the current and the upcoming versions of WebUI. It was a delicate dance, but we are pretty happy with the result. The overall code size decreased substantially, while adding functionality and improving maintainability. The namespace is not perfect – I think it is impossible to design a namespace schema that is simultaneously optimum for every possible point of view or use case – but so far it feels pleasant.

> I still have some open questions that I couldn’t figure out by myself:

> In general. I’ve changed quite a bit of the structure; perhaps some of them are not really the best choice. I’m therefore curious what your overall impression is.

Addressed above in detail. Overall, you seem to be moving in a direction that I personally like – and based on my conversations with Bart, I suspect that he feels generally similar.

> Solenoid pen. Solenoid pen seems to me like a type of spindle. Why is this a separate file?

Probably historical. Spindles are a WIP. If you can merge the pen into the Spindle class structure – or into an improved structure – please give it a shot. But do it as a separate patch (or series).

> Servo. Similarly, I’m a bit confused about the servo_axis and the RcServoClass. Why is not everything in the latter? Or is this still a WIP?

WIP

> Print. I didn’t quite understand the print.cpp functions, as there’s also grbl_send which seems to be the more generic variant.

I don’t know where the print functions came from; perhaps a remnant from classic GRBL. They aren’t used anywhere that I can find. I removed print.cpp and print.h and it still compiled – although I have not tried every possible combination of #defines

> Partial compilation. I found myself wondering what the benefit would be to be able to disable features like WIFI and BT. Since the ESP32 has the required space, why not simply make this a runtime setting? Is there any benefit in using a ifdef here?

That is historical. As mentioned, we would like for all ifdefs to go away. There are many aspects of WebUI integration that are redundant with similar grbl code, so improvements and coalescing are possible. Indeed, the substantial code size reduction that I achieved with the new Settings came mostly from improvements in WebUI code, replacing large chunks of copy-and-paste code with iteration over data.

> The branch I’m working on can be found here: https://github.com/atlaste/GrblEsp32/tree/CleanupDevt/GrblEsp32 . Again, I stress that this is still a WIP and not even compiling; more work is needed, and I’ll continue to invest time whenever I can if possible.

Shall we start the process of incremental PRs?

#8 – atlaste 于 2020-08-04

Just noticed I have to ask for a slack invite. Not sure how that works… but please do send one to: atlaste at yahoo dot com; might be easier for communication.

> Instead of a fork with an omnibus suite of changes, perhaps we can do it incrementally, as a series of PRs, presented one at a time with time for review in between. That makes testing and review much easier and permits bisecting to find problems.

Normally I wouldn’t agree on this, because it implies I have to start over… but since you guys helped me out so much with my board I’ll make an exception.

> Bart and I both use platformio for most of our work now. It can be used either from the command line, or integrated within the VSCode IDE, or in a hybrid mode with VSCode integration but with an external editor. platformio compiles much faster than Arduino, perhaps due to the fact that it compiles individual files instead of doing the dubious Arduino trick of concatenating everything into one file and compiling the entire wad.

vMicro does that as well. I’m not familiar with platformio though; will look into it. Personally I’m happy as long as I can use Visual Studio (btw this is a different product than VSCode). The sole reason I (and many other people) use vMicro, is because Arduino support doesn’t come out of the box.

> > Documentation. I’ve added some MD files for documentation.
>
> Documentation is always very much appreciated! Separate PR.

OK. Might also be a good time to talk about this a bit for a general direction.

Small background… at my company I have to manage a multi-million loc code base. Obviously we have quite some documentation about the code. One thing that has been hard at the beginning was to keep the code and the documentation in sync – because developers tend to change code and not search for documentation.

At some point, we decided to use MD files, and put them in the same folders (with similar names) as the cpp/h files. While this might seem messy at first, the big advantage is that if there’s documentation about code, it’s also ‘in your face’, which helps a lot to keep the code and documentation in sync. I would strongly suggest using this approach here as well.

> That was on our list and Bart took some steps in that direction, but we have not been using it as systematically as we should. I think this would be an excellent first step. Perhaps you could ensure that it works well in the VSCode/platformio environment (VSCode does support clang-format), then teach us how to use it effectively.

I’ll put this in a separate ticket with more details, along with a PR #1.
Just so you know, while VSCode and Visual Studio both support clang-format, but both don’t support the latest version and features.

> A global reformatting that changes only whitespace would be a good patch, along with some sort of git automation to check commits.

I acknowledge CI is a good idea in general, we use it extensively here. However, to setup CI properly is definitely a separate task, and one that can involve quite some time to get right. I can describe what we do at our company briefly to give some idea.

CI shouldn’t commit; it will give strange behavior for developers, because you suddenly get a new branch which needs merging. So, what we do is run clang-format, do a diff, and check if it’s empty. But CI can aid with much more: we automatically build, do static code analysis, run code coverage on all unit tests and create install packages. Some time ago, I created https://github.com/atlaste/CPPCoverage , which we also run along with all our unit tests, to automatically generate a report and check if everything is still working. Because we run a company, we have the agreement that coverage should generally go up between master merges (this is not enforced), so things get better over time.

> This would be a good second patch. I agree that including CPP is not good. I forget why we did it but it might have had something to do with Arduino IDE strangeness. If you can find a better solution that works with both plain Arduino and platformio, that would be a good second patch.

OK, let’s call this PR #2.

> The third would be to eliminate the _ prefix.

OK, let’s call this PR #3.

> > Classes and filenames. A simple way to aid people with what’s where is to put classes in the file with the same name. Also, this helps to speed up compile times, because the compiler should do less work for each class.
>
> This sounds good. The benefit to humans is obvious. Can you explain how it helps the compiler do less work? I didn’t realize that the C++ compiler was cognizant of the relationship between the filename and its contents.

No, the compiler is not. However, if you have a big header file with all the definitions, it has to be parsed every time the base class is included (because all the definitions are there). If a classes are split, the amount of code that needs to be parsed is very small.

There is much overlap with this and the above (PR #2), so I’ll combine these.

> > Namespaces. For me, it made sense to move code to where it belongs. So, spindles go into the Spindles namespace, motors go into the Motors namespace and so forth. This is still a WIP though, it’s hard to decypher where everything should go.
>
> I’m not sure what you are suggesting here. We have the Spindles and Motors classes (which probably need improvement); do we also need namespaces for those concepts?

Let’s just start there, PR #4.
For clarity, the name of the folder is usually the name of the namespace.

I wanted to introduce some more concepts: settings, machines, IO. Let’s do this incrementally and see what works (and what not).

> > Const versus define. Currently, there are a ton of #define’s in the code base that could also be class (const/constexpr) variables. The advantage of the latter is that you get compile-time checks (because they are strongly typed) and that they are scoped. I actually fixed a few implicit rounding bugs by making code const. (In terms of performance or memory usage it doesn’t matter).
>
> Yes, definitely want to do that – and also use enums instead of #defines where that makes sense. Would love to have you help – but as small targeted patches instead of one giant omnibus.

> > Mixing C and C++. The old GRBL code base is pure C, which is mixed with C++. I wanted to change the old GRBL code to classes as well – basically that’s also what was going on anyways with the prefixes, so I might as well give it a push. This is also currently the main reason why the code is not compiling. I’ve started moving things, but this still needs more work.
>
> Seems like one of the later patches in the series.

Let’s call this PR #5 and start with the easy things. Easy here is defined as: put the code in classes and change into static members. That way, naming will be correct (see below for the main reason), even though the code behaves exactly the same.

> > Settings and config. I think it would be best to separate compile-time and run-time settings better.
>
> This is a good topic for discussion on slack.

Yes, I believe more discussion about this subject is a good idea.

> Wow, I am really impressed that you pulled that off, considering the difficulty of sorting out what is used where and the dependency graph. I usually try to do it that way, and especially to structure the code so individual files depend on the fewest number of includes – but in a large project, maintaining that purity becomes difficult over time. I would very much like to see your solution.

Once code is in the right class and namespace, this is actually not that hard. If namespaces correspond with folders and classes with files, you can simply browse the code and see what’s needed. For example, say Planner::cycle_reinitialize() is used in the code, it’s obvious that Planner.h needs to be included. This should therefore also be done after C code has been moved to classes. Once finished, if you want to know if each include was really necessary, remove it and re-compile the file.

What remains are the #define’s of the machine/settings configuration and the platform includes. For the configuration, that’s usually an #ifndef, #define, which I combined in a Config.h. I wasn’t happy with this, because it creates implicit dependencies between files but it’s still a fine first step (I think a better approach would be to use name hiding – but let’s save that for another day).

Anyways, let’s call this PR #6.

> > In general. I’ve changed quite a bit of the structure; perhaps some of them are not really the best choice. I’m therefore curious what your overall impression is.
>
> Addressed above in detail. Overall, you seem to be moving in a direction that I personally like – and based on my conversations with Bart, I suspect that he feels generally similar.

That’s good to hear!

> > Solenoid pen. Solenoid pen seems to me like a type of spindle. Why is this a separate file?
>
> Probably historical. Spindles are a WIP. If you can merge the pen into the Spindle class structure – or into an improved structure – please give it a shot. But do it as a separate patch (or series).

PR #7

> > Servo. Similarly, I’m a bit confused about the servo_axis and the RcServoClass. Why is not everything in the latter? Or is this still a WIP?
>
> WIP

PR #8

> > Print. I didn’t quite understand the print.cpp functions, as there’s also grbl_send which seems to be the more generic variant.
>
> I don’t know where the print functions came from; perhaps a remnant from classic GRBL. They aren’t used anywhere that I can find. I removed print.cpp and print.h and it still compiled – although I have not tried every possible combination of #defines

Let’s just delete them. ESP32 is also included elsewhere for InputBuffer, so it just adds to the confusion.

> Shall we start the process of incremental PRs?

Let me start by joining on Slack, and then I’ll start with #1 above.

#9 – bdring 于 2020-08-23

Closing….discussion moved to Slack