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

3.5 WIP: Hangprinter: Optionally allow up to 5 anchors/motors to be configured #601

Open
wants to merge 5 commits into
base: 3.5-dev
Choose a base branch
from

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Feb 25, 2023

This is a rebase of a subset of #585 which can be seen in https://github.com/GloomyAndy/RepRapFirmware/compare/v3.4-dev...jtimon:v3.4-hangprinter-5anchors?expand=1

This is not complete yet though, because I just passed by the flex stuff. I hope it doesn't take me much to "complete" this PR in that sense.

The 5 anchor version must be the simplest to optionally support. 6 should be easy too, I think.
In 3.4, the first problems appeared with 7 anchors or more, because the config messages didn't fit. which I got some guidance for here: #585 (comment)
But I didn't do it because 3.5 was out and I needed to rebase.

Currently this shouldn't work for 5 anchors yet but it also shouldn't break 4 anchor hangprinters.
I didn't test anything yet for 3.5.

But it's ready for suggestion for changes from review or testing.

This PR contains the following PRs:

TODO:

  • Fully adapt to the changes from 3.4 to 3.5
  • (kind of optional) Adapt HangprinterKinematics::MotorStepsToCartesian to N anchors for better accuracy.

@tobbelobb
Copy link
Contributor

Nice!

// Precondition: all the base anchors belong to the same plane,

I don't think we should add this precondition. Better to have an imperfect IsReachable() implementation than to place restrictions on users.

@jtimon
Copy link
Contributor Author

jtimon commented Mar 1, 2023

I don't think it's imposing restrictions on the users. on the contrary, it's allowing more configurations. Not all possible configurations yet, but more than before. Nothing that was previously allowed will be restricted, only more things will be allowed. In future PRs we can allow more configurations, but I'm afraid trying to get it right for all possible configurations in one go is going to be very hard, specially since I want to scale this to up to 8/9 anchors.

Personally I'm most interested in the configurations with all anchors on top at this moment. So my plan was to focus on that and on adding more anchors. I'm not really sure there will be much interest in supporting pseudo-pyramids with not all the anchors in the same plane, to be honest.
Anyway, if you have an alternative to #598 that is more generic than this, all welcomed.
But at this point, personally I would prefer to focus on lower hanging fruits.

@tobbelobb
Copy link
Contributor

tobbelobb commented Mar 1, 2023

Moved this comment to #598, here: #598 (comment)

@tobbelobb
Copy link
Contributor

Anyway, if you have an alternative to #598 that is more generic than this, all welcomed.

Oh, I have been commenting on the wrong PR. This one is a ok 👌
It's the other one that changes IsReachable(). Sry.

@jtimon
Copy link
Contributor Author

jtimon commented Mar 3, 2023

Well, it applies to this one too since this one contains the other one, but, yeah, I guess better discuss it there if it's specific to that.

@jtimon jtimon force-pushed the 3.5-hangprinter-5anchors branch 3 times, most recently from cc7c485 to a2b32db Compare March 5, 2023 20:56
@@ -342,32 +402,33 @@ inline float HangprinterKinematics::MotorPosToLinePos(const int32_t motorPos, si

void HangprinterKinematics::flexDistances(float const machinePos[3], float const distanceA,
Copy link
Contributor

Choose a reason for hiding this comment

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

This function must take an array of numAnchors distances that must be looped over

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, adapt everything in #569 to not using A_AXIS constants inside.
But first deal with HangprinterKinematics::StaticForces and the other changes in #562

@tobbelobb
Copy link
Contributor

Only had my phone available so only a brief review. Noticed that flex compensation isn't yet rolled into loops over numAnchors. Need to finish that before we can test on hw

Otherwise it gets too deep.
I think there's an option in eclipse to forbid this by project in
format, one can config depness iirc.
@jtimon jtimon force-pushed the 3.5-hangprinter-5anchors branch 3 times, most recently from c626607 to d0d4bd3 Compare March 21, 2023 01:24
This is a clean generalization from 4 to N.
Pseudo-pyramid because the vertices in the base of the pseudo-pyramid
don't need to be on the same plane. There's no base of the pyramid as such.

Better documented and cleaner thanks to @tobbelobb on github.
…etups

- Change from bool to class and introduce None mode for
custom (experimental) setups (all allowed)

- refactor out IsInsidePyramidSides

- refactor out IsInsidePrismSides

- introduce HangprinterAnchorMode::LastOnTop for prism-like setups
  with all anchors on top

- introduce a T parameter in M666 to configure the anchor mode
This allows to print square pyramids instead of tetrahedrons, at the
cost of an extra motor.

WIP: not everything is adapted after recent changes. See the newest TODOs
@jtimon
Copy link
Contributor Author

jtimon commented Mar 21, 2023

Only had my phone available so only a brief review. Noticed that flex compensation isn't yet rolled into loops over numAnchors. Need to finish that before we can test on hw

Yes, that's the kind of thing I mean by "Work In Progress" (WIP).
It means there are unsolved things To Do in the new comments of the PR.

But it's normal to have to rebase. When you were coding the flex stuff you had to focus on the flex stuff working and merging fine with the current branch at the time, which it did.

But if you do a PR on top of #597 but completing it in the parts related to flex which this doesn't account for, that would be awesome. But if not I will have to understand these new flex parameter at some point anyway.
With M669 and M66 there's a lot of parameters I still don't understand, which is alright as long as I don't break them.
But can I test myself if I broke them without understanding it myself?

In the case of the latest commit, I think yes, I think it's broken at the start part reserving memory dynamically.
The last commit breaks my setup, don't try in yours, try the previous commits. and on this last commit thank you for the review, keep it coming.

But when I break the wifi of my machine with the latest commit because I don't know what I'm doing with the memory...it gets me back to the times of opening the box for changing binaries in the SD or, worse, talkingto the printer by pronterface and an usb cable. Both mean opening the machine, sigh.

Just please nobody run this code on their machine unless you want to see an error and want to take out the sd card to reinstall the binaries, which is a pain in the ass.

Any tester interested in this PR should test the "previous" PR #603 which won't break your printer hopefully like this PR at this point.

@tobbelobb
Copy link
Contributor

if you do a PR on top of #597 but completing it in the parts related to flex which this doesn't account for, that would be awesome.

👍 Will look into it. Since it's not merged yet maybe I can push directly into your PR instead of opening a new one? #597

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.

2 participants