-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
General cleanup of G33 #6533
General cleanup of G33 #6533
Conversation
Marlin/Marlin_main.cpp
Outdated
*/ | ||
inline void gcode_G33() { | ||
|
||
stepper.synchronize(); | ||
if (axis_unhomed_error(true, true, true)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The procedure starts with homing the printer, so this test is not quit required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling gcode_G28
directly is contraindicated, so we simply require homing ahead of any commands that require it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to call the homing procedure. The calibration procedure needs to home the printer at each iteration otherwise there is no way the printer knows at what position it is after tweaking the end-stops and delta height. It just does not work without homing.
Marlin/Marlin_main.cpp
Outdated
set_bed_leveling_enabled(false); | ||
#endif | ||
const int8_t p_value = code_seen('P') ? code_value_int() : DELTA_CALIBRATION_DEFAULT_POINTS; | ||
if (!WITHIN(p_value, -7, 7)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
within (p_value, 1 , 7)
I changed that at your request that no negative values may be used; the further testing relies on that fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
** I'm trying to follow here, so what happens if someone enters P-5 -> p_value = -5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. I missed that the negative-ness of the initial parameter had been changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this:
if (!WITHIN(probe_points, 1, 7)) {
Marlin/Marlin_main.cpp
Outdated
probe_mode = (code_seen('T') && probe_mode > 2 ? -probe_mode : probe_mode); | ||
|
||
int8_t verbose_level = (code_seen('V') ? code_value_byte() : 1); | ||
int8_t probe_mode = WITHIN(p_value, 1, 7) ? p_value : DELTA_CALIBRATION_DEFAULT_POINTS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
** P-5 -> probe_mode is set to DELTA_CALIBRATION_DEFAULT_POINTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No further testing on probe_points is necessary:
int8_t probe_mode = probe_points;
Marlin/Marlin_main.cpp
Outdated
|
||
gcode_G28(); | ||
const uint8_t probe_points = abs(probe_mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P-5 -> probe_points is set to abs(DELTA_CALIBRATION_DEFAULT_POINTS) and further testing is done with probe_points.
IMO this unnecessary complicating things and confusing. I think it is better only to allow positive values to the P parameter and assign it directly to probe_points. Then this line can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already there. I just moved it up a few lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and delete this indeed
Marlin/Marlin_main.cpp
Outdated
#if PLANNER_LEVELING | ||
set_bed_leveling_enabled(false); | ||
#endif | ||
const int8_t p_value = code_seen('P') ? code_value_int() : DELTA_CALIBRATION_DEFAULT_POINTS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make that :
const int8_t probe_points = code_seen('P') ? code_value_int() : DELTA_CALIBRATION_DEFAULT_POINTS;
0001ce6
to
d86660f
Compare
Some developers have been ignoring the embargo on directly calling GCode handler functions, so I've just added |
d86660f
to
af07ee5
Compare
Marlin/Marlin_main.cpp
Outdated
const bool pp_height_only = probe_points == 1, | ||
pp_center_and_towers = probe_points == 2, | ||
pp_all_positions = probe_points == 3, | ||
pp_5_points = probe_points == 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion if you prefer descriptive names:
==1 pp_center_only
==2 pp_towers_or_opposites
==3 pp_towers_and_opposites
(==4 pp_all_single_circle)
==5 pp_all_double_circle
==6 pp_all_triple_circle
==7 pp_all_quadruple_circle
'> 2 pp_with_tower_angles
'> 3 pp_with_intermediates
'> 4 pp_with_multi_circles
Marlin/Marlin_main.cpp
Outdated
* V Verbose level: | ||
* V0 Dry-run mode. Report settings and probe results. No calibration. | ||
* V1 Report settings | ||
* V2 Report settings and probe results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
af07ee5
to
e2ee5fa
Compare
stepper.synchronize(); | ||
const int8_t probe_points = code_seen('P') ? code_value_int() : DELTA_CALIBRATION_DEFAULT_POINTS; | ||
if (!WITHIN(probe_points, 1, 7)) { | ||
SERIAL_PROTOCOLLNPGM("?(P)oints is implausible (1 to 7)."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Marlin/Marlin_main.cpp
Outdated
|
||
int8_t verbose_level = (code_seen('V') ? code_value_byte() : 1); | ||
const char negating_parameter = pp_height_only ? 'A' : pp_center_and_towers ? 'O' : 'T'; | ||
int8_t probe_mode = code_seen(negating_parameter) && code_value_bool() ? -probe_points : probe_points; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
e2ee5fa
to
4122b0e
Compare
Ok, I think I'm done massaging this now… I'll let it cool off for an hour or two, merge it later… |
|
||
gcode_G28(); | ||
home_all_axes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Marlin/Marlin_main.cpp
Outdated
if (pp_greather_3 && !pp_equals_5) // average intermediates to tower and opposites | ||
for (uint8_t axis = 1; axis < 13; axis += 2) | ||
if (points_over_3) // average intermediates to tower and opposites | ||
for (uint8_t axis = 1; axis <= 11; axis += 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but for future maintenance I would prefer you left '<13' or change all to '<=12' that magic number actually means do a full circle no matter what the step is by which axis is increased.
Marlin/Marlin_main.cpp
Outdated
int8_t pp = (code_seen('P') ? code_value_int() : DELTA_CALIBRATION_DEFAULT_POINTS), | ||
probe_mode = (WITHIN(pp, 1, 7) ? pp : DELTA_CALIBRATION_DEFAULT_POINTS); | ||
const char negating_parameter = pp_height_only ? 'A' : pp_center_and_towers ? 'O' : 'T'; | ||
int8_t probe_mode = code_seen(negating_parameter) && code_value_bool() ? -probe_points : probe_points; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Marlin/Marlin_main.cpp
Outdated
pp_7_points = probe_points == 7, | ||
probe_extra_center = probe_points > 2, | ||
point_averaging = probe_points > 3, | ||
points_over_4 = probe_points > 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry this is meaningless! It is not the center points that make the fundamental difference in the different probe grids, it is the radius points...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should it be changed to?
Marlin/Marlin_main.cpp
Outdated
const bool pp_height_only = probe_points == 1, | ||
pp_center_and_towers = probe_points == 2, | ||
pp_all_positions = probe_points == 3, | ||
pp_5_points = probe_points == 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it another try to make some sense out of it:
==5 pp_double_circle
==6 pp_triple_circle (in fact it probes 6 centre points so the name "all_except_center" is misleading)
==7 pp_quadruple_circle
'>2 pp_seven_point_calibration
'>3 pp_intermedate_averaging
'>4 pp_circle_averaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to explain: I do averaging in 2 directions (like X and Y on Cartesians) but for Delta's that would be X=more points than just towers and opposite to the towers on the same circle (starts at n>3) and Y=varying the radius of the probe circle (starts at n>4). So your pick, you could replace "double_circle" in the above names to "double_radius" etc and "radial_averaging" or even "radii_averaging" (love the double Latin i, sounds more posh) and replace "intermediate_averaging" with "rotational_averaging" or "axial_averaging" or something of the sort, I do not mind as long as it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: may I suggest that in the >n names only you leave out the leading pp_. That will help me (and others) to distinguish between the names that have only one value (==n) with leading pp_ or a set of values (>n) without leading pp_.
==5 pp_double_radius
==6 pp_tripple_radius
==7 pp_quadruple_radius
'>2 seven_point_calibration
'>3 axial_averaging
'>4 radial_averaging
5ffe867
to
718f7cb
Compare
Marlin/Marlin_main.cpp
Outdated
do_circle_x4 = probe_points == 7, | ||
probe_extra_center = probe_points >= 3, | ||
point_averaging = probe_points >= 4, | ||
points_over_4 = probe_points >= 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you insist on focusing on the centre points have it your way and make it:
'>=3 : probe_3_extra_center
'>=5 : probe_6_extra_centre
I can live with that, I'm Belgian where surrealisme is not an art movement but a way of life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about probe_center_plus_3 and probe_center_plus_6? Does that accurately convey the meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surrealisme is not an art movement but a way of life
Ah, so that explains the levitating apple in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steve Jobs must have had some Belgian roots...
6c0706a
to
320eda5
Compare
- update comment section - redefined P5 grid - minor clean to probe radius routine - updated EEPROM version to V37 according remark in MarlinFirmware#6517
320eda5
to
ca1dd73
Compare
Cleanup of #6523