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

Preview home fix #58

Closed
wants to merge 0 commits into from
Closed

Preview home fix #58

wants to merge 0 commits into from

Conversation

AnHardt
Copy link
Owner

@AnHardt AnHardt commented Jul 17, 2016

No description provided.

@@ -1691,6 +1691,11 @@ static void do_blocking_move_to(float x, float y, float z, float feed_rate = 0.0
feedrate = old_feedrate;
}

inline void do_blocking_move_to_axis(AxisEnum axis, float where, float feed_rate = 0.0) {
Copy link

Choose a reason for hiding this comment

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

Wouldn't it make sense to be do_blocking_axis_move_to() ?

Choose a reason for hiding this comment

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

do_blocking_move_axis_to()?

Copy link
Owner Author

@AnHardt AnHardt Jul 17, 2016

Choose a reason for hiding this comment

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

Wouldn't it make sense to be do_blocking_axis_move_to() ?

No, not to me.
_to() could be _to_xyz()
_to_x()
_to_y()
_to_xy()
_to_z()
_to_axis()

Choose a reason for hiding this comment

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

Gotcha.

Copy link

Choose a reason for hiding this comment

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

Isn't the "axis moving to" ?

What is the difference between do_blocking_axis_move_to() and do_blocking_move_to() ? You want to have a single function that allows you to move axis to a coordinate vs the need to call multiple functions, each one for each axis.

From a semantics standpoint the correct term is what Scott said "do_blocking_move_axis_to", from a "sorting/grouping" standpoint yours could make sense.

Choose a reason for hiding this comment

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

After this, another audit of the move functions and what calls them, to see what they all actually, ultimately do from each place. Some unrolling might be possible then.

Copy link

@thinkyhead thinkyhead Jul 17, 2016

Choose a reason for hiding this comment

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

From a semantics standpoint

I kind-of get it. You're moving to_x or to_y and those are axes, so in to_axis the term axis stands in for x, y, or z. So… how about do_blocking_move_to_axis_pos, or…

  • do_blocking_move_to_axis_position
  • do_blocking_move_on_given_axis_to_given_position
  • do_blocking_move_to_axis_pos_updating_current_position

Copy link

Choose a reason for hiding this comment

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

If you want to review the func names I would say let's keep them as they are and do this exercise then.. in fact we already discussed about grouping all the into a new Movement object or we could use the existing Nozzle..

Nozzle::move_to_x(), Nozzle::move_to_axis() and so on.

PS: Or maybe rename Nozzle to Tool.. dunno.

Choose a reason for hiding this comment

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

All kinds of possibilities, to be sure! All I know is in the long run it's going to get less ugly. This is a lot like unraveling spaghetti.

Copy link
Owner Author

Choose a reason for hiding this comment

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

After 25 years of kite flying i know how to untangle kite lines. Software is more complicated. :-)

@thinkyhead
Copy link

MarlinFirmware#4338 is ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants