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

Delta auto-calibration updates #6410

Merged
merged 18 commits into from
Apr 30, 2017
Merged

Conversation

LVD-AC
Copy link
Contributor

@LVD-AC LVD-AC commented Apr 21, 2017

Thanks to @teemuatlut, I was able to recreate

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 21, 2017

Need some help here, resolving the conflicts

LVD-AC added 3 commits April 21, 2017 11:58
- Making M665 compatible with repetier (see
http://reprap.org/wiki/G_code#M665:_Set_delta_configuration)

- M665 B also sets the radius for manual calibration menu

- Converting tower ajustment definitions to arrays - tower angle
corrections compatible with Esher 3D wizzard

- Only tower angles need to be adjustable with M665 and stored to EEPROM
- tower radius and diag rod can be adjusted in the FW only with #define
and getting rid off verbose level 3 and configuration_adv settings
- adding Cn 5, 6 and 7
@teemuatlut
Copy link
Member

Rebased and resolved conflicts.

I've done what I can without knowing more about the subject so if you can take a quick look everything is there. I only had to go through the configuration_store.cpp file.

Adding '7-point' tower angle correction calibration
@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 21, 2017

I was just one bracket short? The rest seems to be fine... Thanks a lot

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 21, 2017

Before this is merged I have 2 small changes to add. How do I proceed? I open this PR in my desktop - made the changes but I can't publish: no write access...

@teemuatlut
Copy link
Member

You can make the PR against my repo and I'll merge it then.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 21, 2017

You lost me again, how do I do that? Whatever I try Desktop tells me I do not have permission to do it... I'm done with GitHub, I'm not allowed to do anything whatsoever.

@teemuatlut
Copy link
Member

I've given you push access to my repo for a while. Maybe that'll help.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 21, 2017

No luck, I'm no longer allowed to clone your branch now... (a debug log of 3 pages cryptic error messages). I can no longer make PR either some 'validation failed' error. GitHub doesn't like me and the feeling is very mutual.

I have only made 2 little changes in marlin_main: https://github.com/LVD-AC/Marlin-LVD/commit/1b3e677711142e3297c7856f076cf3c3774682c6

* 372 M665 B delta_calibration_radius (float)
* 376 M665 X delta_tower_angle_trim[A] (float)
* 380 M665 Y delta_tower_angle_trim[B] (float)
* --- M665 Z delta_tower_angle_trim[C] (float) is always 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.

If the EEPROM layout changes, the EEPROM version should change too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I leave that EEPROM version up to you, so it's done properly

Copy link
Member

Choose a reason for hiding this comment

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

Just increment the version number by 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Not seeing it reflected in this PR yet.

Copy link
Member

@thinkyhead thinkyhead Apr 28, 2017

Choose a reason for hiding this comment

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

No need to include delta_tower_angle_trim[C] here.
For one thing, it doesn't exist. The array now has only A and B.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -461,28 +461,30 @@
// See http://minow.blogspot.com/index.html#4918805519571907051
//#define DELTA_CALIBRATION_MENU



// set the radius for the calibration probe points - max 0.8 * DELTA_PRINTABLE_RADIUS if DELTA_AUTO_CALIBRATION enabled
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need these extra blank lines.

Copy link
Contributor Author

@LVD-AC LVD-AC Apr 28, 2017

Choose a reason for hiding this comment

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

@thinkyhead
Copy link
Member

thinkyhead commented Apr 22, 2017

Overall looking pretty good. Only small spacing changes required. If needed, I can apply cleanup and squash to a single commit before merging.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 22, 2017

  •  #define DELTA_TOWER_ANGLE_TRIM {0.0, 0.0}
    

Three elements here?

The tower angles are normalized by rotating all three (ABC) until C-tower = zero.

Two options here:

  • not allowing the user to change C-value with #define and/or M665
  • allowing the input of the C-axis value and normalizing them after input

I made a mix of both approaches : so either disable the 'M665 Z' option or apply commit https://github.com/LVD-AC/Marlin-LVD/commit/065b915502a2fa00624e492a44cb9751830fecfd that allows the input of 3 values in the #define statement and normalizes them internally (makes more sense to me)

@teemuatlut : can you add that commit as well
@thinkyhead : please go ahead, to clean, squash and merge after that

More consistent with M665 where C-value is allowed as well, then
normalized
@thinkyhead
Copy link
Member

please go ahead, to clean, squash and merge

Pretty soon. Busy days.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 26, 2017

Take your time. Thanks for the help.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 26, 2017

@teemuatlut : One more commit to add, if you can beat @thinkyhead to it: https://github.com/LVD-AC/Marlin-LVD/commit/4688393e963f716f0e40e2c225624c19ada01a7a

Avoids to have to recalibrate the delta_height after a z_offset change
@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 27, 2017

@teemuatlut I added another feature : https://github.com/LVD-AC/Marlin-LVD/commit/46a6dfd33e2380a10a7e6839b8eecab665f19d88

Giving a negative number of probe points disables the tower angle
correction calibration ('4point' instead of '7point' solution)

EEPROM version updated
@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 27, 2017

@teemuatlut : Flsun FB group is pretty buzzy beta testing. Dotting some i's: https://github.com/LVD-AC/Marlin-LVD/commit/5aece3363c273cf0252adb8f693b3eb27fb607a9

@teemuatlut
Copy link
Member

I'll update tomorrow morning if you have more than one that still needs adding.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 29, 2017

did:
git fetch teemuatlut
git checkout lvd_delta_work
git merge teemuatlut/LVD-Delta

But nothing comes up in desktop??? Do I need to close that process with something like "Git push –f"?

Tried it but still nothing in the desktop?

OK got it I have to go trough all the files to see the changes made

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 29, 2017

I made 2 commits, synced it, squashed it to one commit LVD-AC@49d09f9 but when I try to make a pull request it says the usual error:

"Request failed - validation error"

@thinkyhead what now???

Didn't work in desktop, but did work online ???? Strange : teemuatlut#3

This is what I did yesterday:

- basicly gave the tests more comprehensive names; put all the
declarations at the top; got rid of the magic negative C-value (renamed
to P + A, O, T)

- "cos(RADIANS(180 + 30 * axis)) * (1 + circles * 0.1 * ((zig_zag) ? 1 :
-1)) * delta_calibration_radius" compiles wrong is zig_zag statement is
without brackets

- DELTA_TOWER_ANGLE_TRIM reset to 3 values (the calcs use the 3th value
to normalize will not compile otherwise)

-Wrote 3 dummies to keep EEPROM lenght the same

-Reset the configs to the 'original' with autocal + menu disabled (but
can be enabled of course)
@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 29, 2017

I do not know what happend in the last BugFix but my printer almost went through the roof just homing it. The speed went up by at least a factor 10. All 3 carriages cracked when trying to send the end-stops in outer space. This is undoable and this is nothing I changed. My non Git copy were I put all the same changes as in this PR (except all the changes of the last days that had nothing to do with me in RCBugFix branche) is still performing normally. I just flag this up since you want to release tomorrow. And there is serious problem there.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 29, 2017

now I'm done @teemuatlut @thinkyhead thanks for all the help

@thinkyhead
Copy link
Member

Didn't work in desktop, but did work online

Best to avoid using Github Desktop except for the simplest things like making commits, comparing changes, and making simple PRs to targets that it knows about. It forces the use of "git merge" when most of the time "git rebase" is much better.

@thinkyhead
Copy link
Member

thinkyhead commented Apr 30, 2017

So I'm curious, what happens when you use the command like git rebase -i upstream/RCBugFix on your system? Does it open up an editor so that you can fix up commits, or does it just bark?

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 30, 2017

It opens notepad to squash (if no conflicts or errors were encountered) but for fixing conflicts it loads a file in desktop, I'm using Arduino to edit them. (I have been playing a bit since @thinkyhead pointed me in the right direction by fixing that wrong upstream that blocked me from everything). (y)

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 30, 2017

That speed problem I had/have, seems to be solved in the RCBugFix branche, so I guess that will be OK once this is merged.

@thinkyhead thinkyhead changed the title Lvd delta Delta auto-calibration updates Apr 30, 2017
@thinkyhead thinkyhead merged commit 671a44b into MarlinFirmware:RCBugFix Apr 30, 2017
@thinkyhead
Copy link
Member

thinkyhead commented Apr 30, 2017

It opens notepad to squash … I'm using Arduino to edit them

Ouch. I'd recommend Sublime Text 3. It's great. With the DevIoT plugin you can build and upload Marlin from within the editor. It also has github integration and other excellent features. Makes working on the code so much easier.

I also hear NotePad++ is good for doing diffs — with the right plugin.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented Apr 30, 2017

So we made the deadline?

@thinkyhead
Copy link
Member

Looks that way! And we have an extra 18 hours for last-minute patches….

@LVD-AC
Copy link
Contributor Author

LVD-AC commented May 1, 2017

Like #6523 : change that EEPROM version @bgort spotted; managed to make that PR on my own - pretty proud it went without git spitting errors all around...

@thinkyhead
Copy link
Member

pretty proud it went without git spitting errors all around

Haha! The more I mess with git the better my luck seems to become.

@LVD-AC
Copy link
Contributor Author

LVD-AC commented May 2, 2017

Please add documentation for G33 to the RepRap wiki at your earliest convenience.

done : http://reprap.org/wiki/G-code#G33:_Delta_Auto_Calibration_.28Marlin_1.1.0.29

@teemuatlut teemuatlut deleted the LVD-Delta branch May 2, 2017 12:11
@thinkyhead
Copy link
Member

thinkyhead commented May 4, 2017

Good to have documentation!

But now as I look at the RepRap wiki, it seems like G33 should be changed to G34 (or something else) and that our UBL G26 should be changed to G33, because that would match up more closely with the definition of G33 for the two other firmwares that implement it. And, G33 does actually look like it would be a good thing for Marlin to have (instead of overloading G29 as we now do).

@thinkyhead
Copy link
Member

thinkyhead commented May 4, 2017

Actually, meh, G26 is its own thing. Still, we could theoretically support G33 for compatibility with those other firmwares… Why did we select G33 for auto-calibration?

@Roxy-3D
Copy link
Member

Roxy-3D commented May 4, 2017

We can move G26 to G33. But I do think 'Mesh Validation Pattern' is going to be important enough to have its own number. We can move it up to G123 or some other easy to remember number.

@thinkyhead
Copy link
Member

thinkyhead commented May 4, 2017

G26 is fine.

The time is nigh to document your G26 and the UBL version of G29 on the RepRap wiki GCode page, so have at it! We're supposed to document them before we publish, to "claim" the codes.

@Roxy-3D
Copy link
Member

Roxy-3D commented May 4, 2017

The time is nigh to document your G26 and the UBL version of G29 on the RepRap wiki GCode page,

I tried to do that. It won't give me a log in to make changes. They shut down the credential process and only people that originally registered can make any changes to the Wiki.

@thinkyhead
Copy link
Member

Twats.

Well, here's the page source code. Edit it as you wish, and then post the updated version here and I will publish it for you.

GCode.txt

@LVD-AC
Copy link
Contributor Author

LVD-AC commented May 4, 2017

I just did send an e-mail asking for a login, 2 days later I got a message back with my password... ???

And why G33, because a year ago when I started developing this it was available...

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.

4 participants