Module: STEPGEN. Timing parameters issue.

[GTCLive]

Describe the bug
The steplen and dir-hold-time timing are mixed up.

To Reproduce
In the hal config, setting setp xxx.stepgen.00.dir-setup-time 7500 will affect the timing of stepgen.00.steplen, rather than setting up a dir-setup-time of 7500 as it should.

Steps to reproduce the behavior when using LinuxCNC:
The behavior can be observed using an oscilloscope by probing the stepgen step pin output.

Expected behavior
Timing to affect the correct parameter.

Screenshots
Potential suspect at line 374 of:
https://github.com/Peter-van-Tol/LiteX-CNC/blob/main/src/litexcnc/firmware/modules/stepgen.py

372 # - source which stores the value of the counters
373 self.steplen = Signal(10)
374 self.dir_hold_time = Signal(10) <--------
375 self.dir_setup_time = Signal(12)
376 # - counters
377 self.steplen_counter = StepgenCounter(10)
378 self.dir_hold_counter = StepgenCounter(11)
379 self.dir_setup_counter = StepgenCounter(13)

LinuxCNC(please complete the following information):

• OS: Debian 12 Bookworm
• Linuxcnc Version 2.9.1

Additional context
I've yet tested if the timings of stepspace, dir-hold-time and dir-setup-time. Grateful if you could double-check if the parameters are matched correctly in the code while investigating this issue. Happy to support with traces and debug if required.

Thanks.

[GTCLive]

I've noticed the comment in:
https://github.com/Peter-van-Tol/LiteX-CNC/blob/main/src/litexcnc/driver/modules/litexcnc_stepgen.c

// Timings
// ===============
// All stepgens will use the same values for steplen, dir_hold_time and dir_setup_time.
// The maximum value is governing, so we start with the value 0. For each instance
// it is checked whether the value has changed. If it has changed, the time is
// converted to cycles.
// NOTE: all timings are in nano-seconds (1E-9), so the timing is multiplied with
// the clock-frequency and divided by 1E9. However, this might lead to issues
// with roll-over of the 32-bit integer. 
// TODO: Make the timings settings per stepgen unit

And therefore closing this for now. I'll get a set of updated (modern?) stepper drives that works and less timing-fancy rather. Mines are way too old anyway [corrosion apparent!!]

[Peter-van-Tol]

Even if closed, I will follow up on this matter. Would it be helpful for you to be able to set the timing per stepgen separately?

What kind of drive rare you using which requires such tight timing, or was the unexpected behavior due to corrosion?

I really think it is cool that you checked the timings with the scope. I only had the possibility of running simulations of the FPGA and check timings in the created output.

[GTCLive]

My existing Corroded/Old/Expired/Slow/Useless drives are min.10000 steplen. Got a new inexpensive kit currently in the mail, cost almost nothing. Those can pulse at 200Khz (STEPLEN 2500~5000) and so the current firmware will be fine. I think. Should.

"Would it be helpful for you to be able to set the timing per stepgen separately?"
Not really in all honesty (in my case anyway). Guessing everyone uses consistent motion hardware for all joints (i.e. same drives/brand/make) and so One-Fits-All config works. I wouldn't know about robotics and/or for complex kinematics though.

Yeah, resumed the work onto Litex-Cnc last weekend. Received the hat pcbs a few days ago and finally got the hardware/connectors up, and so probing all the signals to be ready. I'm a week or two from swapping this parallel port for your solution. Honestly can't wait!!

[GTCLive]

Unrelated (but related). If you find spare time and fancy toying with the STEPGEN module, see if there is a way to put up a HAL config for it that uses the PID for error comp. So far I'm down to the setup below however I can't make it work reliably. Does work in position mode without PID correction (xx.prediction?) though, but I assume this ain't reliable error comp-wise. Again though, I'm pretty new to all this and so I could be world-size wrong. Just experimenting & testing at the moment.

###INI
[EMCMOT]
EMCMOT = motmod
COMM_TIMEOUT = 1.0
SERVO_PERIOD = 2000000

###HAL
#AXIS X JOINT 0 (VELOCITY MODE)
setp pid.x.Pgain [JOINT_0]P
setp pid.x.Igain [JOINT_0]I
setp pid.x.Dgain [JOINT_0]D
setp pid.x.bias [JOINT_0]BIAS
setp pid.x.FF0 [JOINT_0]FF0
setp pid.x.FF1 [JOINT_0]FF1
setp pid.x.FF2 [JOINT_0]FF2
setp pid.x.deadband [JOINT_0]DEADBAND
setp pid.x.maxoutput [JOINT_0]MAX_OUTPUT
setp pid.x.error-previous-target true
#This setting is to limit bogus stepgen
#velocity corrections caused by position
#feedback sample time jitter.
setp pid.x.maxerror 0.012700

net x-index-enable => pid.x.index-enable
net x-enable => pid.x.enable
net x-pos-cmd => pid.x.command
net x-pos-fb => pid.x.feedback
net x-output <= pid.x.output

#Step Gen signals/setup
setp LITEXCNC.stepgen.00.steplen [JOINT_0]STEPLEN
setp LITEXCNC.stepgen.00.stepspace [JOINT_0]STEPSPACE
setp LITEXCNC.stepgen.00.dir-setup-time [JOINT_0]DIRSETUP
setp LITEXCNC.stepgen.00.dir-hold-time [JOINT_0]DIRHOLD
setp LITEXCNC.stepgen.00.position-scale [JOINT_0]STEP_SCALE
setp LITEXCNC.stepgen.00.max-acceleration [JOINT_0]STEPGEN_MAXACCEL
setp LITEXCNC.stepgen.00.max-velocity [JOINT_0]MAX_VELOCITY
setp LITEXCNC.stepgen.00.velocity-mode 1

#Openloop stepper signals (velocity mode)
net x-pos-cmd <= joint.0.motor-pos-cmd
net x-vel-cmd <= joint.0.vel-cmd
net x-output => LITEXCNC.stepgen.00.velocity-cmd
net x-pos-fb <= LITEXCNC.stepgen.00.position-feedback
net x-pos-fb => joint.0.motor-pos-fb
net x-enable <= joint.0.amp-enable-out
net x-enable => LITEXCNC.stepgen.00.enable

net x-home-sw => joint.0.home-sw-in LITEXCNC.gpio.04.in-not
net x-neg-limit => joint.0.neg-lim-sw-in
net x-pos-limit => joint.0.pos-lim-sw-in

[Peter-van-Tol]

To understand your setup better:

1. Do you need to switch between position mode and velocity mode during running your program?
2. You use PID for the open loop stepper. Do you measure the actual position with encoders (linear or rotational)?

If the answer on both answers is no, then you can use the following the following setup for your stepgen (see https://litex-cnc.readthedocs.io/en/latest/modules/stepgen.html):

loadrt [KINS]KINEMATICS
loadrt [EMCMOT]EMCMOT servo_period_nsec=[EMCMOT]SERVO_PERIOD num_joints=[KINS]JOINTS
loadrt litexcnc
loadrt litexcnc_eth config_file="[LITEXCNC]CONFIG_FILE"

# Add the functions to the thread
addf [LITEXCNC](NAME).read servo-thread
addf motion-command-handler servo-thread
addf motion-controller servo-thread
addf [LITEXCNC](NAME).write servo-thread

[...]

STEPGEN - X-AXIS
########################################################################
# - Setup of timings
setp [LITEXCNC](NAME).stepgen.00.position-scale   [JOINT_2]SCALE
setp [LITEXCNC](NAME).stepgen.00.steplen          5000
setp [LITEXCNC](NAME).stepgen.00.stepspace        5000
setp [LITEXCNC](NAME).stepgen.00.dir-hold-time    10000
setp [LITEXCNC](NAME).stepgen.00.dir-setup-time   10000
setp [LITEXCNC](NAME).stepgen.00.max-velocity     [JOINT_2]MAX_VELOCITY
setp [LITEXCNC](NAME).stepgen.00.max-acceleration [JOINT_2]STEPGEN_MAXACCEL
# setp [LITEXCNC](NAME).stepgen.00.debug 1
# - Connect velocity command
net xpos_cmd joint.0.motor-pos-cmd => [LITEXCNC](NAME).stepgen.00.position-cmd
net xpos_cmd joint.0.motor-pos-fb  <= [LITEXCNC](NAME).stepgen.00.position-prediction
# - enable the drive
net xenable joint.0.amp-enable-out => [LITEXCNC](NAME).stepgen.00.enable

Basically the FPGA is lagging one cycle behind LinuxCNC. So therefore we have to use position-prediction for position feedback. This pin contains the expected position at the end of the cycle. There is no need to use PID, as this has been built in the component, using the same algorithm as with the 'regular' stepgen which is part of LinuxCNC.

[GTCLive]

Awesome. You know what you're talking about and obviously thought of everything, and so I'll build my configs based on that.

Background: I was left assuming PID was the way to go regardless whether it's open & closed, hence myself chasing my tail in trying to make this work. PID controlled actually does work however only for tiny distances (let's say max 200mm @ +/- 0.02mm error). Clearly lot changed since 2016.
https://forum.linuxcnc.org/38-general-linuxcnc-questions/31046-open-loop-stepper-pid#75701

Peter, ref to the hal config you've suggested above, what sort of INI requirement(s) is needed for this to work reliably? Do I need the "FERROR = x.xx MIN_FERROR = x.xx" in there? and if so what sort of error is acceptable?

[hmnijp]

I ran some tests (v1.2.3), and I notice that the dir setup parameter changes the value of step lenght... (this was reported at the beginning of this issue, it is closed and I thought that this bug was already fixed, but apparently it still exists. ..)
Several other people from the local group also reported this error after they were unable to configure the step time parameters.
But I couldn’t determine where the step lenght parameter leads without looking at the source code...

[Peter-van-Tol]

Thank you for this elaborate research. Have reopened this issue and will investigate. Many thanks for putting a scope on the board!

[Peter-van-Tol]

``The order of the fields in the firmware is:

fields=[
                CSRField("steplen", size=10, offset=0, description="The length of the step pulse in clock cycles"),
                CSRField("dir_hold_time", size=10, offset=10, description="The minimum delay (in clock cycles) after a step pulse before "),
                CSRField("dir_setup_time", size=12, offset=20, description="The minimum delay (in clock cycles) after a direction change and before the next step - may be longer"),
         ] 

In the code the order is:

config_data.timings = htobe32((steplen_cycles << 22) + (dirhold_cycles << 12) + (dirsetup_cycles << 0));

The order of the fields have been revered. Mea culpa. The corrected line should read:

config_data.timings = htobe32((dirsetup_cycles << 20) + (dirhold_cycles << 10) + (steplen_cycles << 0));

When I get on a laptop I will create a branch. @hmnijp : would you be able / willing to test this with a scope?

[Peter-van-Tol]

Will also take this as an opportunity to configure each stepper separately. However, that will will be done is a separate issue / branch.

Fixing this bug won't change the communication protocol, so this bug fix can be released under 1.2.4.

Adding config for each stepper separately increases the config buffer. Thus this feature should go under version 1.3.x. This also implies that users have to recompile their firmware. This change can be together with #76

[Peter-van-Tol]

The fix has been implemented. To install the updated driver for testing purposes:

pip install git+https://https://github.com/Peter-van-Tol/LiteX-CNC@59-module-stepgen-timing-parameters-issue
sudo env "PATH=$PATH" litexcnc install_driver

This should fix the timing issue. If possible, please test. Then I can draft a new release next week.

[hmnijp]

I checked - it works) Thank you for the quick fix!

There is one more question: @romanetz seems to have told me that a warning should appear when the frequency is high and the timings no longer match (T(1/F) << StepTime+StepSpace)

In the example above - stepspace in ini = 7.5 µs, but at a frequency of 245 kHz it turns out (1/245 kHz - steptime) = 1.38 µs.. that is, a “warning” should have appeared, but nothing signals that the steps can become permanently high level...

Is this warning no longer there, or has it stopped working due to changes?

[Peter-van-Tol]

The warning is still there:

if (instance->hal.param.max_velocity > (stepgen->data.max_frequency * fabs(instance->hal.param.position_scale))) {
                // Limit speed to the maximum. This will lead to joint follow error when the higher speeds are commanded
                float max_speed_desired = instance->hal.param.max_velocity;
                instance->hal.param.max_velocity = stepgen->data.max_frequency * fabs(instance->hal.param.position_scale);
		        // Maximum speed is too high, complain about it and modify the value
		        if (!instance->memo.error_max_speed_printed) {
		            LITEXCNC_ERR_NO_DEVICE(
			            "STEPGEN: Channel %zu: The requested maximum velocity of %.2f units/sec is too high.\n",
			            i, 
                        max_speed_desired);
		            LITEXCNC_ERR_NO_DEVICE(
			            "STEPGEN: The maximum possible velocity is %.2f units/second\n",
			            instance->hal.param.max_velocity);
		            instance->memo.error_max_speed_printed = true;
		        }
	        }

So when requesting a speed higher then the ability of the stepgen, an error message should be printed. However, the method should use the inverse of the position scale...

Solved this problem and indeed a warning is displayed in my Axis.

[Peter-van-Tol]

Going to merge now. On a side note, please note the following definitions of the parameters:

        (1): 'steplen' = length of the step pulse
        (2): 'stepspace' = minimum space between step pulses, space is dependent
        on the commanded speed. The check whether the minimum step space is obeyed
        is done in the driver
        (3): 'dirhold_time' = minimum delay after a step pulse before a direction
        change - may be longer
        (4): 'dir_setup_time' = minimum delay after a direction change and before
        the next step - may be longer
                   _____         _____               _____
        STEP  ____/     \_______/     \_____________/     \______
                  |     |       |     |             |     |
        Time      |-(1)-|--(2)--|-(1)-|--(3)--|-(4)-|-(1)-|
                                              |__________________
        DIR   ________________________________/

The maximum step frequency is therefore 1 / (steplen + stepspace). I followed the definition of parameters from LinuxCNC.

[Peter-van-Tol]

Re-opened as enhancement for making the timing specific per driver.

[Peter-van-Tol]

@hmnijp : Could you help me out on one more subject?

I have pushed some changes to the PWM-module in #76 , especially the possibility to invert the output. I would like to release this branch together with the individual timings of the stepgen. Would you be willing to test the PWM with your scope?

[Peter-van-Tol]

Closed: making the timing specific per driver will be done in #87