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!
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.
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…
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…
.h
to .cpp
file and compile to see where it fails, citing dependencies.extern
and #include
lines to resolve the dependencies.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()
).
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.
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.
@MarlinFirmware/32bit-maintainers
To convert bugfix-2.0.x
to the new layout in a fresh branch…
Conversion Script
#!/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
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
Thanks Roxy
@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.
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.
@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?
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?
@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);
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.
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 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!!!
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.
Marlin.cpp
into several units (still being shuffled)feature/leds
feature/bedlevel
module/delta.cpp
*FWRETRACT
into a featureMarlin.cpp
.h
files to .cpp
filesI 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…
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…
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.
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%.
@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.
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);
^
Well with such a major refactor of the code at least we get an overview of what may be missing from the Travis tests.
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…
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!
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
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.
@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.
"'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.
@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.
@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.
@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.
@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.
@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.
@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
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
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
@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
@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).
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.
"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.
@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
@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…
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.
@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.
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.
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.
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.
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
@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.
@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 lineMarlin_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
@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.
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...
@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
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?
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
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).
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?
I've patched all the reported compiler errors, and M100
should now work in the 32-bit environments, displaying 8-digit hex addresses.
Thank you for your patience and forbearance through the refactoring process. The initial rearrangement is the hardest part, and basically a lot of grunt work, trial-and-error, and cursing the dreaded ❌ every time Travis CI choked.
If there are no objections, I could simply replace the bugfix-2.0.x
branch with the refactored one, then if anyone has patches for the HAL or other parts of the code, you can just make them to the updated codebase. If some are hard to apply, let me know and I don't mind applying them manually, if necessary. Apart from necessary tweaks, most changes to the HAL are entirely spacing-related. Stripping excess whitespace before diffing should produce leaner output.
I'd like to do more testing with the Re-ARM and get my character display working (especially since I don't have a graphical display at the moment). We'll definitely want to have that working with the first release, if it's at all possible. I'll replace the RAMPS on my i3 with the Re-ARM this weekend and get into that, if I don't get too bogged down with bug-fixes.
There are some issues piling up in the queue that will need patches to the 1.1.x code. Some of these are important, so my plan is to take every patch to 1.1.x and also apply it to 2.0.x. As for new features, they can also be added to 2.0.x within the feature-based layout. At this time the internal APIs are mostly identical to 1.1.x, so it should be easy to bring things over during this transitional period.
Hopefully this refactor provides a good starting foundation for the next generation of Marlin and customized forks, with an eye towards modularity. Interested contributors and GitHub members may comment on the new file layout, suggest improvements, and so on.
There will continue to be some rearrangement — for example, adding a cartesian.cpp
module, splitting up the three different forms of auto-bed-leveling so they can be seen in their essence, and evaluating whether relocating G-code handlers into their relevant classes is sensible. The motion.cpp
module is a pretty big stew, so that may be split up even further. And the lcd
code needs some work to replace the code-generating includes (rightly frowned-upon as an unnecessary cheat) with proper .cpp
files.
Onward….
If there are no objections, I could simply replace the bugfix-2.0.x branch with the refactored one
Yes. Agreed.
There are some issues piling up in the queue that will need patches to the 1.1.x code. Some of these are important, so my plan is to take every patch to 1.1.x and also apply it to 2.0.x
Yes. Agreed.
There will continue to be some rearrangement — for example,
Probably the best thing to do is let things stabilize for a while. Once development activity gets back to normal let's just contain the refinement to one well defined area at a time. Otherwise everything grinds to a halt. This re-organization was needed but it was very disruptive. Things need to stabilize.
My guess is if we replace the current bugfix-2.0.x branch with the re-organized one and start pointing people to it... It will take a few weeks to get all the hiccups out of it and back under control. That is OK! And then, further refinement of various sub-systems can continue.
@thinkyhead . All my tests are done with bugfix 2.0.x New layout.
I just gave a try to the last version:
Marlin\src\feature\caselight.cpp: In function 'void update_case_light()':
Marlin\src\feature\caselight.cpp:38:44: error: 'USEABLE_HARDWARE_PWM' was not declared in this scope
if (USEABLE_HARDWARE_PWM(CASE_LIGHT_PIN)) {
^
Marlin\src\feature\caselight.cpp:36:11: warning: unused variable 'case_light_bright' [-Wunused-variable]
uint8_t case_light_bright = (uint8_t)case_light_brightness;
^~~~~~~~~~~~~~~~~
*** [.pioenvs\Re-ARM\src\src\feature\caselight.o] Error 1
@thinkyhead I think replacing bugfix-2.0.x is the best option, and getting people testing 2.0 on AVR is very important for a smooth transition to it being the default dev branch. The other platforms need testing too but AVR has to be a priority at this point. (says the man that doesn't have any printers that run AVR)
I think replacing bugfix-2.0.x is the best option, and getting people testing 2.0 on AVR is very important for a smooth transition to it being the default dev branch.
From another perspective... If the outstanding Pull Requests are duplicated and applied to the new bugfix-2.0.x that allows us to freeze bugfix-1.x.x It makes it much easier to say "bugfix-2.0.x is where you should be."
PRs #7699, #7700, #7706, #7701 currently open against bf20ng branch to fix some AVR/Due compile errors found in issues list so could probably be applied before R&R of the new layout branch to bug fix-2.0.x
Probably the best thing to do is let things stabilize for a while.
if we replace the current bugfix-2.0.x branch … and start pointing people to it... It will take a few weeks … That is OK!
I agree. The only last-minute tweaks that I would do at this point would pertain to the compiler errors reported above, and general cleanup. There's no point where it can all be "perfect" but we can keep refining as we go along. The first Marlin 2.0 release can be a little rough around the edges, and we'll get it into more ideal shape by the time we tag 2.0.5.
With that in mind, I'll apply the patches from @fiveangle and any others we need, and update bugfix-2.0.x
forthwith. (The current bugfix-2.0.x
will be backed up as bugfix-2.0.x-backup-21092017
.)
Still not compiling for re-arm with case_light.
I see that USEABLE_HARDWARE_PWM
is only defined for AVR. @Bob-the-Kuhn could probably tell you off the top of his head what the new test should be. I'll see if I can figure it out and add a macro for the other platforms. I think the issue is that we aren't super familiar with the scope of PWM and Timer relationships on each of these new chips.
Here's the USEABLE_HARDWARE_PWM for the LPC1768.
case_light.zip
case_light now compiles BUT I have not been able to test it. I have not been able to build an image that runs from the reorg branch.
It appears that I'm running into some kind of linker or symbol table problem.
I can see the start of several routines in FLASH right where the ELF files says it'll be but the branches to these routines end up somewhere else.
The VTOR register says the vector table starts at 0 but it's actually at 0x0000 4000.
I used PlatformIO to compile the bugfix-2.0.x-new-layout branch from early this evening. The only change I made was to set the board to BOARD_RAMPS_14_RE_ARM_EFB, set the machine name, set the author and set the temp0 sensor to 998.
When I single step from a reset ,the PC is picked up from address 4 which is 0x0000 AAAD so the execution starts at 0x0000 AAAC. The problem is that's not where the reset routine is located. It's actually at 0x0000 41AC. The vector table is actually at 0x0000 4000. When I look at VTOR it's contents are 0x0000 0000.
The object dump of the ELF file and the disassembly in the debugger agree on the 0x0000 4xxx addresses.
Apparently that's just the first problem. When I set the PC to 0x0000 41AC I can follow the execution through the reset routine but when it branches to the SystemInit routine it doesn't go to the right place. Using the objectdump & the debugger I can see the SystemInit code in FLASH right where the ELF file says it should be.
The VTOR register says the vector table starts at 0 but it's actually at 0x0000 4000.
Makes sense, the bootloader lives in the first 16k.
Does the marlin code touch VTOR somewhere?
Otherwise this should be the last location that touches VTOR.
@Bob-the-Kuhn . I tried the last version of Bugfix 2.0.x NL adding the corrections for case_light for re-arm. It compiles now if the option MENU_ITEM_CASE_LIGHT is disabled. EDIT: I tried on my "re-arm" printer and the function works (M355 S0-S1). If MENU_ITEM_CASE_LIGHT is activated, compilation fails:
Marlin\src\lcd\ultralcd.cpp: In function 'void case_light_menu()':
Marlin\src\lcd\ultralcd.cpp:258:41: error: cannot convert 'int*' to 'int16_t* {aka short int*}' for argument '2' to 'void menu_action_settin
g_edit_callback_int3(const char*, int16_t*, int16_t, int16_t, screenFunc_t, bool)'
And:
menu_action_ ## TYPE(__VA_ARGS__); \
^
Marlin\src\lcd\ultralcd.cpp:269:7: note: in expansion of macro '_MENU_ITEM_PART_2'
_MENU_ITEM_PART_2(TYPE, LABEL, ## __VA_ARGS__); \
^~~~~~~~~~~~~~~~~
Marlin\src\lcd\ultralcd.cpp:310:53: note: in expansion of macro 'MENU_ITEM'
#define MENU_ITEM_EDIT_CALLBACK(type, label, ...) MENU_ITEM(setting_edit_callback_ ## type, label, PSTR(label), ## __VA_ARGS__)
^~~~~~~~~
Marlin\src\lcd\ultralcd.cpp:774:7: note: in expansion of macro 'MENU_ITEM_EDIT_CALLBACK'
MENU_ITEM_EDIT_CALLBACK(int3, MSG_CASE_LIGHT_BRIGHTNESS, &case_light_brightness, 0, 255, update_case_light, true);
^~~~~~~~~~~~~~~~~~~~~~~
Marlin\src\lcd\ultralcd.cpp:262:113: error: cannot convert 'int*' to 'int16_t* {aka short int*}' for argument '5' to 'void lcd_implementatio
n_drawmenu_setting_edit_callback_int3(bool, uint8_t, const char*, const char*, int16_t*, ...)'
@thinkyhead Could you also add the solution for filtering temp developped here ? Pid stability ReArn #7585
@Bob-the-Kuhn One more idea with regards to the strange problem you are having. Are you singlestepping right after reset? If so the CPU might be in the boot ROM region.
It is automatically mapped to 0 after each reset. Check the MEMMAP register to verify.
So, I see that bugfix-2.0.x
is now just about leaving the Arduino IDE behind.
The only files visible from the IDE are Marlin.ino
, Configuration.h
, and Configuration_adv.h
.
Unless I'm missing some new IDE configuration.
I am using IDE 1.8.4.
Will PlatformIO compile and send to the Mega?
@Tannoo The Arduino toolchain supports folders but the IDE doesn't appear to display them, thinkyhead moved the configuration files back to the folder containing Marlin.ino in order to make it easy for end users to modify configs and compile, this is about the only use of the Arduino IDE anyway you can't use it for development.. (ok that's only an opinion).
Ok. I will have to load the project in PlaformIO to find the compile errors I'm getting with the IDE.
I will dive into that in the am.
My vote is Arduino can pull in v2.0.0 and compile for the AVR's. And Platform-IO can compile for some of the 32-bit boards and also do AVR's.
The AVR IDE compiles just fine if everything is left as default.
Arduino IDE
@Tannoo I would give a try to PIO even if i found it not so easy to start with (not very stable).
But, then, you find back all your files and can upload to AVR without problem. And, as Roxy says, you can compile for mega and re-arm
Oh yeah. PIO compiles my Re-ARM just fine. I have not used PIO for AVR though.
Everything was fine until the new layout got into bugfix-2.0.x
.
I just checked the last version of bugfix-2.0.x. . I suppose that, since now, it will be the "track to follow". Am I right?
This version compiles fine with PIO for atmega. (As it was the case for bugfix 2.0.x new layout). But still problems to compile with case_light enabled for re-arm.
So, I did get the error sorted and PIO compiled and uploaded to the Mega.
I just checked the last version of bugfix-2.0.x. . I suppose that, since now, it will be the "track to follow". Am I right?
@Bergerac56 bugfix-2.0.x is the the dev branch for 2.0, there have just been varying levels of bleeding edge for the last few weeks,(they were pretty much just thinkyheads working branches), this is now converging again thankfully.
@Roxy-3D I think the aim is to keep Marlin building with Arduino IDE for any platform that supports it, but the Arduino IDE was never designed to develop anything more than simple projects linking together a few libs so we are not worrying about the files/folders not showing up (other than the configs). If PlatformIO (Atom IDE) was a bit more .. seamless .. I would say just switch to that as default for end users but the variability in how easy it to setup (from simple to impossible apparently) that's not an option.
We do have #7707 to worry about though, The Arduino toolchain is limited by the maximum command length of the OS .. with the now longer paths because of the reorganise it has become a problem it seems.
@Bergerac56 - this compiles for me when MENU_ITEM_CASE_LIGHT is enabled. It also fixes the S0, S1 options so they actually do something on Re-ARM.
case_light_2.zip
I'm not able to test the MENU_ITEM_CASE_LIGHT option. My system won't run when I have VICKY2 enabled.
@Bob-the-Kuhn - I tested with the content of your case_light_2 spead on the right places (I think ;) ) and... it does not compile. Message:
Compiling .pioenvs\Re-ARM\src\src\feature\caselight.o
Marlin\src\feature\caselight.cpp: In function 'void update_case_light()':
Marlin\src\feature\caselight.cpp:51:44: error: 'USEABLE_HARDWARE_PWM' was not declared in this scope
if (USEABLE_HARDWARE_PWM(CASE_LIGHT_PIN)) {
^
Marlin\src\feature\caselight.cpp:57:44: error: 'USEABLE_HARDWARE_PWM' was not declared in this scope
if (USEABLE_HARDWARE_PWM(CASE_LIGHT_PIN))
^
*** [.pioenvs\Re-ARM\src\src\feature\caselight.o] Error 1
[ERROR] Took 19.21 seconds
The command "build_marlin_pio ${TRAVIS_BUILD_DIR} ${TEST_PLATFORM}" exited with 0.
0.01s$ restore_configs
The command "restore_configs" exited with 0.
0.02s$ opt_enable NUM_SERVOS Z_ENDSTOP_SERVO_NR Z_SERVO_ANGLES DEACTIVATE_SERVOS_AFTER_MOVE
The command "opt_enable NUM_SERVOS Z_ENDSTOP_SERVO_NR Z_SERVO_ANGLES DEACTIVATE_SERVOS_AFTER_MOVE" exited with 0.
0.01s$ opt_set NUM_SERVOS 1
The command "opt_set NUM_SERVOS 1" exited with 0.
0.02s$ opt_enable AUTO_BED_LEVELING_3POINT DEBUG_LEVELING_FEATURE EEPROM_SETTINGS EEPROM_CHITCHAT
The command "opt_enable AUTO_BED_LEVELING_3POINT DEBUG_LEVELING_FEATURE EEPROM_SETTINGS EEPROM_CHITCHAT" exited with 0.
0.02s$ opt_enable_adv EXTENDED_CAPABILITIES_REPORT AUTO_REPORT_TEMPERATURES AUTOTEMP G38_PROBE_TARGET
The command "opt_enable_adv EXTENDED_CAPABILITIES_REPORT AUTO_REPORT_TEMPERATURES AUTOTEMP G38_PROBE_TARGET" exited with 0.
5.57s$ build_marlin_pio ${TRAVIS_BUILD_DIR} ${TEST_PLATFORM}
Marlin/Marlin_main.cpp: In function 'void gcode_G29()':
Marlin/Marlin_main.cpp:4505:21: warning: unused variable 'abl2' [-Wunused-variable]
int constexpr abl2 = 3;
^
/tmp/cccXj0v9.ltrans0.ltrans.o: In function `process_next_command()':
cccXj0v9.ltrans0.o:(.text+0x22d6): undefined reference to `Servo::move(int)'
cccXj0v9.ltrans0.o:(.text+0x2304): undefined reference to `Servo::read()'
/tmp/cccXj0v9.ltrans1.ltrans.o: In function `set_probe_deployed(bool)':
cccXj0v9.ltrans1.o:(.text+0x1322): undefined reference to `Servo::move(int)'
/tmp/cccXj0v9.ltrans3.ltrans.o: In function `handle_interrupts(timer16_Sequence_t, unsigned int volatile*, unsigned int volatile*)':
cccXj0v9.ltrans3.o:(.text+0x44): undefined reference to `ServoCount'
cccXj0v9.ltrans3.o:(.text+0x68): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0x6a): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0x92): undefined reference to `ServoCount'
cccXj0v9.ltrans3.o:(.text+0xba): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0xbc): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0x112): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0x114): undefined reference to `servo_info'
cccXj0v9.ltrans3.o:(.text+0x13e): undefined reference to `servo_info'
/tmp/cccXj0v9.ltrans3.ltrans.o:cccXj0v9.ltrans3.o:(.text+0x140): more undefined references to `servo_info' follow
/tmp/cccXj0v9.ltrans4.ltrans.o: In function `main':
cccXj0v9.ltrans4.o:(.text.startup+0x510): undefined reference to `Servo::attach(int)'
cccXj0v9.ltrans4.o:(.text.startup+0x518): undefined reference to `Servo::detach()'
cccXj0v9.ltrans4.o:(.text.startup+0x524): undefined reference to `Servo::move(int)'
/tmp/cccXj0v9.ltrans9.ltrans.o: In function `global constructors keyed to 65535_0_G26_Mesh_Validation_Tool.o.3750':
cccXj0v9.ltrans9.o:(.text.startup+0xac): undefined reference to `Servo::Servo()'
collect2: error: ld returned 1 exit status
*** [.pioenvs/megaatmega2560/firmware.elf] Error 1
I tried compiling with all those flags, and it worked with just a few warnings:
Compiling .pioenvs/megaatmega2560/lib/TMC2130Stepper/source/TMC2130Stepper_DRV_STATUS.o
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp: In member function 'uint16_t TMC2130Stepper::microsteps()':
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp:290:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp: In member function 'uint8_t TMC2130Stepper::blank_time()':
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp:308:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp: In member function 'uint8_t TMC2130Stepper::sg_current_decrease()':
.piolibdeps/TMC2130Stepper/src/source/TMC2130Stepper.cpp:331:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
Archiving .pioenvs/megaatmega2560/lib/libServo.a
Compiling .pioenvs/megaatmega2560/lib/TMC2130Stepper/source/TMC2130Stepper_GCONF.o
Indexing .pioenvs/megaatmega2560/lib/libServo.a
Compiling .pioenvs/megaatmega2560/lib/TMC2130Stepper/source/TMC2130Stepper_IHOLD_IRUN.o
Compiling .pioenvs/megaatmega2560/lib/TMC2130Stepper/source/TMC2130Stepper_PWMCONF.o
Compiling .pioenvs/megaatmega2560/lib/LiquidCrystal_I2C_ID576/LiquidCrystal_I2C.o
Archiving .pioenvs/megaatmega2560/lib/libTMC2130Stepper.a
Indexing .pioenvs/megaatmega2560/lib/libTMC2130Stepper.a
Archiving .pioenvs/megaatmega2560/lib/libLiquidCrystal_I2C_ID576.a
Indexing .pioenvs/megaatmega2560/lib/libLiquidCrystal_I2C_ID576.a
Archiving .pioenvs/megaatmega2560/lib/libU8glib_ID7.a
Indexing .pioenvs/megaatmega2560/lib/libU8glib_ID7.a
Linking .pioenvs/megaatmega2560/firmware.elf
Building .pioenvs/megaatmega2560/firmware.hex
Calculating size .pioenvs/megaatmega2560/firmware.elf
AVR Memory Usage
----------------
Device: atmega2560
Program: 71382 bytes (27.2% Full)
(.text + .data + .bootloader)
Data: 2920 bytes (35.6% Full)
(.data + .bss + .noinit)
========================= [SUCCESS] Took 12.91 seconds =========================
@tcm0116 - it may be good to resubmit to see if travis is happy again (could be one of the many fixes applied to bf20 since you last submitted)
@thinkyhead - is it possible to move forward with #7695 and we can address the travis issues afterward if they still exist ?
Compiling for Printrboard_RevF with graphics panel for example fails in AIDE on Windows, works in AIDE on Mac, but not on PIO for either due to failures in the LPC17xx HAL (which isn't actually needed for the target):
Compiling .pioenvs/printrboard_revf/src/frameworks/CMSIS/LPC1768/driver/lpc17xx_dac.o
*** [.pioenvs/printrboard_revf/src/frameworks/CMSIS/LPC1768/driver/lpc17xx_can.o] Error 1
Marlin/frameworks/CMSIS/LPC1768/driver/lpc17xx_clkpwr.c:39:28: fatal error: lpc17xx_clkpwr.h: No such file or directory
#include "lpc17xx_clkpwr.h"
^
compilation terminated.
*** [.pioenvs/printrboard_revf/src/frameworks/CMSIS/LPC1768/driver/lpc17xx_clkpwr.o] Error 1
Marlin/frameworks/CMSIS/LPC1768/driver/lpc17xx_dac.c:38:25: fatal error: lpc17xx_dac.h: No such file or directory
#include "lpc17xx_dac.h"
^
compilation terminated.
*** [.pioenvs/printrboard_revf/src/frameworks/CMSIS/LPC1768/driver/lpc17xx_dac.o] Error 1
*** [.pioenvs/printrboard_revf/src/Marlin.ino.o] Error 1
========================== [ERROR] Took 2.63 seconds ==========================
================================== [SUMMARY] ==================================
Environment megaatmega2560 [SKIP]
Environment megaatmega1280 [SKIP]
Environment printrboard [SKIP]
Environment printrboard_revf [ERROR]
Environment brainwavepro [SKIP]
Environment rambo [SKIP]
Environment anet10 [SKIP]
Environment sanguino_atmega644p [SKIP]
Environment DUE [SKIP]
Environment teensy35 [SKIP]
Environment Re-ARM [SKIP]
========================== [ERROR] Took 2.63 seconds ==========================
Reason I'm mentioning is that nobody can compile on Windows for targets with U8glib because of AIDE limitations (tries to pass gcc.exe a commandline that is >32k). There's a hacky work-around but an alternative is to just use Sublime Text 3/DevIoT/PIO (now that gepd has fixed it).
I was planning on a quick "Getting Started on Marlin 32-bit" video but since this Windows fiasco (due to increased path lengths with the new layout) it could easily be a "Compiling Marlin with PIO" video that covers everyone. Useful ?
is it possible to move forward with #7695 and we can address the travis issues afterward if they still exist ?
I'll try and get that fixed today
One thing that may keep at least 128k board users on Arduino IDE is that it is compiling smaller than PIO for both PROGMEM and SRAM, even for the simplest configurations o_O
AIDE 1.8.4:
Sketch uses 70594 bytes (27%) of program storage space. Maximum is 253952 bytes.
Global variables use 2914 bytes (35%) of dynamic memory, leaving 5278 bytes for local variables.
Maximum is 8192 bytes.
PIO core 3.4.1
Building .pioenvs/megaatmega2560/firmware.hex
AVR Memory Usage
----------------
Device: atmega2560
Program: 71382 bytes (27.2% Full)
(.text + .data + .bootloader)
Data: 2920 bytes (35.6% Full)
(.data + .bss + .noinit)
When my Printrbot_RevF was compiling on PIO, it was around 3.5k smaller on AIDE. No idea why that would be the case.
@Bergerac56 - those errors were fixed by the first set of files. Did you use them this time?
@Bob-the-Kuhn - Ooops right. Sorry. I restarted from a clean version of bugfix and forgot the first file. Done. It compiles fine.
EDIT: It works fine. The graphic LCD functions are OK (ON:OFF and Brightness). Nice job ;). Thanks
A workaround…?
- DECLARE_MENU_EDIT_TYPE(int16_t, int3);
+ DECLARE_MENU_EDIT_TYPE(int, int3);
I'll take a stab at fixing #7707 by combining more G-codes together into single units.
Sorry about the question. I'd like to contribute an EEPROM over FLASH emulation (NOT based on the ones already floating on the net, that use an Erase/Write cycle of a fixed FLASH page for each byte that is being stored.
Rather than that, my implementation groups and combines several writes into a page write, saving tons of writes to the FLASH (right now it uses just one FLASH E/W cycle for each M500 command, and distributes writes between a pool of FLASH pages,
This means that, for example, using 16 FLASH pages, we end with 16x10000 = 160000 configuration stores maximum, more than enough for any reasonable use of that command (is even more than most EEPROMS offer!). And using 16 pages only consumes 16*256 =4096 bytes in the FLASH,
My original implementation is written and tested on the SAM3 Due board, and it works. I have tested it very carefully.
But i think this kind of emulation could also benefit other targets - It could easily be generalized for any other targets that do not have EEPROM on board, but have FLASH memory that can be self programmed
That is why i ask... I could create a branch and do a pull request, but, instead, i'd like some opinions if the code could be generalized, if a better abstraction could lead to sharing between devices of the emulator...
By the way, i now realize the first block comment does not apply as an explanation on how this works (the comment was an explanation for a previous algorithm that i did that also worked, but not on Arduino Due due to some limitations the SAM3X flash controller has on Partial writes. (essentially, you can't reprogram a byte that has been programmed previously without doing a full page erase, otherwise, page bit corruptions happen)
The new algorithm is a way simpler. It just stores "blocks" of changes.
A block is composed by the starting address of the EEPROM (2 bytes, LSB first), the count of changed values (1 byte), and the new values for that defined address range,
Blocks are accumulated on a page sized RAM buffer, and blocks are created, merged and expanded as new bytes arrive (eeprom_write). As soon as the RAM buffer becomes full, the buffer is written to a new FLASH page on the current group of FLASH pages.
We have at least 2 groups of FLASH pages (all groups are the same size). Once a group of pages becomes full, then the emulator "compresses" all the Blocks of that group into a new block that is stored on the pages of the next group. Then the actual group is erased and the next group becomes the actual group
So, essentially, is more or less an storage of differences to the previous values. And we assume the emulated EEPROM default byte value is 0xFF
@thinkyhead - I'd hold off on any permanent work to resolve #7707 because it appears they have POC of a workaround in arduino-builder already that I tested and works:
https://github.com/arduino/Arduino/issues/6751#issuecomment-331879017
@ejtagle
I have no familiarity with the SAM3 controllers so I can't comment on your implementation, but I have been rolling the idea of EEPROM emulation in FLASH for LPC17XX/Re-ARM for a while in my head.
The minimum erase size for LPC17XX is one sector (4kB/32kB) and the minimum write size is 256 bytes.
My idea for emulating EEPROM was simpler:
I have never actually tested or implemented this but in theory it should allow for using only one E/W cycle for each M500 command.
@Bob-the-Kuhn - can you tell me if I'm right that this redefine is correct ?:
In file included from Marlin/src/inc/MarlinConfig.h:37:0,
from /Users/speedster/dev/github/Marlin/Marlin/Marlin.ino:31:
Marlin/src/inc/Conditionals_post.h:1009:0: warning: "NOT_A_PIN" redefined
#define NOT_A_PIN 0 // For PINS_DEBUGGING
^
In file included from /Users/speedster/.platformio/packages/framework-arduinoteensy/cores/teensy/Arduino.h:2:0,
from /Users/speedster/.platformio/packages/framework-arduinoteensy/cores/teensy/elapsedMillis.h:29,
from /Users/speedster/.platformio/packages/framework-arduinoteensy/cores/teensy/WProgram.h:26,
from /Users/speedster/.platformio/packages/framework-arduinoteensy/cores/teensy/Arduino.h:1,
from /var/folders/q1/g9l9mqmn319fh27k43yqpy440000gr/T/tmpuqVitL:1:
/Users/speedster/.platformio/packages/framework-arduinoteensy/cores/teensy/pins_arduino.h:43:0: note: this is the location of the previous definition
#define NOT_A_PIN 127
From comments in teensy/pins_arduino.h it's clear that even Teensyduino guys didn't think that having NOT_A_PIN
as 127 was normal:
// This allows CapSense to work. Do any libraries
// depend on these to be zero?
#define NOT_A_PORT 127
#define NOT_A_PIN 127
#define NOT_AN_INTERRUPT -1
It sounds to me that 0 is correct and I should tweak Conditionals_post.h to #undef NOT_A_PIN immediately before setting defining as 0 to prevent the string of compile warnings ?
#ifdef TEENSYDUINO
#undef max
#define max(a,b) ((a)>(b)?(a):(b))
#undef min
#define min(a,b) ((a)<(b)?(a):(b))
#define NOT_A_PIN 0 // For PINS_DEBUGGING
#endif
I want to fix it only because it takes 10x longer for _Sublime Text 3_ to cat these errors to the screen than it does to actually do the compile 😝 )
I do everything on Windoze... But even I know you just do a >>/dev/null to get rid of error messages.
If you use Arduino they automatically define NOT_A_PIN as zero.
It looks like NOT_A_PIN is only used to check if a port index is valid.
As best I can tell all the Arduino port tables always have NOT_A_PIN as the first entry followed by portA, then portb, ... That way porta is index 1, portb is index 2, ... and index zero is always illegitimate.
If that's the case then any number over 13 would also do for NOT_A_PIN.
I can see why redefining it to 127 would cause a lot of error messages. It's used in pinMode, digitalWrite and digitalRead.
From the comment it appears that the CapSense library needed the 127 value at one time. I downloaded the current CapSense libraries pointed at by Arduino and they do not use NOT_A_PIN. I couldn't find the older ones.
My opinion is we only need to worry about an old CapSense library being used in a printer controller. That's pretty far fetched. I think changing back to zero is OK.
@thinkyhead - I would like to come back on the problem when printing a cylinder with a good resolution. As explained, it makes the motors shaking. (problem with very short moves on RE-ARM). It still happens with the last release. Do you have an idea how to solve it? Thanks
@Bergerac56 Can you supply the problematic gcode to test with, I have a high resolution circle in my motion test gcode and have never had an issue before.
@Bergerac56 are you printing from an SD Card or from a host. If from a host, which one?
@tcm0116 and @p3p
here it is (I tried from the SD card and from a host-repetier)
EDIT: I tried on 2 different hardware setup: one full functionnal printer and one test setup (only the re-arm board was the same)
JOP_TEST.zip.
Configuration.zip
Most helpful comment
Yes. Agreed.
Yes. Agreed.
Probably the best thing to do is let things stabilize for a while. Once development activity gets back to normal let's just contain the refinement to one well defined area at a time. Otherwise everything grinds to a halt. This re-organization was needed but it was very disruptive. Things need to stabilize.
My guess is if we replace the current bugfix-2.0.x branch with the re-organized one and start pointing people to it... It will take a few weeks to get all the hiccups out of it and back under control. That is OK! And then, further refinement of various sub-systems can continue.