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] Hangprinter support #9180

Merged

Conversation

tobbelobb
Copy link

@tobbelobb tobbelobb commented Jan 14, 2018

…for saving anchor locations in eeprom, and new defines.

New gcodes:

  • G6
  • G95 (torque mode)
  • G96 (mark encoder reference point)

Adapted gcodes that still use XYZE parameters:

  • G0/G1 (inverse kinematics)
  • G92

Adapted gcodes that use ABCDE parameters:

  • M92
  • M201
  • M204
  • M205
  • M906

Adapted gcodes with other parameters

  • M665 (set Hangprinter anchor positions)
  • M114 S1 (read encoder mm movement since reference point was marked)

New features that also work on non-Hangprinter machines:

  • UNREGISTERED_MOVE_SUPPORT
  • MECHADUINO_I2C_COMMANDS

Features that are Hangprinter specific:

  • LINE_BUILDUP_COMPENSATION_FEATURE

Other changes:

  • Uses NUM_AXIS[_N] and MOV_AXIS instead of XYZE[_N] and XYZ for indexing and initializing arrays that refer to kinematic axes (movement axes on physical machine) rather than the cartesian axes of incoming gcode.
  • Uses E_CART instead of E_AXIS for indexing arrays that refer to cartesian axes of incoming gcode.

Discussion ahead of this PR: #9101

@tobbelobb
Copy link
Author

Synced with upstream/bugfix-1.1.x, fixed issue found during travis check and squashed.

@thinkyhead
Copy link
Member

Looks good! It's almost ready to merge. I just need to go through it over the next few days to absorb all the changes. I will post review questions in the code. Once it's prepped to merge for this branch, we'll put together the same for 2.0.x. I hope to get this merged relatively soon so it won't go stale.

With the addition of E_CART I'm tempted to replace appropriate instances of X_AXIS (Y,Z) with X_CART for consistency…

@tobbelobb
Copy link
Author

Agree that X_CART, Y_CART, Z_CART for consistency would be a good thing. It's very easy to trip when listing up" X_AXIS, Y_AXIS, Z_AXIS, E_.... CART".

@thinkyhead
Copy link
Member

thinkyhead commented Jan 15, 2018

I'm thinking that for now it would be cleaner to always assume X_AXIS == X_CART (and Y, Z) and keep using X_AXIS/Y_AXIS/Z_AXIS when referring to the XYZ G-code inputs, rather than immediately induce head-scratching with X_CART as if it were distinct. For our purposes it's a reasonable synonym.

On the planner side, we already have A_AXIS/B_AXIS/C_AXIS to refer to 3 post-kinematic axes, and a D is available to shunt in before E, if wanted.

@tobbelobb
Copy link
Author

Copy/pasting in parts of my comment from the previous thread here for reference.

I don't like E_CART, but had to make a trade off. Alternatives I considered:

  • Order axes like {A_AXIS, B_AXIS, C_AXIS, E_AXIS, D_AXIS}. Breaks the assumption that motors are lined up like [X/A_AXIS + i] and extruders are lined up like [E_AXIS + i]. I found this assumption in a few different places. It's would also give us trouble the day want to add a fifth motion axis, and a couple more extruders, which is not unlikely.
  • Separate names of "cartesian" axes and kinematic axes, like {X_CART, Y_CART, Z_CART, E_CART}, {A_KINE, B_KINE, C_KINE, D_KINE, E_KINE}. Feels like a proper conceptual distinction, but adds work and would be a bigger change with the same practical drawbacks as only introducing E_CART.
  • Make completely separate arrays/variables for extruder parameters everywhere, so E_AXIS/E_CART wouldn't ever be needed. This would allow fifth and sixth motion axes, along with additional extruders, to be easily added, but it would take some more work to implement.

I'm in favour of splitting all arrays that contain both kinematic axes and extruder axes later, maybe not in this PR. As you say, keeping X_AXIS and E_CART as they are in the PR for now is the least work intensive and least headscratch-causing option in the short run.

In the longer run, some Hangprinter users might scratch their heads if E_AXIS is used in place of E_CART, which will cause "index out of bounds" error for them. Could maybe be easily exposed by the travis-script if it runs a test compilation using example_configurations/hangprinter/Configuration.h ?

@fiveangle
Copy link
Contributor

This is a brand new feature that never existed in Marlin before. Maybe it would be easier for @tobbelobb to support if only in one branch ? Otherwise, you'll have some people on 1.x and others on 2.x. Seems a bit messier with not along of benefit ? 🤷‍♂️

@tobbelobb
Copy link
Author

How is it going with the code review? Have questions come up that I can help sort out?

@thinkyhead
Copy link
Member

@fiveangle Likely this will only be merged to 2.0.x. But it's easier for some to develop for 1.1.x and then move it over afterward.

@teemuatlut
Copy link
Member

What was the change to M906? I don't see any mentions to hangprinter in it.

Also don't forget to update all the other the config files too.

Will E_AXIS be a valid macro after this?

@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch from fdabf8e to be5af5b Compare January 22, 2018 07:17
@tobbelobb
Copy link
Author

tobbelobb commented Jan 22, 2018

Synced with upstream.

@teemuatlut You're right, the former M906 code would just have put D-current == E-current. Updated M906 to take ABCDE parameters and set a separate D-current.

E_AXIS will continue to exist, but change value (from 3 to 4) when the HANGPRINTER option is enabled. Arrays that refer to XYZE (coordinate system assumed in gcode) should use E_CART instead. The arrays current_position and destination are the most important such arrays. So

current_position[E_CART]
destination[E_CART]

will work. But

current_position[E_AXIS]
destination[E_AXIS]

will break Hangprinter compatibility.

@thinkyhead I'm about to implement a few more Hangprinter features. Since Hangprinter won't be merged into 1.1.x anyways, can I push new features to this branch (bugfix-1.1.x_hangprinter branch on tobbelobb/Marlin)? If yes, then commits will show up in this PR, and I will be saved from a lot of extra rebasing and branch bookkeeping whenever I sync with upstream.

I guess keeping this PR (or another thread where I can keep you up to date and answer questions about Hangprinter code) alive is a good idea.

@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch from b7d8e93 to f780dd9 Compare January 22, 2018 16:56
@tobbelobb
Copy link
Author

tobbelobb commented Jan 22, 2018

EDIT: The reason for Travis build fail seems to be the thing that is fixed in #9302

@thinkyhead
Copy link
Member

thinkyhead commented Jan 24, 2018

@tobbelobb For an extra collaboration channel you can send me a request to connect to the Marlin project on Slack. See #9154 for details.

Don't worry about making the transition to 2.0.x while you're in the zone getting things patched up and improved here. My thinking was that once it was ready I would quickly port it over to 2.0.x — a process which is pretty quick and painless … usually — and then we would go from there. So carry on as usual.

It's true that we'll be closing some other PR's targeted to 1.1.x shortly. This one should stay open while it's underway, and just let me know when you feel it's top notch and ready to move over.

@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch 2 times, most recently from ddf4578 to 71a22f3 Compare February 8, 2018 09:49
@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch from d2508ce to 5def24b Compare February 14, 2018 14:15
@tobbelobb
Copy link
Author

I think maybe this should be merged now. It's very hard to get something like this top notch in the first try, so please bear with me. It works on my setup and has been used both for printing and auto-calibration.

It would be very good if you could look for mistakes that could break UX for cartesian printer users in there.

This is a tad nervous, but the PR is now so big that keeping up to date with upstream is eating into my time budget.

@ghost
Copy link

ghost commented Mar 2, 2018

I just discover what is hangprinter ... or spiderprint or aracnea printer , it's pure madness ....
I have seen videos on youtube , and the first thing I imagine , is the size limitless ......
Are you on Youtube ?

@tobbelobb
Copy link
Author

My Youtube channel is here: https://www.youtube.com/channel/UCw1Nz0VCw4z-dfq4WjSkFzQ

@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch from 8324dfe to 1ca1889 Compare March 2, 2018 10:08
@tobbelobb
Copy link
Author

tobbelobb commented Mar 2, 2018

Hey @thinkyhead, there were some more new breaking changes there, but I patched it all together again and squashed.

Testing the new patched version on hardware, with Hangprinter and all the other options enabled takes a few days. Within a few days, breaking changes are merged upstream, and I'm caught in a never ending interrupt loop. So I have to ask you to take a chance.

Can you please merge this now?

@tobbelobb tobbelobb force-pushed the bugfix-1.1.x_hangprinter branch from 1ca1889 to 30d6bf1 Compare March 2, 2018 10:35
@tobbelobb
Copy link
Author

Research is being done on adding computer vision feedback and also physically modelling to predict line flex (like this). Getting onto Marlin v2 and 32-bit controllers will boost our progress.

@ghost
Copy link

ghost commented Mar 2, 2018

@tobbelobb Damned !!!!! ......... 😨

@thinkyhead thinkyhead force-pushed the bugfix-1.1.x_hangprinter branch 4 times, most recently from 89ddbb5 to 9d43f48 Compare September 8, 2018 03:43
@thinkyhead thinkyhead force-pushed the bugfix-1.1.x_hangprinter branch from 9d43f48 to 25ad724 Compare September 8, 2018 04:01
Marlin/Marlin.h Outdated
#define disable_E1() NOOP
#else
#define disable_E1() E1_ENABLE_WRITE(!E_ENABLE_ON)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

@tobbelobb — Since E_STEPPERS is the same as EXTRUDERS and this block begins with #if E_STEPPERS > 1 && HAS_E1_ENABLE there's no possibility for #if ENABLED(HANGPRINTER) && EXTRUDERS == 1 to evaluate as true. So disable_E1 will always be defined as E1_ENABLE_WRITE(!E_ENABLE_ON) regardless.

Copy link
Member

@thinkyhead thinkyhead Sep 8, 2018

Choose a reason for hiding this comment

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

Applied a correction where these are undefined / redefined just below… See the commit labeled "Assign ABCD like X2, Y2, Z2" for details.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, good catch! I think the correction commit implements the desired behavior properly.

@thinkyhead thinkyhead force-pushed the bugfix-1.1.x_hangprinter branch 2 times, most recently from e5fcf95 to 36df404 Compare September 8, 2018 05:06
@thinkyhead thinkyhead force-pushed the bugfix-1.1.x_hangprinter branch from 36df404 to 463bf35 Compare September 8, 2018 05:49
@tobbelobb
Copy link
Author

Thanks for rebasing, reviewing and investing your time. Getting merged into official Marlin releases would greatly benefit the Hangprinter Community.

@thinkyhead thinkyhead merged commit 330c4bc into MarlinFirmware:bugfix-1.1.x Sep 9, 2018
thinkyhead pushed a commit that referenced this pull request Sep 22, 2018
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.

7 participants