Proposed changes for the default PnCConf stepgen generated configs.

I would like to propose a few changes to the way PnCConf sets up the stepgens especially when encoders are available. I work better with diagrams than .hal files and I have attached diagrams showing my understanding of the current open/closed loop setup and my suggestions of how they could be setup. I've been using Mesa cards with steppers and encoders for the last couple of years and finally think I have my head around the way the velocity loop is set up. I occasionally toggle back and forth between open/closed loop by changing x_pos_fb from the encoder position (closed loop) to stepgen position-fb.

My proposed improvements are as follows:

1. If an encoder is attached to an axis, the encoder position should always be attached to the joint.N.motor_pos_fb. (Open loop doesn't currently do this.)
2. I think the joint.N.vel_cmd should always be connected to the pid.N.command-deriv. The trajectory planner already knows what the desired velocity is, there is not sense in estimating velocity from position within the pid.
3. If an encoder is attached to an axis, I think the encoder.N.velocity should be connected to the pid.N.feedback-deriv. With the encoder velocity calculated in the fpga it should be at least as accurate as the pid calculation.

I made these changes in my .hal files for my mill and lathe. Would it make sense for the PncConf Wizard to generate these changes directly?

I reread my post and should clarify that "open" and "closed" refer to the LinuxCNC position control loop formed around the joint and stepgen. I apply this method with both "open loop steppers" and "closed loop steppers".

On number 2, I get better results (lower following error) using the PID comps local
calculation. I suspect it some kind of pipelining issue so I think this is better left as is.

#1 sounds like an error pncconf should consider any stepper with an encoder closed loop.
Peter, so would you say 1 and 3 are a good idea then?

Yes, they both make sense.

I am a bit confused about the term openloop steppers _with_ encoder. There is no such thing.
Looking at the code, if there is an encoder designated for an axis feedback, then that is closed loop and it is connected as such.
Maybe I am missing something.

By the way your diagrams are terrific and should be in our manual when we get everthing correct.

I did modify pncconf to connect pid.N.feedback-deriv on a closed loop stepper system (ie #3)
I have posted a sample HAL file after the mod. X is closed loop the others are open.
Does this look right?

I just verified that pncconf from the 2.9.2 iso still sets up two definitions for x-pos-fb (one must be commented out). I also notice the encoder -vel-fb is already available. So I now show the x-vel-fb signal name in blue as it already exists, but draw the new proposed connection to pid.x.feedback-deriv in red.

If you decide to use any of these proposed changes, I can adjust the diagram signal names to whatever you want. I do think an additional signal name is needed for the open loop control that just uses the encoder as joint feedback.

I'm using:
#
# You must choose open/closed and comment out the other section.
#
# Open Loop Stepper operation. (Often useful to run open loop for testing out system.)
#
net x-pos-fb      <= hm2_7i96.0.stepgen.00.position-fb => pid.x.feedback
#
# Closed loop stepper operation. (Enable or disable both of these lines.)
#
net x-pos-fb      <= hm2_7i96.0.encoder.00.position    => pid.x.feedback
net x-vel-fb      <= hm2_7i96.0.encoder.00.velocity    => pid.x.feedback.deriv
 

Missed a few .hal lines:
#
# New signal that always connects encoder to the joint position feedback.
#
net x-pos-encoder <= hm2_7i96.0.encoder.00.position => joint.0.motor-pos-fb
#
# New connection to always use the trajectory planner velocity instead of an estimate.
#
net x-vel-cmd <= joint.0.vel-cmd => pid.x.command-deriv

#
# You must choose open/closed and comment out the other section.
#
# Open Loop Stepper operation. (Often useful to run open loop for testing out system.)
#
net x-pos-fb <= hm2_7i96.0.stepgen.00.position-fb => pid.x.feedback
#
# Closed loop stepper operation. (Enable or disable both of these lines.)
#
net x-pos-fb <= hm2_7i96.0.encoder.00.position => pid.x.feedback
net x-vel-fb <= hm2_7i96.0.encoder.00.velocity => pid.x.feedback.deriv

I have not pushed my changes till someone can conform they are right.
I don't have an stepper/encoder system to test with.

If you could take the sample I posted an dmark in it the lines that need to change that would be helpful.