Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(1.1.x) serious bug G33 #8137

Merged
merged 1 commit into from Oct 29, 2017

Conversation

LVD-AC
Copy link
Contributor

@LVD-AC LVD-AC commented Oct 28, 2017

No description provided.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Oct 28, 2017

Fix for issue #8124

@@ -5433,7 +5433,7 @@ void home_all_axes() { gcode_G28(true); }
dy = (Y_PROBE_OFFSET_FROM_EXTRUDER);
#endif

for (uint8_t i = 0; i < COUNT(z_at_pt); i++) z_at_pt[i] = 0.0;
for (uint8_t i = 0; i <= 12; i++) z_at_pt[i] = 0.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Magic numbers" are frowned upon, and if the length of the array changes this has to be changed in multiple hard-to-locate places. Please add a #define to replace the literal number 13 so that it can be changed in only one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thinkyhead You have 1-point, 4-point and 7-point calibration; I've added the average of 2 x 7-point with common centre point -> 13-point for more precision. So 1, 4, 7, 13 are the magic numbers for calibration and should not be changed.

So I'm not in favor just to add a #define NUMBER_OF_CALIBRATION_POINTS 13 unless a comment is added : "DO NOT CHANGE"; it would give the false impression that changing the number of calibration points is as easy as changing the #define statement. It requires also changes to the calibration- and autotune-matrices, the steps in the for loops, etc...

However maybe we can consider to make a LOOP_CAL_PT(axis, start, step) similar to LOOP_XYZ(axis) to replace the for loops with the magic number 12, and a CalEnum for the specific magic numbers used all over the code.

See #8171 and #8173 for full implementation; I also changed the probe grids a bit to make them more universal. No change to the documentation is needed.
All magic numbers (array size, indices matrices, steps,...) are set by _7P_STEP. I tested it with 7p- (current set up - _7P_STEP = 1) and 13p-arrays (old set up _7P_STEP = 2). Of course when you extend the array one should do something (average or extend the calibration matrices used) to include the new probe points in the calculations (I left the extra code to average the 13p-array to a 7p-array in comments).

@thinkyhead thinkyhead merged commit c0a8275 into MarlinFirmware:bugfix-1.1.x Oct 29, 2017
@LVD-AC LVD-AC deleted the (1.1.x)bug_G33 branch October 30, 2017 18:50
This was referenced Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants