Marlin 2.0 Refactor

[thinkyhead]

Where to find it

The bugfix-2.0.x-new-layout branch is a refactoring of the Marlin 2.0 codebase which includes the 32-bit HAL.

I'll be doing the meticulous work in my bugfix_refactor_work branch, and will make sure to keep the bugfix-2.0.x-new-layout branch in a working state for testing. Please report if it isn't compiling and I will aim to fix it quickly!

Development roadmap

The intent is to produce a more logically organized and granular codebase, encapsulate globals and their related g-code handlers into static singletons, by feature, and produce a codebase that will be easier to extend. As a refactor, the code size, logic, and performance is meant to remain unaltered. However, in practice, the inlining, linking, and calling behavior is bound to change. Please report any major changes in performance.

As this code develops, you may find things about it bothersome. This code will continue to develop and improve largely according to your feedback. Nothing in the branch is written in stone, and all ideas are welcome.

Initial status

As of this writing the branch has been through a couple of iterations, and it compiles successfully for both Arduino IDE and PlatformIO, giving a near-identical binary as the current "flat" codebase. The g-code handlers are split out into .h files to start to encapsulate them into units, by feature, where possible. This initial breakup into .h files was a good first step, as the main thing was to produce a new layout that could compile but without making any functional change to the firmware. This is proving a useful organization from which to go forward with the next step…

Static classes, namespaces

We don't have to sacrifice compiler optimizations such as inlining smaller handlers just because we move things into .cpp files, so long as all the g-code handler code is encapsulated into static singletons. The smaller inline-able methods can be defined in the .h files of the new singletons, while the bigger handler functions that only bloat the process_next_command function can be static.

The G-code handlers can be moved into singletons by an almost automatic process, taking each G-code handler now inside an .h file and doing a quick refactor…

• Rename the .h to .cpp file and compile to see where it fails, citing dependencies.
• Bring in the needed extern and #include lines to resolve the dependencies.
• In some cases shift the definition to the g-code handler files, or added headers.

By doing this in groups of G-codes that all relate to the same config option, they can then be grouped into a singleton class, any feature-related globals and functions currently defined in Marlin.cpp will be moved to the new singleton class. Any functions they share with other config features can be encapsulated in a shared friend class.

The static class instance has been chosen rather than pure namespaces because they provide some extra features and syntactic sugar. The instances are superfluous, but allow us to use dot-notation to access class data. (planner.something() does the same as Planner::something()).

Parallel Development

As long as most development and bug-fixing to bugfix-2.0.x focuses on the HAL, I can continue to refine the G-code handling code and restructure this branch without disrupting that work. As long as patches made to Marlin_main.cpp and other files are straightforward, they shouldn't be hard to bring into the refactored branch, as the intent is only to reorganize and not, largely, to modify the code.

Branch Cleanup

To make it easier to follow changes, I will re-build the bugfix-2.0.x-new-layout commits again except this time I will move all the files, unchanged (except renames), to their new locations, and only afterward apply all the textual changes to the files. That set of textual changes will be primarily to the #include lines, so it should make a nice picture of how dependencies are affected.

Timetable

• [x] 6 Sept: Rebuilt branch commits, identical to current time (5 Sept)
• [x] 7 Sept: Some G-code handlers refactored into feature singletons.
• [x] 9 Sept: Consolidate motion, kinematics, leveling, etc.
• [x] 12 Sept: Complete handler refactoring.
• [ ] Oct-Nov: Refactor LCD and other code as static singletons, as appropriate

@MarlinFirmware/32bit-maintainers

[thinkyhead]

To convert bugfix-2.0.x to the new layout in a fresh branch…

Conversion Script

```sh #!/bin/bash cd /path/to/MarlinFirmware git reset --hard 27cbb93 git mv platformio.ini Marlin/ cd Marlin git mv Marlin_main.cpp src/Marlin.cpp git mv Marlin.h src/ git add . ; git commit -m "Move root sources" ; echo mkdir -p src/config git mv Configuration*.h src/config/ mkdir -p src/config/examples git mv example_configurations/* src/config/examples/ git add . ; git commit -m "Move configs" ; echo mkdir -p src/feature git mv blinkm.* digipot_mcp*.cpp I2CPositionEncoder.* Max7219_Debug_LEDs.* pca9632.* twibus.* src/feature/ mkdir -p src/feature/dac git mv dac_dac084s085.* dac_mcp4728.* stepper_dac.* src/feature/dac/ mkdir -p src/feature/mbl git mv mesh_bed_leveling.* src/feature/mbl/ mkdir -p src/feature/ubl git mv G26_Mesh_Validation_Tool.cpp ubl* src/feature/ubl/ git add . ; git commit -m "Move 'feature' files" ; echo mkdir -p src/core git mv boards.h enum.h language.h macros.h serial.* types.h utility.* src/core/ git add . ; git commit -m "Move 'core' files" ; echo mkdir -p src/gcode git mv gcode.cpp src/gcode/parser.cpp git mv gcode.h src/gcode/parser.h git add . ; git commit -m "Move 'gcode' files" ; echo mkdir -p src/inc git rm Conditionals.h git mv Conditionals_LCD.h Conditionals_post.h MarlinConfig.h SanityCheck.h Version.h src/inc/ git add . ; git commit -m "Move 'inc' files" ; echo mkdir -p src/lcd git mv thermistornames.h ultralcd_impl_*.h ultralcd.* utf_mapper.h src/lcd/ git add . ; git commit -m "Move 'inc' files" ; echo mkdir -p src/lcd/dogm git mv dogm_*.h ultralcd_st*.h src/lcd/dogm/ mkdir -p src/lcd/language git mv language_*.h src/lcd/language/ git add . ; git commit -m "Move 'lcd' files" ; echo mkdir -p src/libs git mv buzzer.h circularqueue.h duration_t.h hex_print_routines.* least_squares_fit.* nozzle.* point_t.h private_spi.h softspi.h stopwatch.* vector_3.* src/libs/ git add . ; git commit -m "Move 'libs' files" ; echo mkdir -p src/module git mv configuration_store.* endstops.* planner_bezier.* planner.* printcounter.* speed_lookuptable.h stepper_indirection.* stepper.* temperature.* src/module/ git add . ; git commit -m "Move 'module' files" ; echo mkdir -p src/module/thermistor #find . -name "thermistortable*" -exec echo git mv {} $(echo "{}" | sed "s/\.\//src\/module\/thermistor\//" | sed "s/table//") ';' git mv thermistortable_1.h src/module/thermistor/thermistor_1.h git mv thermistortable_2.h src/module/thermistor/thermistor_2.h git mv thermistortable_3.h src/module/thermistor/thermistor_3.h git mv thermistortable_4.h src/module/thermistor/thermistor_4.h git mv thermistortable_5.h src/module/thermistor/thermistor_5.h git mv thermistortable_6.h src/module/thermistor/thermistor_6.h git mv thermistortable_7.h src/module/thermistor/thermistor_7.h git mv thermistortable_8.h src/module/thermistor/thermistor_8.h git mv thermistortable_9.h src/module/thermistor/thermistor_9.h git mv thermistortable_10.h src/module/thermistor/thermistor_10.h git mv thermistortable_11.h src/module/thermistor/thermistor_11.h git mv thermistortable_12.h src/module/thermistor/thermistor_12.h git mv thermistortable_13.h src/module/thermistor/thermistor_13.h git mv thermistortable_20.h src/module/thermistor/thermistor_20.h git mv thermistortable_51.h src/module/thermistor/thermistor_51.h git mv thermistortable_52.h src/module/thermistor/thermistor_52.h git mv thermistortable_55.h src/module/thermistor/thermistor_55.h git mv thermistortable_60.h src/module/thermistor/thermistor_60.h git mv thermistortable_66.h src/module/thermistor/thermistor_66.h git mv thermistortable_70.h src/module/thermistor/thermistor_70.h git mv thermistortable_71.h src/module/thermistor/thermistor_71.h git mv thermistortable_75.h src/module/thermistor/thermistor_75.h git mv thermistortable_110.h src/module/thermistor/thermistor_110.h git mv thermistortable_147.h src/module/thermistor/thermistor_147.h git mv thermistortable_998.h src/module/thermistor/thermistor_998.h git mv thermistortable_999.h src/module/thermistor/thermistor_999.h git mv thermistortable_1010.h src/module/thermistor/thermistor_1010.h git mv thermistortable_1047.h src/module/thermistor/thermistor_1047.h git mv thermistortables.h src/module/thermistor/thermistors.h git add . ; git commit -m "Move 'thermistor' files" ; echo mkdir -p src/pins git mv pins* src/pins/ git add . ; git commit -m "Move 'pins' files" ; echo mkdir -p src/sd git mv Sd* cardreader.* src/sd/ git add . ; git commit -m "Move 'sd' files" ; echo mkdir -p src/gcode/calibrate git mv M100_Free_Mem_Chk.cpp src/gcode/calibrate/M100.h git add . ; git commit -m "Temporary stash of 'M100'" ; echo mv platformio.ini ../ mv -f Makefile src/ cd .. git add . ; git commit -m "Move build files into place" ; echo # # Bring in patched refactor changes # git diff HEAD bugfix-2.0.x-new-layout | git apply git add .gitignore .gitattributes .travis.yml README.md buildroot git commit -m "Support file updates" ; echo cd Marlin/src git add HAL ../frameworks ; git commit -m "HAL updates" ; echo git add config ; git commit -m "Config updates" ; echo git add core ; git commit -m "Core updates" ; echo git add inc ; git commit -m "Inc updates" ; echo git add libs ; git commit -m "Libs updates" ; echo git add feature ; git commit -m "Feature file updates" ; echo git add lcd/language ; git commit -m "Language updates" ; echo git add lcd ; git commit -m "LCD updates" ; echo git add module ; git commit -m "Module updates" ; echo git add pins ; git commit -m "Pins updates" ; echo git add sd ; git commit -m "SD file updates" ; echo git add gcode ; git commit -m "G-code file updates" git add ../Marlin.ino Marlin.* ; git commit -m "Main controller updates" cd ../.. git add . ; git commit -m "Build file updates" ; echo ```

[Bergerac56]

Hello, Using the last bugfix 2.0.x new layout and PlatformIO (which, btw, is not my best friend yet ;-) ), I was able to start testing with the Re-ARM. Is there a specific place to come back with questions/observations/issues?

Thanks

[Roxy-3D]

7076 has most of the activity...

[Bergerac56]

Thanks Roxy

[TGMods]

@thinkyhead I just tried the new layout on my Re-ARM and miniVIKI display unfortunately it failed to compile because Platformio was unable to find M250.h. I extracted gcode_M250() and turned it into M250.h. I followed the same layout of the other gcode .h files. Running a build with the new M250.h compiled perfectly (apart from a few warnings). Thank you for all your hard work and I appreciate all the effort and the hours you are putting into this project.

[thinkyhead]

Thanks, @TGMods! I will make the patch asap! Evidently, it is there, just the #include path was wrong.

Current status: Starting to organize G-codes and Marlin main functions into more logical code units. The last commit I made moves all the motion functions out of Marlin.cpp to their own file in module/motion.cpp and motion.h. I will continue to let the dependencies between functions and data guide me in this process until Marlin.cpp contains little else but the main controller logic. Then we'll have something that looks more like a customary application. With the addition of Broadcaster/Listener objects in an upcoming iteration, a lot of Marlin's faux dependencies will also fall away.

[p3p]

@thinkyhead I notice you instantiate your static classes (you often call them singletons but they don't really meet that criteria as what your really doing is just creating a namespace, which is not a bad thing) why not just access them with the :: notation?

[Roxy-3D]

All of the 'singletons' have storage associated with them. I'm not a C++ expert but where would the storage be located when the :: notation is used? Instantiating it declares the storage, right?

[thinkyhead]

@p3p It's true that essentially they are being made into "singleton" classes to group the data and methods together, and to gain some of the semantic sugar of being classes, such as the public and private accessors. While it is definitely possible to divide the functions into more units and assign namespaces, I find that class headers tend to be more organized, are good places to put the bulk of the Doxygen content, and —at least in the interim— provide a nice high-level overview of the machinery at play.

I'm currently in the process of moving obvious function/data groups into .cpp/.h files, but not yet wrapping them into any classes or namespaces. Also one-by-one I'm renaming gcode files like "M200.h" to "M200.cpp", adding the needed "#include" lines, and so on, so that they compile successfully. The code is congealing through this process, and I will push the new commits when it is more stable.

When I get further along in this process, the code will —if nothing else— be organized far more closely to what it ought to be, so whether we choose to use namespaces or continue with the "singleton" approach, the data and functions will have been grouped into units that make some sense.

Sorry if the linked branch is in a broken state at any given moment. Check the "commits" tab to see that it is at least passing Travis CI before downloading the current ZIP. I will soon post an additional branch whose HEAD will only ever point to a working state.

where would the storage be located when the :: notation is used?

@Roxy-3D Namespaces only affect scope, not storage, so you simply have to specify the namespace when you refer to a variable, object instance, class, etc., which has been defined within the namespace. For example...

// Declare a class within a namespace
namespace Marlin {
  bool some_value;

  class ObjectManager {
    public:
        void DoSomething(bool yes) {}
    };
    void Func(ObjectManager) {}
}

// Use 'using' to bring 'Marlin' into scope
using namespace Marlin;
ObjectManager mgr;  
mgr.DoSomething(some_value);  
Func(mgr);  

// Use fully-qualified namespace (in root namespace)
Marlin::ObjectManager mgr;
mgr.DoSomething(Marlin::some_value);
Marlin::Func(mgr);

// Use 'using' to bring 'Marlin::ObjectManager' into scope
using Marlin::ObjectManager;
ObjectManager mgr;
mgr.DoSomething(false);

[p3p]

All of the 'singletons' have storage associated with them. I'm not a C++ expert but where would the storage be located when the :: notation is used? Instantiating it declares the storage, right?

@Roxy-3D A static variable in a class is defined (stored) somewhere else, it's basically the same as extern.

Class Header:

class StaticClass {
public:
  static int static_variable;
  static int whats_the_value() {
    return static_variable;
  }
}

without this definition somewhere else (ie the class implementation) it will not link: int StaticClass::static_variable = 0; So as long as a class is completely statically defined you don't need to instantiate, you access everything on the class rather than an instance of the class.

StaticClass::static_variable = 5;
int value = StaticClass::whats_the_value()

The above can actually be defined with an identical API but with namespaces instead. header:

namespace StaticClass {
  extern int static_variable;
  int whats_the_value();
}

implementation:

namespace StaticClass {
  int static_variable = 0;
  int whats_the_value() {
    return static_variable;
  }
}

@thinkyhead I wasn't questioning your use of static classes they come in handy, just that you instantiate them instead of using them directly, its not wrong I was just curious. Although I noticed a none static method when I skimmed through, so that would explain it.

[thinkyhead]

Thanks for the additional clarification. I forget that it's unusual to instantiate a singleton, especially as it's only for the sake of getting to use the slightly neater dot-notation rather than ::, which has been known to cause bleeding from the eyeballs.

Fun fact: No matter how many Planner objects you create, storage won't change, as they will all be interchangeable and refer to the same data. So, yeah, creating the instance is superfluous… except for the dot thing.

I'd be open to doing a global search-and-replace on the uses of planner.(\w\w+) with Planner::$1 (etc.) at some point and dropping the instances. For the time-being, I'll keep using the dots because it's going to be quicker for this process.

---

Quick update: I've encapsulated the motion, queue, and FWRETRACT functions into separate compilation units. The motion functions are grouped with the standard "modules." The queue functions are those that feed commands to the command queue from all sources.

In conjunction with these changes, the process_next_command.h file is now part of gcode.cpp, making the GcodeSuite class the official dispatcher for individual commands that come from the queue. So, already we can see more clearly the division of labor between these units.

Some of the G-code .h files have been converted to .cpp also, including the FWRETRACT, I2C position encoder, and some of the temperature G-codes. My work branch will be:

https://github.com/thinkyhead/Marlin/commits/bugfix_refactor_work

I'm running tests until they pass, and will post the updated branch soon.

[Roxy-3D]

@Roxy-3D A static variable in a class is defined (stored) somewhere else, it's basically the same as extern.

Thank you... You didn't need to type anything more. Everything makes sense!!!

[thinkyhead]

I've pushed the newest refactor code that passes Travis CI to the bugfix-2.0.x-new-layout branch. At this point I consider the refactor to be about halfway done, with many things still in rough form. But as the process goes along, it's getting clearer where code should be placed. (This thing pretty much refactors itself.)

I've intentionally avoided using #include more than needed for any individual file, to self-document what the dependencies are and, presumably, to speed up compilation. In the first pass I'm leaving everything in plain C form. I'll convert any things that should be classes after the organizational work is done.

What's Done

• A loose root hierarchy for different types of code
• Breakup of Marlin.cpp into several units (still being shuffled)
• Grouping of all RGB(W) LED code in feature/leds
• Consolidation of bed leveling in feature/bedlevel
• Some delta code moved to module/delta.cpp*
• Consolidation of FWRETRACT into a feature

Agenda

• Go camping and swim in a lake
• Consolidate remaining kinematics code
• Consolidate remaining features in Marlin.cpp
• Convert all remaining gcode .h files to .cpp files
• Clean up trailing spaces, tabs, etc.
• Refactor LCD code and main controller code

I can probably beat my outlined schedule, since progress will accelerate, so expect the bulk of the work to be done by Tuesday. In the meantime, please check out the code evolution. You should be able to see where it's headed and how far there is yet to go. The most pleasing part is having a Marlin.cpp that's only ~3300 lines long, and soon to be smaller still…

[thinkyhead]

Progress continues! I pushed some updates yesterday and the newest batch just now. The project passes Travis testing, but let me know if there are any compile errors with your configuration and architecture and I'll patch it.

There are still many G-codes to be moved into .cpp files. At this point I tend to favor naming the files according to their feature name, where possible, and grouping all related G-codes in these files rather than having files named by G-code. On the positive side, this would reduce the number of files while still keeping them relatively small, while allowing for more use of inline rather than extern helper functions. On the other hand, this leads to…

1. Obscurity. Global search will still be needed to find specific G-code handlers, just as it is now. With G-code-based naming they can be found by poking through the file list. But this is no panacea anyway, since you still need to know where to look.

2. Will requires careful naming. Not a big caveat since the role of module and feature files is already pretty clear without pedantic names. Marlin's components are pretty well established, so naming by feature / module / etc. works ok so far.

Whether naming files by G-code or by feature, it'll be possible to excise unnecessary code from Marlin for a custom build or fork by simply removing files/folders from the project, making distributions potentially very small.

---

In my next session I'll take some of the remaining G-code .h files that belong together, group them into files by feature / module, and see whether it leads to a cleaner result than the current "M211-M213.cpp" approach. I expect the update I push tomorrow will be ~90% complete, with today's being ~70%.

[Bergerac56]

@thinkyhead . A quick compile with PIO for Re-ARM gives me this error with babystepping activated:

Marlin\src\lcd\ultralcd.cpp: In function 'void lcd_babystep_zoffset()':
Marlin\src\lcd\ultralcd.cpp:1023:25: error: 'class Planner' has no member named 'abl_enabled'; did you mean 'autotemp_enabled'?
if (planner.abl_enabled)

It compiles fine with babystepping desactivated.

With PIO for Mega2560, same results: Does not compile with babystepping activated but well without. I did not investigate why yet.

config.zip

[GMagician]

Enabling BLTOUCH on a Mega2560 gives:

sketch\src\Marlin.cpp: In function 'void setup()':
sketch\src\Marlin.cpp:1649:34: error: 'bltouch_command' was not declared in this scope
     bltouch_command(BLTOUCH_RESET);
                                  ^
sketch\src\Marlin.cpp:1650:30: error: 'set_bltouch_deployed' was not declared in this scope
     set_bltouch_deployed(true);
                              ^

[p3p]

Well with such a major refactor of the code at least we get an overview of what may be missing from the Travis tests.

[thinkyhead]

Thanks for pointing out those missing tests. I'll add them and catch more issues on the next round. I will post an update shortly…

[thinkyhead]

Posted. The current HEAD contains the travis tests, more cleanup, and continued consolidation. I grouped the G-codes for bedlevel into individual files. Each one defines the GcodeSuite:: methods it needs. If this scheme is followed for the rest of the code, there should end up being maybe a dozen G-code named files, with the rest named by feature.

@p3p — Chris, your suggestion of thinking forward to developing standard APIs and standard approaches to extending Marlin is in the back of my mind as I go through this process. There will be a second sweep after this "consolidation" step, to apply classes and/or namespaces where appropriate. After that we can get into API concepts in earnest. At some point —let's say milestone 2.0.1— it would be good to add a broadcaster-listener system. Perhaps by 2.0.5 we'll have a near drop-in method to add new features. A general API for user interface (LCD, keypad, etc.) and possibly a simplified menu system would be another interesting challenge. At the moment it must be hard to yank out the current LCD code and replace it entire, but we can continue to work out ways to eliminate integrated dependencies.

I haven't compared the new build size of equivalent configurations (e.g., using bugfix-1.1.x) but I expect that due to more static linkage the new binary size will be a little larger. I'll do a comparison as soon as the rest of the gcode folder is patched up. I don't expect a big change in performance, in any case.

As I get closer to completion I'll post a summary of the file layout and try to justify the files, folders, and locations. As a work in progress, we'll decide which things to re-shuffle, and with one or two more revisions it should be ready to supplant the bugfix-2.0.x branch for continued HAL development!

[Bergerac56]

Last bugfix does not compile with PIO for re-arm (files used as such):

Linking .pioenvs\Re-ARM\firmware.elf
.pioenvs/Re-ARM/src/src/core/serial.o: In function `serialprintPGM(char const*)':
C:\Users\olivi\Desktop\Marlin-bugfix-2.0.x-new-layout BFNL08/Marlin\src\core\../inc/../HAL/HAL_LPC1768/serial.h:122: multiple definition of 
`serialprintPGM(char const*)'
.pioenvs/Re-ARM/src/src/HAL/HAL_LPC1768/arduino.o:C:\Users\olivi\Desktop\Marlin-bugfix-2.0.x-new-layout BFNL08/Marlin\src\HAL\HAL_LPC1768/se
rial.h:122: first defined here
collect2.exe: error: ld returned 1 exit status
*** [.pioenvs\Re-ARM\firmware.elf] Error 1

[TGMods]

I've just tried a couple of trial builds, one for my Re-ARM and miniVIKI which passed using my basic configuration. The second build was for my Azteeg X3 Pro and VIKI2 display which failed, both using Platformio and Arduino IDE. I tried a second build for the Azteeg using the RRD display which passed in Arduino IDE (I didn't try it using Platformio). The only change to my configuration was to change the definition for my display

The following is just a small sample of the very long VIKI2 error report.

In file included from C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\../inc/../HAL/HAL_AVR/HAL_AVR.h:46:0,

                 from C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\../inc/../HAL/HAL.h:81,

                 from C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\../inc/MarlinConfig.h:28,

                 from C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\ultralcd.cpp:23:

C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\dogm/ultralcd_st7565_u8glib_VIKI.h: In function 'void ST7565_SWSPI_SND_8BIT(uint8_t)':

C:\Users\Sally\AppData\Local\Temp\arduino_build_903296\sketch\src\lcd\../inc/../HAL/HAL_AVR/fastio_AVR.h:89:34: error: 'DIODOGLCD_SCK_RPORT' was not declared in this scope

I felt it was important to try builds for both my boards as we will all be using Marlin 2 in the future.

[thinkyhead]

@TGMods The cause of that error would be that DOGLCD_SCK isn't defined. We'll have to poke around and see how that's supposed to work. Do you see the same error when compiling the bugfix-2.0.x branch under the old file layout?

@Bergerac56 I'll check on that re-definition of serialprintPGM and see if it needs a patch. Be sure to do a clean between builds, as sometimes those linker errors are due to an old .o file hanging around.

[thinkyhead]

"'DIO[pin]_RPORT' was not declared in this scope."

@TGMods — Indeed, the DOGLCD_SCK pin isn't defined in the pins_AZTEEG_X3_PRO.h file, nor the pins_RAMPS.h file which it includes. The pin would need to be defined in one of these files for the display to work. The same goes for any other pins it's throwing errors about.

If you can figure out the pins that work for your setup, let me know and I will add them to the X3 pins file.

[thinkyhead]

@Bergerac56 Apparently serialprintPGM is being defined by the HAL for Re-ARM, even though it's a more "high-level" function. I'll remove the extra definition and localize the implementation difference in core/serial.cpp. It should be ready by the time this comment is 30 minutes old.

[TGMods]

@thinkyhead This is the first time I've tried to build any branch of Marlin 2 for the Azteeg. When I connected the VIKI2 to the Azteeg I followed page 3 of the pdf in the attached zip file.

Viki2_Wiring_Diagram.zip

[TGMods]

@thinkyhead I've just tried building a version of bugfix-2.0.x for my Azteeg and it also fails for problems with DOGLCD_SCK

I also tried building the latest 1.1.x branch for the Azteeg and VIKI2 which had no problems, if that helps.

[TGMods]

@thinkyhead I've just compared the relevant pins files in version 1.1.x and new-layout and there are no differences apart from all the spi pin definitions which have been moved into the HAL.

[TGMods]

@thinkyhead I've been messing around with the Azteeg pins file and I inserted #define DOGLCD_SCK SCK_PIN into this code snippet from pins_AZTEEG_X3_PRO.h.

#if ENABLED(VIKI2) || ENABLED(miniVIKI)
  #undef SD_DETECT_PIN
  #define SD_DETECT_PIN    49   // For easy adapter board
  #undef BEEPER_PIN
  #define  BEEPER_PIN      12   // 33 isn't physically available to the LCD display
#else
  #define STAT_LED_RED_PIN 32
  #define STAT_LED_BLUE_PIN 35
#endif

The problem with this is that it throws up another error concerning DOGLCD_MISO and adding the definition for MISO also throws up a MOSI error. Adding all three definitions cures the problem. While I can sort out the problem for my configuration, it won't sort out the problem for any other board and VIKI2 configuration. I wonder if adding the DOGLCD_ definitions to the spi pin definitions in the HAL would be a sensible move.

[Bergerac56]

@thinkyhead Sorry to come back late, but I think we are not on the same continent ;). It compiles fine now, even with my complex configs. Except for the Advanced Pause feature, which still fails (never compiled fine with the new layout)

Compiling .pioenvs\Re-ARM\src\src\core\serial.o
In file included from Marlin\src\Marlin.cpp:468:0:
Marlin\src\gcode/sdcard/M25.h: In function 'void gcode_M25()':
Marlin\src\gcode/sdcard/M25.h:31:45: error: 'enqueue_and_echo_commands_P' was not declared in this scope
enqueue_and_echo_commands_P(PSTR("M125")); // Must be enqueued with pauseSDPrint set to be last in the buffer
^
*** [.pioenvs\Re-ARM\src\src\Marlin.o] Error 1

[fiveangle]

This shouldn't be picking up the HAL content at all when compiling for AVR, should it ?

In file included from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/../../inc/MarlinConfig.h:37:0,
                 from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/LPC1768_Servo.cpp:63:
/var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/../../inc/Conditionals_post.h:1009:0: warning: "NOT_A_PIN" redefined
     #define NOT_A_PIN 0 // For PINS_DEBUGGING
 ^
In file included from /Applications/Arduino 1.8.3-teensy.app/Contents/Java/hardware/teensy/avr/cores/teensy/Arduino.h:2:0,
                 from /Applications/Arduino 1.8.3-teensy.app/Contents/Java/hardware/teensy/avr/cores/teensy/elapsedMillis.h:29,
                 from /Applications/Arduino 1.8.3-teensy.app/Contents/Java/hardware/teensy/avr/cores/teensy/WProgram.h:26,
                 from /Applications/Arduino 1.8.3-teensy.app/Contents/Java/hardware/teensy/avr/cores/teensy/Arduino.h:1,
                 from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/../../inc/../HAL/HAL_AVR/HAL_AVR.h:38,
                 from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/../../inc/../HAL/HAL.h:81,
                 from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/../../inc/MarlinConfig.h:32,
                 from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/arduino_build_798104/sketch/src/HAL/HAL_LPC1768/LPC1768_Servo.cpp:63:
/Applications/Arduino 1.8.3-teensy.app/Contents/Java/hardware/teensy/avr/cores/teensy/pins_arduino.h:43:0: note: this is the location of the previous definition
 #define NOT_A_PIN 127
 ^

…plus reams and reams of more errors, then eventual failure.

Is there something more to do besides port configs into the ones at the top level and select board in AIDE183 ? (Submline still won't work with Deviot because the plug in keeps blowing away the src_dir, but you're aware of that)

https://github.com/fiveangle/Marlin/tree/bf2ng/Marlin https://github.com/fiveangle/Marlin/blob/bf2ng/Marlin/Configuration.h https://github.com/fiveangle/Marlin/blob/bf2ng/Marlin/Configuration_adv.h

[Bergerac56]

The last bugfix 2.0.x (from 5 hours ago) compiles fine for Re-ARM, included advanced_pause. But CASE_LIGHT_ENABLE does not compile anymore (Previous version of bugfix was fine for that. It also does not compile for MEGA2560):

Compiling .pioenvs\Re-ARM\src\src\core\serial.o
Marlin\src\Marlin.cpp: In function 'void setup()':
Marlin\src\Marlin.cpp:764:5: error: 'case_light_on' was not declared in this scope
case_light_on = CASE_LIGHT_DEFAULT_ON 
^~~~~~~~~~~~~
Marlin\src\Marlin.cpp:765:5: error: 'case_light_brightness' was not declared in this scope
case_light_brightness = CASE_LIGHT_DEFAULT_BRIGHTNESS;
^~~~~~~~~~~~~~~~~~~~~
Marlin\src\Marlin.cpp:766:23: error: 'update_case_light' was not declared in this scope
update_case_light();
^
*** [.pioenvs\Re-ARM\src\src\Marlin.o] Error 1
 [ERROR] Took 38.76 seconds

[TGMods]

@thinkyhead I just tried building the latest version of the new layout for my Azteeg and VIKI2 and foun that it still fails for DOGLCD_SCK. I added these lines in the DOGLCD section of Configuration_adv.h and it compiled just fine. I also did a trial build of my Re-ARM and miniVIKI using the same additions and everything compiled without problems for my very basic configuration.

  // VIKI2 and miniVIKI require DOGLCD_SCK and DOGLCD_MOSI to be defined.
  #ifdef VIKI2
    #define DOGLCD_SCK SCK_PIN
    #define DOGLCD_MOSI MOSI_PIN
  #endif

  #ifdef miniVIKI
    #define DOGLCD_SCK SCK_PIN
    #define DOGLCD_MOSI MOSI_PIN
  #endif

[thinkyhead]

@Bergerac56 I've just patched caselight now, so give it another try.

@TGMods Thanks for continuing to follow up! I'll insert those lines into the latest build and as we get back to working on the HAL I'm sure we'll figure out what's going on with those pins.

---

I've been focused on finishing up the last of the G-codes and then moving the last remnants out of the Marlin.cpp file, so the latest build is nearly complete (as far as the first stage of reorganization goes).

G-code Filenames

I opted to keep the filenames with G-codes because it offers a lot of convenience, and it doesn't matter to the class structure. The particular categorization is definitely flexible. "Should M48 go into calibration or into probe?" We can decide on these things, especially now that the G-codes are in .cpp files and they can be moved around more freely. (The #include paths only need to change if they go up or down a level.)

When we move the implementations of some (or all) G-codes into the pertinent classes, they can still be defined in individual G-code named files and kept in the same locations. They'll just have a different class prefix on them (Temperature:: instead of GcodeSuite::). The process_next_command function can just call them directly in their classes, or we can inline the calls (as with I2CPEM currently). It makes sense to move G-code handlers to their associated classes, encapsulate functionality, and give them the same access rights as their features. The only "special" thing about a G-code handler is that it calls parser for its parameters.

So… Please give this another test when you can, and if we clear up all the bugs, then finally we can make this the new bugfix-2.0.x branch and continue with the HAL.

[Roxy-3D]

"Should M48 go into calibration or into probe?"

M48 uses the probe to get data... But it doesn't contribute anything to the probe class. M48's whole purpose in life is calibration.

[TGMods]

@thinkyhead I've refined the code to

  // VIKI2 and miniVIKI require DOGLCD_SCK and DOGLCD_MOSI to be defined.
  #if ENABLED(VIKI2) || ENABLED(miniVIKI)
    #define DOGLCD_SCK SCK_PIN
    #define DOGLCD_MOSI MOSI_PIN
  #endif

[thinkyhead]

@Bob-the-Kuhn @p3p @MarlinFirmware/32bit-maintainers @MarlinFirmware/general-maintainers …

https://github.com/MarlinFirmware/Marlin/commits/bugfix-2.0.x-new-layout

I've made great effort not to mess with the HAL too much, so it should be functionally at the point we left it when I started this branch. I understand there are some patches pending to fix issues found in the last couple of weeks, and I want to make sure we can get the very latest HAL code in place as soon as possible along with this refactor.

The easiest thing would be to point me to the latest HAL code and allow me to merge the changes in as much as possible in front of this pile of commits (which I can squash down to only a few eventually), so that I can rectify any changes made afterward. You probably found the same few issues that I found. All the HAL changes are in one commit, so please review: https://github.com/thinkyhead/Marlin/commit/ceb64687857905b569c25c30b46126331ff57e63?w=1

I realize this branch is still not quite complete. There are some dangling #include lines in Marlin.cpp that are no longer needed. As things have shifted around, there may be some Marlin.h includes that no longer pertain. I've labeled some includes with the variables they bring in to help catch these.

So… the next step is to certify that this can take over where 2.0.x-bugfix left off. At that point, we can continue to figure out the HAL and hardware side, I will continue to clean up and refine the code organization at the "feature" level, and so on. It would make sense to release a Marlin 2.0 "beta" as soon as we have it working for AVR and mostly-working for the 32-bit platforms, rather than wait for it to be "perfect." But, I anticipate a few more weeks before then…

[thinkyhead]

Thanks, @TGMods — It feels like a "hack" since we don't need it for most platforms, but hey it works, so I'll leave it in for now so that we can press forward.

[TGMods]

@thinkyhead I agree it is a bit of a "hack" but I just can't figure out why Marlin 2 doesn't work the same way as Marlin 1.1.x. I doubt that many users are going to be using a VIKI2 or a miniVIKI but there are quite a lot of "pins" files that reference them and I suspect that that will also throw up problems.

[thinkyhead]

I just did a test build to compare Marlin 2.0 build size to Marlin 1.1 using my test configuration, which just enables as much as possible all at once.

• Marlin 1.1 : 196426 PROM, 5852 SRAM
• Marlin 2.0 : 189998 PROM, 5812 SRAM

196426 - 189998 = 6,428 bytes smaller, and 40 bytes less SRAM.

This is very preliminary. I'll have to compare with some more ordinary configs and see if the trend continues.

[thinkyhead]

there are quite a lot of "pins" files that reference them and I suspect that that will also throw up problems.

@TGMods Maybe the ultimate solution is to make an assumption at the pins.h level where other dangling pin settings are applied. It makes sense that if these pins haven't been assigned to something else —including -1 to specifically disable them— that the DOGLCD pins should revert to SCK_PIN and MOSI_PIN since these are almost always passed through to the LCD to conserve the i2c bus, and they only apply when using a display where they would be required.

[tcm0116]

6,428 bytes smaller

I wonder if that's because there's less inlining going on

On Sep 18, 2017, 5:50 PM, at 5:50 PM, Scott Lahteine notifications@github.com wrote:

I just did a test build to compare Marlin 2.0 build size to Marlin 1.1 using my test configuration, which just enables as much as possible all at once.

• Marlin 1.1 : 196426 PROM, 5852 SRAM
• Marlin 2.0 : 189998 PROM, 5812 SRAM

196426 - 189998 = 6,428 bytes smaller, and 40 bytes less SRAM.

This is very preliminary. I'll have to compare with some more ordinary configs and see if the trend continues.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/MarlinFirmware/Marlin/issues/7622#issuecomment-330378066

[thinkyhead]

@tcm0116 I suspect there may be less inlining in some cases. Another possibility is that with all the G-codes split into separate units, the linker can decide what code to place closer to other code to get short jumps and branches, whereas with a 13,000 line Marlin_main.cpp file the compiler isn't able to shuffle the functions around in relation to each other.

[tcm0116]

@thinkyhead that's an interesting concept. I wouldn't think the organization of the code into compilation units would necessarily affect how the linker organizes the final assembly since all of the object files are provided to the linker at once. I wish I knew more about how the linking process works.

On Sep 19, 2017, 3:37 PM, at 3:37 PM, Scott Lahteine notifications@github.com wrote:

@tcm0116 I suspect there may be less inlining in some cases. Another possibility is that with all the G-codes split into separate units, the linker can decide what code to place closer to other code to get short jumps and branches, whereas with a 13,000 line Marlin_main.cpp file the compiler isn't able to shuffle the functions around in memory.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/MarlinFirmware/Marlin/issues/7622#issuecomment-330665973

[thinkyhead]

@tcm0116 I know a little about "linkage" from having worked with assemblers in the Amiga days and then some of the C and C++ compilers along the way. My hope is that things are not done in exactly the "rote" way that they were done from the beginning, because there are so many opportunities to optimize once you have the code compiled into some intermediate form.

In the case of both assemblers and compilers, you get intermediate .o files that list all their unresolved symbols in a table, along with the offsets into the .o file (or into one or more CODE blocks). The primary job of the linking process is to aggregate all the unresolved symbols and fill them in to produce the final binary. It also decides where the .o files go in relation to one another in memory at this point. (Hopefully not just alphabetically!) Linker errors occur when there are references to symbols which are not defined, or which don't have the right kind of "linkage" (e.g., static, etc.).

If linkers still work exactly that way and haven't added more intelligence I would be very surprised!

One very common optimization is to use a short jump or branch when possible to save some bytes. Or, if some function in another unit only calls another function, inline it instead. If the intermediate .o representation is more flexible, so that the code within it can shift as shorter code replaces it, then it offers more opportunities to apply these kinds of optimizations.

The pessimistic side of my brain is saying "no, compiler writers would prefer not to do that, have allowed .o files to remain a hard standard, and continue doing things the 'correct' way, as they were taught in Compilers 101." So… darn it, now I'll have to look at the source code for gcc and check my suspicions!

Anyway, those are probably small compared to things the compiler can do within functions, especially with optimization level -O3. For example, the gcc compiler can replace a call to a function with a determined result with just the result, such as factorial(10) with 3628800 (as long as the function is defined in the same unit). As I understand it, the --whole-program flag is meant to effectively group all code into a single .o file and compile it as a single unit to take advantage of all possible optimizations.

[fiveangle]

I do enjoy your stream-of-consciousness sidebars on occasion Scott :)

I'm now envisioning a compiler dev devout to the K&R religion rebelliously slipping --whole-burrito in as an alternate syntax to the --whole-program flag, while his Green Hills boss is none-the-wiser. comp.compilers havoc ensues...

[Bergerac56]

@thinkyhead . The last bugfix still does not compile for re-arm and mega2560.

Compiling .pioenvs\Re-ARM\src\src\core\serial.o
Marlin\src\Marlin.cpp: In function 'void setup()':
Marlin\src\Marlin.cpp:751:23: error: 'update_case_light' was not declared in this scope
update_case_light();
^
*** [.pioenvs\Re-ARM\src\src\Marlin.o] Error 1
 [ERROR] Took 12.96 seconds

[rafaljot]

Ohh, I missed this thread and I started new one for this issue. [https://github.com/MarlinFirmware/Marlin/issues/7687]() And general question. What do you think, which graphic LCD could be (or it is) most common one, recomended for 32bit Marlin Firmware?

[Bergerac56]

Two new potential issues: Re-Arm / last bugfix: 1> LCD/PREPARE/ UBL TOOLS / BUILD MESH PLA: Nozzle and bed heat but printer stops when temperature is reached (heaters return to 0). The mesh is not built. The only way to build the mesh is with a G29 P1via pronterface for ex. 2> When printing a cylindric element, the motors start to shake and the printer slows after having printed the first layer. The "wall" is not perfect (crimpy?). I tried the same test with a box (cube) and the problem does not occur

Here is the stl file and gcode used: test.zip

[Roxy-3D]

And general question. What do you think, which graphic LCD could be (or it is) most common one, recomended for 32bit Marlin Firmware?

For the 8-Bit AVR's... The 20x4 LCD Panels require significantly less program memory and CPU cycles compared to the Graphical LCD's. But right now, on the 32-bit work, the Graphical LCD's have better support (and code size and CPU cycles don't matter as much there).

[thinkyhead]

Thanks for the heads-up on the compile issue with CASELIGHT, @Bergerac56. I will post the updated build as soon as it passes Travis.

I've also attempted to make M100 and the hex_print_routines compatible with 32-bit. On 32-bit, it may be fine to make the hex print routines just use sprintf(_hex, "%02x") (etc.) to generate these strings. But for now, it will be good to see how performant our AVR code is on the 32-bit boards. Considering all our optimizations, it should be a lot more efficient than Smoothieware.

Two new potential issues:

@Bergerac56 — Are you seeing these in the current -new-layout build, or just in the current bugfix-2.0.x? I would expect there to be some slight differences between them, as I've patched some small issues. But maybe not these. The issue with the cylinder is interesting. Maybe it has to do with very small moves. Does the issue appear using G2/G3 commands? What about with a cylinder that has low resolution on its facets?