Beam tilt correction

[johnboiles]

I'm starting to cherry-pick features and fixes from @madgrizzle's fork so that we can get them merged back upstream.

This change adds beam tilt correction -- From reading the code, it adjusts the x,y coordinates of the motors to account for the top beam being out-of-level.

Related GroundControl change is in MaslowCNC/GroundControl#779

[MaslowCommunityGardenRobot]

Congratulations on the pull request @johnboiles

Now we need to decide as a community if we want to integrate these changes. Vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up!

[johnboiles]

Some of the commits in this commit history are related to @madgrizzle's changes to chain tolerance, which I can post in a followup PR. I reverted those changes in ae35481 so that this would be a single-feature PR.

I'd recommend either squash-merging this into master OR I could squash together some of the individual commits to make more sense independently. Let me know if anyone has a strong preference.

[davidelang]

On Mon, 15 Oct 2018, John Boiles wrote: Some of the commits in this commit history are related to @madgrizzle's changes to chain tolerance, which I can post in a followup PR. I reverted those changes in ae35481 so that this would be a single-feature PR. I'd recommend either squash-merging this into master OR I could squash together some of the individual commits to make more sense independently. Let me know if anyone has a strong preference.

I think we should hold off on this for a bit, we need to be simplifying the math to figure things out, not add more variation. David Lang

[davidelang]

this isn't a long-term no, just a short term one.

please reorder/squash/rebase to make things clearer but I think we will end up wanting to do tilt correction after we do other stuff

[johnboiles]

Rebased to two commits -- changes from @madgrizzle and then my small fixes on top of that.

[blurfl]

You and @madgrizzle have been doing much good work, it's great to see it beginning to bear fruit. The G2/G3 fix(separated from the nomenclature changes) will be most welcome.

However, I agree with @DavidLang that this one might best wait until it is needed in the Kinematics calculations. At that point, it will need documentation on what units to use and how to measure it.

A couple other comments - where does the number '0.0174532925199433' come from? Too, on the Arduino, Floats have only 6-7 decimal digits of precision.
Finally, when preparing a merge, how about unselecting all the unintentional little white space changes that creep into the files.

[blurfl]

@BarbourSmith is off line for a couple days, how about closing this for a few days until he gets back?

[madgrizzle]

This change was made to assist in arriving at a solution to the "holey maslow" calibration routine (which is on the back burner for the time being). My theory is that some of the errors people see in their calibration, including me, is a result of a slightly out-of-level top beam (there are other factors.. this is just one of them). This method uses the top beam tilt to calculate discrete coordinates for each motor. The affect of tilt on the x coordinates is minute, but its significant on the y coordinates. The FW currently assumes that the center of the workarea is perfectly centered between the motors (X coordinates of motors are calculated by dividing the distance between motors by two) and it is perfectly level (Y coordinates of each motor are equal to the height of motors). If we strive for 0.5 mm accuracy, I believe that it's important that either this assumption is correct or that if its not correct, we account for it. How accurate can we reasonably measure with a level? Google search says the best bubble level is accurate to 0.3 degrees. If I calculate it correctly, a 0.3 degree tilt results in one motor being ~19 mm higher than the other. The current calibration model doesn't consider this at all and I think that's a source of error.

We can approach this issue one of three ways.. 1) ignore it, which I think is wrong; 2) incorporate top beam tilt into the calibration routine or, 3) let people manually measure tilt and enter it (of course, if you can measure it you may be able to correct it). The problem with incorporating it into the calibration routine is that the current calibration routine is somewhat flawed. I spent a lot of time on "holey maslow" but I don't think its the end-solution. The current routine could use some refinement in how adjusts the parameters, but it has that cut34offset fudge factor in it that I think is problematic (I think that's where the top beam tilt devil is hiding). We really should be reporting that value at the end of calibration to give people an indication of how "well" their calibration is. I think some work will be needed to the calibration routine to bake in top beam tilt.

My opinion is that if we get this out there and let some people with calibration issues manually try it, we might get some feedback.. either positive or negative. There's only so much @johnboiles and I can do to test things out. To use it, I would suggest people download a level app on their phone, place their phone on the top beam and enter the measurement into the software and then rerun calibration and test to see if there is an improvement.

[madgrizzle]

A couple other comments - where does the number '0.0174532925199433' come from?

It's just a conversion factor to take degrees to radians (Pi/180)... yeah, too many digits but I copied and pasted it from excel. Didn't see the harm.

[madgrizzle]

And this probably deserves its own thread somewhere, but who is developing software or working on things? I know @johnboiles and I are doing work, but is anyone else? I saw @blurfl posted an issue to FW a couple days ago, but that's about all.

[johnboiles]

I think we should hold off on this for a bit, we need to be simplifying the math
to figure things out, not add more variation.

I'll leave it to you and @madgrizzle to debate the value of beam tilt correction. In my opinion it can't hurt. It seems like a great experimental feature that the community should continue to explore. I think there's value in making experimental features easier to try for people that don't feel comfortable running from source.

With regards to simplification, I think the change here that breaks out separate variables for leftMotorX, leftMotorY, rightMotorX, rightMotorY is a simplification. In the existing code, the _xCordOfMotor variable really means more of 'x distance from the center' and it's then multiplied by -1 to get get the actual x coordinate of the left motor. Even if we decide not to merge top beam correction, what do you think about keeping the separate leftMotorX, leftMotorY, rightMotorX, rightMotorY variables? I think it's overall more readable. To make the existing code even more readable we could separate the top beam correction into separate local variables

float topBeamTiltXCorrectionFactor = cos(sysSettings.topBeamTilt*0.0174532925199433);
float topBeamTiltYCorrectionFactor = sin(sysSettings.topBeamTilt*0.0174532925199433);

leftMotorX = topBeamTiltXCorrectionFactor*sysSettings.distBetweenMotors/-2.0;
leftMotorY = topBeamTiltYCorrectionFactor*sysSettings.distBetweenMotors/-2.0 + (sysSettings.motorOffsetY+sysSettings.machineHeight/2.0);
rightMotorX = topBeamTiltXCorrectionFactor*sysSettings.distBetweenMotors+leftMotorX;
rightMotorY = topBeamTiltYCorrectionFactor*sysSettings.distBetweenMotors/2.0 + (sysSettings.motorOffsetY+sysSettings.machineHeight/2.0);

[blurfl]

I agree with @DavidLang that this should wait until it is

It's just a conversion factor to take degrees to radians (Pi/180)... yeah, too many digits but I copied and pasted it from excel. Didn't see the harm.

In the interest of clarity, how about using a named constant for it? The number of digits isn't directly harmful, but rather misleading.

who is developing software or working on things?

While github's 'Insights' view shows recent activity, it doesn't show what anyone is working on that hasn't been submitted. I've got irons in the fire looking at different processors and encoders. The Teensy 3.5/3.6 would bring welcome full floating point accuracy and a nice jump in speed. Separating the encoders from the motors could exchange chain sag for sag of a different, possibly lighter line. Neither ready for prime time, though.

If I calculate it correctly, a 0.3 degree tilt results in one motor being ~19 mm higher than the other. The current calibration model doesn't consider this at all and I think that's a source of error.

I think this is a good example of something that would be better addressed by a simple hardware fix (3/4" or 19mm shim under a frame leg) than a software complication. That hardware approach would be pretty easy to test for someone set up to do the accuracy tests. Math on the Arduino platform is not the strong suit, and should be avoided when simpler solutions are available.

[johnboiles]

However, I agree with @DavidLang that this one might best wait until it is needed in the Kinematics calculations. At that point, it will need documentation on what units to use and how to measure it.

Are you referring to the calibration routine? I've seen reports in the forums of people like @dustcloud (in this post) having better success using measured values. I personally had better success using hand-measured values than using the calibration routine. In my opinion, the more ways we can give users to represent the real-world dimensions of their build, the better, as long as the defaults are sane.

Finally, when preparing a merge, how about unselecting all the unintentional little white space changes that creep into the files.

I started to remove whitespace changes from @madgrizzle's commits but git wrestled me at every turn (since this change was originally spread across ~10 different commits). Turns out git add -p doesn't like committing whitespace-only changes. I'm open to reverting all these, but then I'll just need to fight them in the future. I think these particular whitespace changes (removing blank whitespace from ends of lines) are a net positive. But I don't feel too strongly, let me know if you do.

[johnboiles]

In the interest of clarity, how about using a named constant for it? The number of digits isn't directly harmful, but rather misleading

Looks like Arduino.h defines DEG_TO_RAD . We should use that.

[johnboiles]

Closing this until @BarbourSmith is back. Let's continue the discussion though.

[madgrizzle]

I think this is a good example of something that would be better addressed by a simple hardware fix (3/4" or 19mm shim under a frame leg) than a software complication.

I built my frame on casters so I could move it and they don't have height adjusters. I think my frame in itself is level, just that the shed it's in is not level. I'm not trying to cut all the way to the edge of plywood so I'm not worried about the workarea being level, but I'd rather not start shimming the top beam/motor mount to overcome things.. If the top beam tilt value is zero, then there is no difference in behavior between this PR and the current branch.

Math on the Arduino platform is not the strong suit, and should be avoided when simpler solutions are available.

I'd argue that the math is technically simpler since the iterative process of triangularInverse doesn't have to calculate the motor positions (they are precalculated).

[johnboiles]

I think my frame in itself is level, just that the shed it's in is not level. I'm not trying to cut all the way to the edge of plywood so I'm not worried about the workarea being level, but I'd rather not start shimming the top beam/motor mount to overcome things..

Likewise, my shop floor is far from level, so I attached my frame to the ceiling. The ceiling is more level but certainly not level either. I tried my best to adjust where I mounted the beam to get it level, but it’s not quite perfect. My best physical adjustment would be to shim the motor mounts. But that’s going to be very difficult / slow to iterate on to find the right size shim. I’d much rather tweak this value a tiny bit and then see if my calibration improves

[blurfl]

It still hasn't been demonstrated that the firmware without these changes is sensitive to leveling. I can see that a ceiling-mounted frame would be difficult to adjust for the purpose of testing, but a floor-standing one is possible.
Shimming the top beam would be a step in the wrong direction, the beam wants to be parallel to the workarea and centered on it. Leveling a piece of equipment is a pretty standard requirement when precision is wanted.
To really prove the need, runs at level and positive and negative angle should show the magnitude of the discrepancies that vary with the angle and direction.

[madgrizzle]

It still hasn't been demonstrated that the firmware without these changes is sensitive to leveling.

Maybe the simulator updates need to be added to the GC PR.. had forgot I did those.

I can see that a ceiling-mounted frame would be difficult to adjust for the purpose of testing, but a floor-standing one is possible.
Yes, but then every time I move it to get to a new sheet of plywood, I have to jack the thing up, remove the shim, lower the jack, move it, get the plywood, move it back, jack it up, reshim it, lower the jack.

Shimming the top beam would be a step in the wrong direction, the beam wants to be parallel to the workarea and centered on it. Leveling a piece of equipment is a pretty standard requirement when precision is wanted.

Yes, I agree.. if I was doing this commercially, I'd have a Rick Sanchez come and make my floor truly level (that's from an episode of a tv show). But I'm not going to relevel my shed 0.2 degrees for a hobby.

To really prove the need, runs at level and positive and negative angle should show the magnitude of the discrepancies that vary with the angle and direction.

I think using the simulator shows the effects.

[johnboiles]

Maybe the simulator updates need to be added to the GC PR.. had forgot I did those.

Oops I missed those changes.

It still hasn't been demonstrated that the firmware without these changes is sensitive to leveling.

At very least, an out-of level beam is going to affect the chain length measurements since we use the vertical sprocket to measure the chain to known lengths. But maybe if you could get the sprocket perfectly perpendicular to the angle of the beam, it wouldn't matter, as long as you don't care about alignment with the edges of the work piece. @madgrizzle does that sound right? The simulator is definitely going to show error, since it's showing you the alignment error to the work piece.

[blurfl]

I think using the simulator shows the effects.

I think what this simulator shows is the situation where the top beam isn't parallel to the workarea. I guess I misunderstood their purpose.
If that's what these settings are meant for, then adjusting the workarea sheet within the frame is a better answer, and is independent of the frames attachment to the floor or ceiling.

[davidelang]

On Tue, 16 Oct 2018, madgrizzle wrote: We can approach this issue one of three ways.. 1) ignore it, which I think is wrong; 2) incorporate top beam tilt into the calibration routine or, 3) let people manually measure tilt and enter it (of course, if you can measure it you may be able to correct it).

I am not saying we should ignore it. I'm taking the position that in the short term we need to fix the calibration and kinematics and adding more complications to the math are not good. In the meantime, have people measure the tilt and correct it. After we figure out the correct math for chain sag, and fix the calibration routine to work for level machines, then adding this sort of thing to figure out non-level machines is a good thing. But in the meantime, it's just adding more variables to an already complex problem. David Lang k

[blurfl]

an out-of level beam is going to affect the chain length measurements since we use the vertical sprocket to measure the chain to known lengths.

From above, we're talking about a 0.3 degree (19mm, 3/4") tilt, on a 10-tooth sprocket with #25 chain that's 0.05mm error on each side. That's in the order of negligible. I think that there are larger errors to find and fix.

[davidelang]

On Tue, 16 Oct 2018, John Boiles wrote: > I think the whitespace cleanup is useful, but it should be it's own commit (if not it's own PR) I'm open to this, but I really don't want the job of whitespace wrangler. What do y'all think about using clang-format or some other automatic formatter so that we can have an automated tool that can handle whitespace / formatting consistency? That way devs don't have to spend time reverting whitespace fixes made automatically by their editor.

I think it's good to have a PR checker that checks for bad whitespace, but I really don't like the idea of having anything force corrections. There are a number of tools available for developers to clean up their code before they check it in (lint for C/C++ programs see https://en.wikipedia.org/wiki/Lint_(software) ). I think it would be a good idea to define a code style to have lint enforce on the firmware (python has a separate code style checker) and then see if we can get the helpful robot to run these checks and report on the result. Then right after a release, let's have someone do a whitespace/code style cleanup and commit it.

[blurfl]

whitespace wrangler

At present, that job amounts to asking a contributor to clean up the submission using the 'Tools/AutoFormat' in the Arduino IDE.

clang-format or some other automatic formatter

So long as it's available on all platforms and reasonably easy for a contributor to set up and use, great!

[blurfl]

Then right after a release, let's have someone do a whitespace/code style
cleanup and commit it.

If we use the IDE AutoFormat tool, it would be available to all. Asking a contributor to run that on a submission seems reasonable.

[johnboiles]

I think it's good to have a PR checker that checks for bad whitespace, but I
really don't like the idea of having anything force corrections.

At present, that job amounts to asking a contributor to clean up the submission using the 'Tools/AutoFormat' in the Arduino IDE.

Just to be clear, y'all are asking me to revert good (as defined as consistent with the majority of the rest of the files in this project) whitespace changes in files that are modified in this PR.

I bet running 'Tools/AutoFormat' here would add a whole lot more whitespace changes to this PR.

I'm fine with either. I just want guidance on whether we want to do sweeping formatting fixes using an automated tool, OR optimize PRs to minimize non-code-related changes.

[davidelang]

On Tue, 16 Oct 2018, Scott Smith wrote: > Then right after a release, let's have someone do a whitespace/code style cleanup and commit it. If we use the IDE AutoFormat tool, it would be available to all. Asking a contributor to run that on a submission seems reasonable.

Long term, this is what we should do. But when multiple people are working and the old code is not clean, making the change can break other people's patches. That's why I was suggesting that right after a release we do the cleanup, and then push people to use the arduino autoformat (or equivalent) to keep things clean. David Lang

[davidelang]

On Tue, 16 Oct 2018, John Boiles wrote: > I think it's good to have a PR checker that checks for bad whitespace, but I really don't like the idea of having anything force corrections. > At present, that job amounts to asking a contributor to clean up the submission using the 'Tools/AutoFormat' in the Arduino IDE. Just to be clear, y'all are asking me to revert _good_ (as defined as consistent with the majority of the rest of the files in this project) whitespace changes in files that are modified in this PR. I bet running 'Tools/AutoFormat' here would add a whole lot more whitespace changes to this PR. I'm fine with either. I just want guidance on whether we want to do sweeping formatting fixes using an automated tool, OR optimize PRs to minimize non-code-related changes.

I would say that for _this_ PR, we should not include other correct whitespace changes, but should do those changes in a separate PR. The reasoning behind this is that the whitespace changes make it harder to see the functionality changes that this PR introduces. Now, as I'm also advocating delaying this PR, it may be that by the time we are ready to merge it, all those whitespace changes have been committed and so we will want them in here.

[johnboiles]

OK, so the plan is

1. Revert whitespace changes here
2. Sometime later run tools/autoformat on all files (preferably right after a release)
3. Require devs to also run tools/autoformat in their PRs before merging (so that they don't have to manually fight potential whitespace changes from their editor)

Is that right? That works for me; I'm happy with any future where the computer deals with whitespace manually.

[davidelang]

On Tue, 16 Oct 2018, John Boiles wrote: OK, so the plan is 1. Revert whitespace changes here 2. Sometime later run tools/autoformat on all files (preferably right after a release) 3. Require devs to also run tools/autoformat in their PRs before merging (so that they don't have to manually fight potential whitespace changes from their editor) Is that right? That works for me; I'm happy with any future where the computer deals with whitespace manually.

That's what I'm suggesting. David Lang

[blurfl]

There has only been one commit since the recent release. I could open a PR with the above format sweep (ready to submit). Would that be helpful?

[johnboiles]

I support this. At first I was concerned this was going to make generating PRs for other changes from @madgrizzle a pain, but on second thought I think I can can squash his commits together and just run autoformat on the modified files there and it should make clean PRs.

[johnboiles]

re-opened / reclosed to pull in the whitespace changes (looks like diffs don't update on closed PRs)

[BarbourSmith]

Great discussion everyone! I just want to add that y'all don't have to wait for me to merge pull requests. The goal of the automatic PR process is that I can go off the grid for a few days without worrying that I'm holding everything up.

If anyone feels like a pull request is contentious and not getting enough votes feel free to call attention to it in the forums and get more folks to vote it up or down

[johnboiles]

@BarbourSmith do you have an opinion on this one or would you rather us drive towards a community decision first? I can start a forum post if the latter

[BarbourSmith]

Let's start a community discussion and I'll participate! Now that we're moving towards a more distributed model with multiple people selling kits I can't wait to be just another average Joe 😀

I think the question for me is that if the work area is parallel to the to the top bar but the whole machine is angled by a few degrees because of a slanted floor what is the right thing to do?

[davidelang]

On Wed, 17 Oct 2018, BarbourSmith wrote: I think the question for me is that if the work area is parallel to the to the top bar but the whole machine is angled by a few degrees because of a slanted floor what is the right thing to do?

the "right thing to do" is to make the machine level :-) now, what we should do if that's not practical is a different question, and to start with, we should answer the question of how much of a difference does it make. the difference is going to matter most in the bottom corners where tension is the lowest. Could someone make something long with regular marks in it and cut it out as low on the sled as they can, then tilt the machine a significant amount (I'd say raise one end significantly higher than the other, say put the legs on one side on top of a 2x4) and then cut it again and compare the two pieces. If this is a problem, the high side should end up being shorter than the low side. Let's see how much they differ David Lang

[madgrizzle]

In a thought experiment, I tilted the machine 90 degrees clockwise. All it did was move the router up and down when the left motor turned and the right chain wrapped itself around the sprocket when the right motor turned ;)

[johnboiles]

Reopening (for voting) and started the forum discussion here

[johnboiles]

Let's start a community discussion and I'll participate! Now that we're moving towards a more distributed model with multiple people selling kits I can't wait to be just another average Joe

What if @MaslowCommunityGardenRobot started a post automatically when a PR was published (since more people are checking the forums than are checking GitHub?)

[davidelang]

On Wed, 17 Oct 2018, John Boiles wrote: What if @MaslowCommunityGardenRobot started a post automatically when a PR was published (since more people are checking the forums than are checking GitHub?)

that would just mean that the discussion is happening in two places and people will miss things said in one (with a lot of duplication between them) Please stick with one place to discuss things as much as possible. David Lang

[BarbourSmith]

I would like to make the robot at least make an announcement that a vote has been called in the forums, but I haven't gotten around to it yet

[MaslowCommunityGardenRobot]

Time is up and we're ready to merge this pull request. Great work!

[johnboiles]

Hmm I'm not sure if this is actually what the majority of people wanted based on the forum discussion. This is why it's important to vote 😅

[BarbourSmith]

Hmmm I think we got confused and voted on the wrong pull request. I feel silly now saying this but I didn't realize that there was a firmware PR and a GC pull request. I will remerge this one if the GC pull request goes through. @johnboiles sorry for the confusion and thanks for bearing with us while we figure out the whole "vote to merge" system! It's a bit messy right now but I think we'll get it worked out pretty quick