The latest firmware release is v0.99. The development master is at v1.00, and you might have grabbed that to match the GC development master v1.00. No rollbacks at present
Yeah, I realize my mistake. I downloaded the master branch and not the release. But it appears the only difference between the two is the version number.
What exactly is meant by “the nearest number of whole encoder steps”. If encoder steps is 8148, can you give me an example of the “nearest number of whole encoder steps”. Is it some fractional value of 8148? like 1/8148, 2/8148, 3/8148?
(I also hope the round() function in the code above simply truncates rather than rounds up or down otherwise the logic will give inaccurate results…but that’s another discussion. I can go look up the function reference myself).
I’m fairly sure in python that round() does a round (1.6 goes to 2.0). int() will truncate (1.6 goes to 1)
Tje comment in the python code is “// Ensure that _pidSetpoint is equal to whole number of encoder steps”. The operations in the code are:
Multiply by 2 --> round off --> divide by 2
My guess is that this is done for speed (multiply by 2 and divide by 2 are just bit shift operations, as opposed to floating point division) and it’s done this way to enhance the resolution of the rounding function.
My understanding is that the intent is for the output of the PID loop to be an encoder position that the motors then aim to reach. Since the encoder only counts in integer steps (e.g. it can be at the position of step 1000 but it’s not possible to resolve a position of step 1000.762) you need the output of the PID loop to be such an integer and not a floating point number.
I missed the start of this conversation so forgive me if I answer things that have already been answered or not even asked at all. I am intrigued by your problem, let me test this tonight and see if I see the same behavior.
This could be a software calculations problem which would be repeatably and symmetrical. This could also be some quirk in our interrupts that count the encoder steps, but I don’t think this one would repeat so consistently.
The firmware stores some of this data is a less than ideal manner. And I have wondered for a while if it is causing us a problem. In short, we track the position of each axis as a float of the number of rotations from zero.
When you ask an axis to move a distance, that gets translated into 635mm/mmPerRotation = 10.0. Now, we don’t want to request a position that falls in between an encoder step. So we do 10.0 rotations * encoderSteps per revolution = 81480.0 The firmware then rounds this to 81480, finally we divide by steps to get 81480/stepsPerRevolution = 10.
I added the multiply by 2 round divide by 2 code because I think I was under the impression that round operated like floor, but in looking now it doesn’t. So that is just wasted CPU time. I will mark that for fixing.
I have been worried that storing the position as a float of revolutions is an issue. Floats only have 6-7 decimal places of precision, plus I think a single integer for at best a precision of 8.
In theory, I think this should all work. We can get upwards of ~40 rotations to cover the entire work surface. 40 *8148 = 325,920 encoder steps Moving back 1 step to 325,919. 3.99998772E+2 and two steps is 3.99997545E+2.
However, there is this language on the arduino page “Floating point numbers are not exact, and may yield strange results when compared.” It has been on my todo list to convert the tracking to a long of the encoder steps. Maybe I need to speed that up.
https://www.arduino.cc/reference/en/language/variables/data-types/float/
Anyways, that was a long rambling answer that no one asked for. If anyone has a direct question about the firmware I may be able to help.
Isn’t the position based on the value of position from Encoder.h which is an int32_t representing encoder steps? If I understand, you suggest doing the math based on encoder steps - this makes sense to me.
In an aside, I wish I could have a little floating window on top of GC to display things like Axis.read() or MotorGearboxEncoder.cachedSpeed 
Also, I’ve looked at the Encoder.h code and wished that it kept count of the number of times it handled two pulses instead of one. This could give a feeling for whether we’re often nearing the limit of the interrupt response timing.
@krkeegan : makes sense, except for the bit where you say that 40 revolutions minus one step is 3.99998772E+2, which is 399.998772 revolutions… Is it 40 revolutions or 400 revolutions max?
When I saw the “x2 then round then /2” sequence, I also assumed this was done because the round() function just truncates the decimal, but it doesn’t look like it does that.
What is the maximum long integer that can be handled by python?
…also, not only should the setpoint (target) of the PID loop be an integer number of encoder steps, but also the output of the PID loop should be constrained to an integer number of encoder steps (possibly using MOD and DIV functions?)
Yep, or a long in Arduino types.
I can probably do some testing to see if this is happening.
I think I mean E+1 there. I think we need around 40 revolutions to cover the work surface I just took the total width of the worksurface / mmPerRevolution. So it is a really ballpark guess.
I think you mean c++ or arduino, but they should all be the same -2,147,483,648 to 2,147,483,647. An unsigned long, which we can use would be 0 to 4,294,967,295.
The PID loop doesn’t work that way. The positional PID output is in RPM and the Velocity output is in voltage, or we can think of them that way at least. Their units doesn’t really matter, you just tune them until you get the results you want.
Python is only limted by available RAM (arbitrarily long ints)
But the firmware is written in C, where you can use 32 bit (~±2B) or 64bit
ints
32 bit ints are more than long enough (they will support something like 250,000
revolutions or something like 10 miles of chain.
we had someone who did some work and created a version of the firmware that
didn’t use floating point (aiming for speed) and found that it was slightly
slower than the stock maslow
16 bit ints would only allow for about 8 revolutions of the sprocket, far too
little.
David Lang
remember that there are multiple PID loops.
There is the position loop
and there is the velocity loop
the velocity loop needs to be able to support very slow speeds, down to the
point where counting pulses isn’t accurate enough, you need to track the time
since the last pulse and tricks like that.
it’s pretty close, somewhere around 60 revolutions will handle even slightly
oversized machines
So, I did some testing. I removed all of the rounding and 2 arithmetic from the setpoint function and it had no effect.
I then added a quick function to output the encoder steps of each axis. Before running the command it starts at 77987. After running the command for 635mm it outputs 159467. Which is exactly where it should be. (as a side note, that is pretty cool to hit the encoder steps dead on like that).
But I totally agree that it looks like the sprocket has turned too far, maybe 12 degrees. And when I go in reverse, the sprocket returns to the right position. I even tried increasing the loopinterval of the system to 20 milliseconds and I still see roughly the same error. I also put the feed rate down at 100 (which is about as much fun as watching paint dry) and saw the same error exactly still roughly 12 degrees. It just doesn’t seem like skipped encoder steps.
By fiddling around, I would estimate ~30 steps per revolution. If this is as a result of missing encoder steps, it is remarkably consistent.
Is it at all possible that the gear ratio is 290 and not 291? Has anyone taken the gearbox apart to count the teeth? That would be 28 steps or 8120 per revolution.
If anyone is interested in trying this you can use the following commands:
G91
B09 L635 F1000
G90
When it finishes if you just click the stop button in GC it will cause the firmware to report the new location. You won’t lose your calibration. The L is the left axis R would be right the numbers represent the distance in MM plus being towards the floor, negative lifting the sled off the floor. and F is the feedrate (1000 being the max).
I tried changing the encoder steps to 8120 and ran it for 36 revolutions (i have removed the chain now). And it was still slightly over.
I bumped it to 8114 steps per revolution and that looks perfect all the way up to 50 revolutions it still looks perfect.
Is there any math that results in the number of steps being very close to 8114 per revolution?
is there any backlash in the gearbox?
If you take off the chain and try to manually turn the gearbox left and right by hand (using a lot of force since the sled normally exerts a lot of force on the chain when mounted) then if you notice there is any slack in the sprocket as you are turning it, that would indicate backlash…which could result in the symptoms you describe (namely consistent error in one direction and no error in reverse).
The interesting thing is that the gearbox is theoretically always loaded in one direction only so theoretically should not be affected by backlash.
so we could just disable the velocity loop below a certain rpm threshold.
not that I can see, that would be a ratio of 289.7857:1 with 7 PPR (28 encoder
steps/rev)
has anyone taken a gearbox apart to count the teeth?
doing the test all in one direction would eliminate backlash after the it starts
moving.
The fact that he’s taken it to 50 revolutions indicates that it’s not backlashy
There is, but not enough to cause this much movement and that doesn’t explain why the error compounds the higher the number of revolutions. If I run it for 36 loops the error is 1.5 teeth.
First this is already a solved problem, counting the time between encoder steps works very well and has a nice smooth transition from counting steps to counting time. But for academic purposes, how would this work? What would determine the voltage necessary to achieve the desired velocity?
no, we want to track the very slow rotation, otherwise we end up with the
machine doing steps instead of very gradual rotation
Yeah, I was stuck at the same point. What about 8113? That is 289.75:1. I could be just looking for reinforcement, but that seems like a decent coincidence. Someone around here had some pictures of the gearbox.