Skip to content

[2.0.x] Split first planner move for better chaining#8611

Merged
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
thinkyhead:bf2_planner_split_first
Dec 3, 2017
Merged

[2.0.x] Split first planner move for better chaining#8611
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
thinkyhead:bf2_planner_split_first

Conversation

@thinkyhead
Copy link
Copy Markdown
Member

@thinkyhead thinkyhead commented Nov 30, 2017

Based on #8608


Addressing #8595.

Always split up the first move to an empty planner so that subsequent moves will be able to chain more easily.

@thinkyhead thinkyhead added T: Design Concept Technical ideas about ways and methods. PR: Improvement PR: Bug Fix labels Nov 30, 2017
@thinkyhead thinkyhead force-pushed the bf2_planner_split_first branch 2 times, most recently from 26d1511 to 8cb0898 Compare December 1, 2017 00:02
@thinkyhead thinkyhead force-pushed the bf2_planner_split_first branch 23 times, most recently from 4458d92 to d343134 Compare December 3, 2017 07:25
@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 3, 2017

thanks for answering and reverting the commit.
I had to fix some other issues with my atom/platformio, but I hope I'm back now...

I'll try your suggestions soon...

thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Dec 3, 2017
…anner_split_first"

This reverts commit 824980e, reversing
changes made to aa7efb9.
@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 3, 2017

didn't know if you want me to test these together or separate so I tried all combinations.
None of them worked.

I enabled the debug outputs (had to change all to SERIAL_PAIR).

One reproducible behavior is this:
I moved Z axis 7 x 1mm down and each move was bigger than before.

  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:260 (260 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:520 (520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:460 (460 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:920 (920 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:660 (660 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1320 (1320 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:860 (860 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1720 (1720 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1060 (1060 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:2120 (2120 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1260 (1260 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:2520 (2520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1460 (1460 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:2920 (2920 steps) E:0 (0 steps)

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

ah, when I reversed the direction (up) from there, the axis still moved down, but each move was shorter then the previous, until it stops at zero speed.

  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1260 (1260 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:2520 (2520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1060 (1060 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:2120 (2120 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:860 (860 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1720 (1720 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:660 (660 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:1320 (1320 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:460 (460 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:920 (920 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:260 (260 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:520 (520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:60 (60 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:120 (120 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (0 steps) C:0 (0 steps) E:0 (0 steps)
...

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

If I home Y and move 100mm and 100mm back, I get this:

  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-8560 (4000 steps) C:2000 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-4560 (8000 steps) C:2000 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:2000 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:2000 (0 steps) E:0 (0 steps)

I have 80 usteps/mm for X/Y and 400 for Z.
As you see, I get two moves 4000 steps and 8000 steps instead of one.
And the position is negative?

thinkyhead added a commit that referenced this pull request Dec 4, 2017
[2.0.x] Revert PR #8611 — split first planner move
@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

I also enabled another debug print section I found.

So, now I homed and then moved Z down a bit to come out of the endstops.

Now I moved 1mm down each and after the !<<<, I moved up again each 1mm.
Z behaved similar, but this time Y also moved each step (a short length each) and continued to move after Z stopped.

I also noticed, that you write (xxx steps) in _buffer_line, but use the target array. Isn't it the target position?

  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:1.600000 (640 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:320 (320 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:640 (640 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:2.600000 (1040 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:520 (520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1040 (1040 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:3.600000 (1440 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:720 (720 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1440 (1440 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:4.600000 (1840 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:920 (920 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1840 (1840 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:5.600000 (2240 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:1120 (1120 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:2240 (2240 steps) E:0 (0 steps)
>>> !
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:4.600000 (1840 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:920 (920 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1840 (1840 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:3.600000 (1440 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:720 (720 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1440 (1440 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:2.600000 (1040 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:520 (520 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:1040 (1040 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:1.600000 (640 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:320 (320 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:640 (640 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:0.600000 (240 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:120 (120 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:240 (240 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:0.000000 (0 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:0.000000 (0 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-150.000000 (-12000 steps) Y:-150.000000 (-12000 steps) Z:0.000000 (0 steps) E:0.000000 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12280 (280 steps) C:0 (0 steps) E:0 (0 steps)
  _buffer_steps FR:200.000000 A:-12000 (0 steps) B:-12000 (560 steps) C:0 (0 steps) E:0 (0 steps)

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

narrowing the issue...

I changed the debug output to see the position->target pairs in _buffer_line.
It's obvious that the position is not updated as it should.
The position never changes in this one, that explains why an axis can move if it is not targeted.

  _buffer_line FR:200.000000 X:-149.599976 (-11968 -> -11968) Y:-149.799988 (-12560 -> -11984) Z:8.000000 (0 -> 3200) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:-11968 (0 steps) B:-11984 (576 steps) C:3200 (3200 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-149.599976 (-11968 -> -11968) Y:-149.699982 (-12560 -> -11976) Z:8.000000 (0 -> 3200) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:-11968 (0 steps) B:-11976 (584 steps) C:3200 (3200 steps) E:0 (0 steps)

BUT...

as you can see here, it is different with X...
X is updated, but at the same time Y and Z are not.
So either the COPY macro at the end of _buffer_steps doesn't work or X is updated at another place.

  _buffer_line FR:200.000000 X:-149.899994 (-12000 -> -11992) Y:-150.000000 (-12560 -> -12000) Z:8.000000 (0 -> 3200) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:-11992 (8 steps) B:-12000 (560 steps) C:3200 (3200 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-149.799988 (-11992 -> -11984) Y:-150.000000 (-12560 -> -12000) Z:8.000000 (0 -> 3200) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:-11984 (8 steps) B:-12000 (560 steps) C:3200 (3200 steps) E:0 (0 steps)
  _buffer_line FR:200.000000 X:-149.699982 (-11984 -> -11976) Y:-150.000000 (-12560 -> -12000) Z:8.000000 (0 -> 3200) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:-11976 (8 steps) B:-12000 (560 steps) C:3200 (3200 steps) E:0 (0 steps)

X always moved correctly, only the first move was far to long. I guess it had a wrong start position.

I wonder if it only effects corexy? or any kinematic printer?

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

seems like COPY doesn't work for anyone but X:

  _buffer_line FR:200.000000 X:3.000000 (160 -> 240) Y:0.000000 (0 -> 0) Z:0.000000 (0 -> 0) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:200 (40 steps) B:0 (0 steps) C:0 (0 steps) E:0 (0 steps)
COPY(160, 0, 0 <- 200, 0, 0)
now  200, 0, 0
  _buffer_steps FR:200.000000 A:240 (40 steps) B:0 (0 steps) C:0 (0 steps) E:0 (0 steps)
COPY(200, 0, 0 <- 240, 0, 0)
now  240, 0, 0
  _buffer_line FR:200.000000 X:4.000000 (240 -> 320) Y:0.000000 (0 -> 0) Z:0.000000 (0 -> 0) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:280 (40 steps) B:0 (0 steps) C:0 (0 steps) E:0 (0 steps)
COPY(240, 0, 0 <- 280, 0, 0)
now  280, 0, 0
  _buffer_steps FR:200.000000 A:320 (40 steps) B:0 (0 steps) C:0 (0 steps) E:0 (0 steps)
COPY(280, 0, 0 <- 320, 0, 0)
now  320, 0, 0
  _buffer_line FR:200.000000 X:4.000000 (320 -> 320) Y:1.000000 (0 -> 80) Z:0.000000 (0 -> 0) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:320 (0 steps) B:40 (40 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 40, 0)
now  320, 0, 0
  _buffer_steps FR:200.000000 A:320 (0 steps) B:80 (80 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 80, 0)
now  320, 0, 0
  _buffer_line FR:200.000000 X:4.000000 (320 -> 320) Y:2.000000 (0 -> 160) Z:0.000000 (0 -> 0) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:320 (0 steps) B:80 (80 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 80, 0)
now  320, 0, 0
  _buffer_steps FR:200.000000 A:320 (0 steps) B:160 (160 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 160, 0)
now  320, 0, 0
  _buffer_line FR:200.000000 X:4.000000 (320 -> 320) Y:3.000000 (0 -> 240) Z:0.000000 (0 -> 0) E:0.000000 (0 -> 0)
  _buffer_steps FR:200.000000 A:320 (0 steps) B:120 (120 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 120, 0)
now  320, 0, 0
  _buffer_steps FR:200.000000 A:320 (0 steps) B:240 (240 steps) C:0 (0 steps) E:0 (0 steps)
COPY(320, 0, 0 <- 320, 240, 0)
now  320, 0, 0

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

that's the reason:

Marlin/src/module/../inc/../core/macros.h:141:52: warning: 'sizeof' on array function parameter 'target' will return size of 'const int32_t* {aka const long int*}' [-Wsizeof-array-argument]
#define COPY(a,b) memcpy(a,b,min(sizeof(a),sizeof(b)))

@AnHardt
Copy link
Copy Markdown
Contributor

AnHardt commented Dec 4, 2017

Great finding!
And indeed i tested on a unhomed Prusa i3 like machine with moves in x only. :-(

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

this is one of the C relicts.

You can declare the array parameter like this
void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const uint8_t extruder)

you can protect it by

  static_assert(COUNT(target) > 1, "array as function parameter should be declared as reference and with count");
  COPY(position, target);

But I wonder if it should be solved generally...
Also, things like using memcpy for COPY will probably lead to other problems in the future.
Is there a defined way how the community decides such things?

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

that's how it is done in ubl_motion, which cries out for a more general decision:

  #if UBL_DELTA

    // macro to inline copy exactly 4 floats, don't rely on sizeof operator
    #define COPY_XYZE( target, source ) { \
                target[X_AXIS] = source[X_AXIS]; \
                target[Y_AXIS] = source[Y_AXIS]; \
                target[Z_AXIS] = source[Z_AXIS]; \
                target[E_AXIS] = source[E_AXIS]; \
            }

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

And indeed i tested on a unhomed Prusa i3 like machine with moves in x only. :-(

so an i1 :-) called Murphy

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 4, 2017

@thinkyhead (or who may change the pull request)

here is a patch for planner.cpp and planner.h

fix-COPY.patch.zip

It uses the array parameter as reference.
Because this doesn't have compatibility problems with C (C doesn't have references) it works like it should.

I am not sure, which compilers and versions are supported by Marlin, but this should work for most C++ compilers. It's quite standard for a long time.

@thinkyhead
Copy link
Copy Markdown
Member Author

D'oh!

@thinkyhead
Copy link
Copy Markdown
Member Author

thinkyhead commented Dec 5, 2017

things like using memcpy for COPY will probably lead to other problems in the future. Is there a defined way how the community decides such things?

A general principle is to define a macro for things we do often, to save on typing if nothing else. I believe that static_assert may be embeddable inside macros, so such a test could be part of the COPY macro. I'll double-check that. I think we should use static_assert liberally to check on things inside function scope that can't be asserted elsewhere. It costs nothing and each use adds more security.

The subject of unit tests has come up a lot, including another new issue recently. If someone wants to "get on that" it seems like it would be a good thing! I'm not sure what a total application of unit tests would fully entail, but I think it's a good idea. To be completely thorough we'd want to insist that every function include some static_asserts in the head to test arguments and other compile-time properties.

I've proposed a "simulation build" before, but better would be a "unit test build" that when booted doesn't run the main loop, but just runs internal tests on the firmware to test expected function behavior, where every function has —essentially— extra debugging added into it to (at minimum) check the inputs, and where the test code checks for expected output and/or other effects. That might be easier said than done, depending on what kinds of tests it runs. It could get fancy and ask for user input to test LCD functions, and so on. I guess then we're talking about a whole separate .ino file… and yet more conditional blocks…

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 5, 2017

the whole unit test thing would be much easier if everything would be class based.
A modular application where each module (not in the sense of a C module) has it's clearly defined functionality is much easier to test.
Such "modules" can be tested separately and this test functions as a contract and also as an example how to use the module.
Usually, in C++ such a module is a class.

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 5, 2017

should we start an issue for discussion?

@thinkyhead
Copy link
Copy Markdown
Member Author

thinkyhead commented Dec 6, 2017

the whole unit test thing would be much easier if everything would be class based.

Contributors should feel free to consolidate code into singleton classes, as I've done with Temperature, Endstops, Stepper, Planner, LEDLights, and others. Classes should be stringently static to preserve the leaner static linkage and calling.

Marlin's first aim as an AVR-based firmware is to find ways to improve performance, keep the code small, and keep SRAM usage low. Classes in Marlin are used more as a convenience, to group related code, than for the benefits that classes are generally touted for — inheritance, easy instantiation, and so on.

C++ code can be written in such a way that it doesn't sacrifice performance, but some principles have to be kept in mind.

should we start an issue for discussion?

I've got a lot of things on my plate right now trying to juggle the next 1.1 release with fielding issues, following up on bug reports, and keeping the 1.1 / 2.0 branches in sync. If we're going to start applying unit testing and all the rest, I suggest that this effort be delayed until the 2.0 release, whenever that is. There's no way I'm going to be able to take on the load of yet another overhaul to two separate branches.

@hg42
Copy link
Copy Markdown
Contributor

hg42 commented Dec 6, 2017

totally clear :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Bug Fix PR: Improvement T: Design Concept Technical ideas about ways and methods.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants