Change Encoder Steps to Float - Conflicts Fixed #344
Conversation
The encoderSteps and zEncoderSteps variables were changed from int to float, and the changeEncoderResolution function updated accordingly.
Conflicts: cnc_ctrl_v1/CNC_Functions.h
I don't know how important this the whole number of encoder steps really is. It would be more of an issue if we used the KI term in the PID calculations. We can always add back later if we think it actually contributes something important. Also changed the default values of a few related variables, the defaults are never used, but it should help future programers if the defaults at least make some sense.
|
Great! I will merge this as soon as you guys say it's good to go! |
|
Thanks for getting this sorted @krkeegan. One note. I see that the _mmPerRotation is statically calculated. I have seen the sprocket radius entered into other parts of the code as 10.2, whereas it seemed to be calculated as 10.1 here. Do you know which is the correct value? |
|
it needs to be calculated based on link size and sprocket teeth, not everyone is
using the same sprockets.
|
|
I think _mmPerRotation is part of the settings sent from GC and I think that is just a fixed number entered by the user. The value I changed is only a initialized value which can't ever be used, I thought it would just help programmers in the future if they had a default value of its size. I do see that in kinematics there is a sprocket radius hardcoded in there. That should become a variable at some point. |
|
mmPer_Rotation should be calculated from the link size and number of teeth, not
set as a float _anywhere_
people are not going to figure out the radius correctly for a sprocket, let
alone get the other stuff right.
|
|
Good point @davidelang, and calculating it based on a 10 tooth sprocket with a 0.25" pitch on 25 chain, the radius is 10.1mm, just as @krkeegan had. I agree that calculating based on chain pitch and number of sprocket teeth is the more reliable method, but that would be another PR. Also note that the value of 10.2 using in the kinematics formulas is wrong then. |
|
On Fri, 15 Dec 2017, reecej wrote:
Good point @davidelang, and calculating it based on a 10 tooth sprocket with a
0.25" pitch on #25 chain, the radius is 10.1mm, just as @krkeegan had. I agree
that calculating based on chain pitch and number of sprocket teeth is the more
reliable method, but that would be another PR.
Ok, but it should be done now as well.
Also note that the value of 10.2 using in the kinematics formulas is wrong
then.
I was told that it was calculated, not hard-coded (and didn't take the time to
look)
we definantly need to change this.
And I think this 1% difference accounts for the remaining error that was
reported after the gearbox internals fix was implemented.
|
|
If so, do we want to correct that value as well in this release? Several aspects of this code would make a machine calibration useful in the code now versus the previous release (updated motor encoder steps per revolution, updated triangular kinematics), and an updated sprocket radius would be similar as well. It might be useful to push that through and then strongly recommend users perform a calibration after upgrading to account for all changes. |
|
Since the release is next Wednesday, and we will probably be going from 0.99 to
1.0 (rather than 0.100), I think we should include every fix we can here.
We are going through this with a fine toothed comb, and have found many,
individually small, problems that affect accuracy.
I think that the aggregate of the 0.99 pid loop changes and the various accuracy
changes are just about to the point of recommending that people thow away their
.ini file and recreate it from scratch.
David Lang
P.S. again, if we could add better triangulation calibration and chain sag
calculations to this, I think we would be _very_ close to ideal
|
|
Isn't _mmPerRotation already calculated as you say? numberofteeth*chainPitch? In system.cpp, the following appears:
in axis.cpp, that calls:
You can calculate R from that (_mmPerRotation/(2*Pi)). I would just throw that calc into the changePitch routine. |
|
As pointed out, it looks like the _mmPerRotation is calculated as gearTeeth*chainPitch, which is what we want. From what I can see, the value R used in the current quadilateral and triangular kinematics is statically set to 10.2, which is not correct for the configuration of 25 chain and a 10 tooth sprocket. The difference of 10.1 vs 10.2 for the R value would only make a 1% difference on the length of chain wrapping around the sprocket, but 0.2-0.3mm error in chain length is possible. It is a simple fix, and I would recommend we either update it now to the 10.1 value or set it to be calculated similar to _mmPerRotation. |
|
Better to calculate it, as that will permit folks to use non-standard sprockets. |
|
How would you want to do that? Include CNC_Functions.h in Kinematics.h and define R = (gearTeeth x chainPitch)/(2 x 3.14159)? |
|
On Fri, 15 Dec 2017, reecej wrote:
Date: Fri, 15 Dec 2017 13:37:55 -0800
How would you want to do that? Include CNC_Functions.h in Kinematics.h and define R = (gearTeeth*chainPitch)/(2*3.14159)?
that sounds about right
make sure this is correct for the triangular kinematics as well
|
|
It should be part of the updateMotorSettings routine (or subroutine called in it, such as changePitch) |
|
#345 updates the default radius to EDIT: 10.1 |
|
Those look like the solve the concerns raised. |
|
I agree. That will also apply the needed changes for the updated triangular kinematics as well, which now incorporate the sprocket radius. |
|
Great! I'm going to merge this whole batch of pull requests then 👍 👍 😀 Great collaboration everyone! |
|
Are similar changes need in the GC simulator to keep them in sync? I believe GC has R defined statically as 10.2. |
|
Yes, this same issue will be present in the simulator. Great reminder! |
|
the simulator needs these changes, but for the main system, it should be showing
the results of the g-code, not actually calculating how the maslow gets there
(that would make gc far too dependent on implementation details)
|
Fixed the conflicts in @reecej PR #343.
The encoderSteps and zEncoderSteps variables were changed from int to float, and the changeEncoderResolution function updated accordingly.
I also removed the conversion of distance into a whole number of encoder steps. I don't know how much this is really doing for us. Plus after the fact, I realize I made that code much more complicated than necessary. We can always add this concept back in if it is necessary.
I still have not had the chance to test to see if this fixes all of the float->int errors, but it certianly looks like it does.