MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.14k stars 19.2k forks source link

DELTA: Are workspace X/Y shift ever used (RAW_X != LOGICAL_X)? #6927

Closed oldmcg closed 6 years ago

oldmcg commented 7 years ago

Looking for ways to avoid unnecessary delta kinematic calculations.

I know that Z_AXIS raw vs logical is used for home_offset, but are the X and Y workspace shifts EVER used for delta? If not I can rearrange the RAW/LOGICAL macros for delta to skip any calculations for X/Y shifting and just leave it for Z.

Thoughts?

Roxy-3D commented 7 years ago

I'm sure some people will disagree with this opinion. (So... I'm interested in hearing the thinking...) But for Delta's, does it even make sense to shift the work space? The center of the bed is typically at (X,Y) = (0,0) Why would a Delta user want to shift the coordinates?

Any way... The reason I'm asking the question is for Delta's I'm all in favor of reducing any calculations that really don't offer a benefit!!!!!

oldmcg commented 7 years ago

And a related question without opening new topic...

SOFTWARE_ENDSTOPS implementation makes no sense for deltas for X/Y axes -- it just constrains X and Y to MIN/MAX without regard for circular bed. Yet this is enabled by default in delta sample configs. I already added is_reachable checks to blocking_move_to and ubl delta line functions to reject out-of-printable-radius moves, regardless of SOFTWARE_ENDSTOPS.

Somebody might want a Z-axis endstop at zero, maybe, but hard to justify math in 6 directions for 1 possible clamping. Even Z axis endstop doesn't fix runaway probes that crash into bed because probe disables software endstops.

I propose to turn off SOFTWARE_ENDSTOPS for delta entirely, or for delta make it only test Z axis.

oldmcg commented 7 years ago

@Roxy-3D, I'll just #ifdef the RAW/LOGICAL X/Y stuff so it can be enabled again if needed. It won't help much versus those 3 kinematic sqrts for each segment, but it does trickle down into the mesh cell computations too.

Roxy-3D commented 7 years ago

OK! We can leave it like that for a while... And wait a little bit to see if there is a reason to keep it.

On the Software Endstops for Delta's. The big problem was all the sqrt() calculations to do this 'correctly'. And that is why it is in a 'not fully rational' state. One idea that was discussed is we don't actually need to do the sqrt() because we can just compare the the X^2+Y^2 against a number. And some people were considering doing the X^2+Y^2 as integer math.

JustAnother1 commented 7 years ago

Why would a Delta user want to shift the coordinates?

I would think for the same reason that a Cartesian user would want to do that, right?

Back in the days of Skeingforge slicing took some time, so shifting the coordinates allowed to move a sliced model on the build plate. You either needed to avoid a very stressed and therefore bad segment of the bed, or for "mass producing".

But all that should be done in the G-Code parser. The low layer of Cartesian/delta/.. should not be involved with things like this, right?

But I do now only very little about the Marlin code base, so I might be completely wrong here,...

Roxy-3D commented 7 years ago

But all that should be done in the G-Code parser. The low layer of Cartesian/delta/.. should not be involved with things like this, right?

I think I agree. If we did it up in the G-Code parser... everybody benefits and it probably simplifies all of the lower level code.

oldmcg commented 7 years ago

On the Software Endstops for Delta's. The big problem was all the sqrt() calculations to do this 'correctly'. And that is why it is in a 'not fully rational' state. One idea that was discussed is we don't actually need to do the sqrt() because we can just compare the the X^2+Y^2 against a number. And some people were considering doing the X^2+Y^2 as integer math.

That is what is_position_reachable does -- compares the X^2+Y^2 to precomputed R^2 so just two muls, add, compare, but still floating point. Trivial to add that to software endstops for delta, but probably redundant since I already added is_reachable check to blocking_move_to, kinematic_line_to, and ubl_delta_line_to. The G2/G3 arc code might not have that enforcement for delta yet.

oldmcg commented 7 years ago

But all that should be done in the G-Code parser. The low layer of Cartesian/delta/.. should not be involved with things like this, right?

I think I agree. If we did it up in the G-Code parser... everybody benefits and it probably simplifies all of the lower level code.

I think that is essentially what raw vs logical does now. The parser/presentation layer deals in logical, which is translated to raw then back to logical for display, etc. Things like fade height currently work off logical. Leveling and kinematics work off of raw. I have a new M114 D option in next PR to show all these coordinate layers and when they disagree.

If X/Y shifting needs to be done in the firmware instead of in the slicer then I'll leave support for delta shifting enabled by default but add an option to disable it for a tiny bit of speed.

The limiting speed factor for delta is not really the 3 sqrts for every segment -- it is all the stuff under planner._buffer_line called for every 0.1mm of "linear" motion that deals with acceleration. Since a many-segmented line is moving all in one direction (although a delta stepper may slowly reverse direction during course of a line), I think some of these acceleration and jerk checks could be relaxed during a segmented linear move. Project for another day -- I have to start with the low hanging fruit.

Roxy-3D commented 7 years ago

It might be a good line item to add to the v1.1.2 time line?

oldmcg commented 7 years ago

I don't have a cartesian machine to make sure I don't break something else so I'm hesitant to change anything that is not #if DELTA. I cannot figure out from looking through the code where RAW (workspace shift) is ever applied to cartesian X/Y. It seems like logical (gcode-specified) coordinates all the way from G1 down to _buffer_line, meaning stepper counts are in logical not raw. Maybe I'm missing something. For kinematics logical gets converted to raw during kinematics translation, which would account for workspace shift, but for cartesian I just can't find the place in the code where it shifts the coordinates before calling _buffer_line. To further confuse things, soft_endstop_min/max are initialized with X_MIN_POS, etc, which are usually treated as RAW coordinates in the leveling systems, but clamp_to_software_endstops treats them as logical because it just compares user-specified coordinates to them without workspace shift.

I changed the ubl delta segmentation to convert to raw at the top and then deal only in raw for whole line, so it doesn't have to do logical-to-raw during kinematics for every segment. So I guess raw x/y shift won't really matter performance wise if compiled with UBL_DELTA. The odd side of this change is that for cartesian segmentation I have to convert back from raw to logical before calling _buffer_line. No big deal, just seems that raw vs logical is not consistently handled between cartesian and kinematic.

Roxy-3D commented 7 years ago

Let's make sure @thinkyhead sees this! https://github.com/MarlinFirmware/Marlin/issues/6927#issuecomment-305826401

This is sounding more and more like a good thing to get cleaned up for the next point release.

JustAnother1 commented 7 years ago

From what I understand I would say the "shift coordinates" should happen with the logical coordinates. Everything outside of the delta/Cartesian/ specific stuff should be done on logical coordinates. Only in that last layer a conversion (if necessary) should be done.

PLease also don't forget about 32bit in this context,...

Roxy-3D commented 7 years ago

PLease also don't forget about 32bit in this context,...

Please explain. What ever is done on this should be the same for 8-bit AVR's and 32-bit ARM's, right? Why does something different need to be done for 32-bit processors?

JustAnother1 commented 7 years ago

Why does something different need to be done for 32-bit processors?

There is no "special requirement" for 32bit here.

The difference between 8 and 32bit is performance. And performance is part of this discussion. Removing a feature to improve the performance might make sense for 8bit. On 32bit we have more performance and would want to have the feature. So if we want to make this feature optional then it should be done in a way that 32bit could still make it default.

I also think that on 32bit we should implement a step by step correct movement (so no small segment stuff). Making these changes possible should also be taken into consideration.

Roxy-3D commented 7 years ago

Thank you... That makes sense.

oldmcg commented 7 years ago

@JustAnother1, UBL_DELTA already does "step by step" configurable down to as small as you would like with default being 0.1mm for delta (subject to feedrate and delta_segments_per_second) and 1.0mm for cartesian when UBL_GRANULAR_SEGMENTATION_FOR_CARTESIAN enabled. For faster CPU you could just make these smaller down to the stepper resolution.

I agree that 32-bit must be the real long term goal, and design choices should be made with that in mind, but some 32-bit centric coding choices would lead to poor performance on 8-bit. For example, passing 4 floats to a function on 8-bit is far cheaper to pass as an array so single pointer pushed on the stack, but for 32-bit it is better to pass 4 distinct float parameters that could be en-registered by compiler.

Trying to maintain single codebase that can be built for both 8-bit and 32-bit will always be a challenge with design trade-offs in both directions. Once you start making 32-bit centric design choices it becomes hard to make same code perform well on 8-bit platforms.

Personally I wish all 8-bit boards would be declared illegal and 32-bit should be the lowest common denominator. :)

JustAnother1 commented 7 years ago

Trying to maintain single codebase that can be built for both 8-bit and 32-bit will always be a challenge with design trade-offs in both directions. Once you start making 32-bit centric design choices it becomes hard to make same code perform well on 8-bit platforms.

I agree with you. But it possible to do it. I did it. And the current road map as outlined by @Roxy-3D and others plans for a common code base as Marlins way into 32bit world.

But we are also (again)behind schedule on that. There have been some forks for different 32bit cpus, then there was a sort of "official" 32bit branch. Then just a few days ago the decision was made to let @thinkyhead refactor the G-Code decoder before starting to merge the first 32bit stuff into the main development branch. I still hope that this first merge happens this time,...

Roxy-3D commented 7 years ago

Then just a few days ago the decision was made to let @thinkyhead refactor the G-Code decoder before starting to merge the first 32bit stuff into the main development branch. I still hope that this first merge happens this time,...

I believe we have consensus that when the new file organization is done, 32-bit will have a place in it. Even if the 32-bit developer types don't entirely like how the files and folders are organized, they will be free to make changes in that layout that they want to make.

Thinky was thinking he could do the new file and folder layout over a 'long weekend'. When he gets the energy and his schedule lines up right, I'm confident it will happen.

I seriously believe that is going to be a big help to the 32-bit work because that effort won't be a "one off" exercise. It won't have to chase all the other changes in the code base because it will be part of the main code base. In other words, I think doing that is the single best thing we can do to help the 32-bit effort get across the 'finish line'.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.