markmaker commented 3 years ago

Description

Fixes two issues with cycle_probing.c. The fixes were actually authored by @tonyluken but he asked me to build and push this.

Issues described by @tonyluken

I've been investigating the use of the ContactProbeNozzle with TinyG. I believe I have found a bug in the TinyG code that makes nozzle probing unusable (for the most part). It appears that if the rotary axis is at any angle other than zero, the probe command fails.

With the rotary axis at zero, the gcode fragment G38.2 Z-42 F800 probes correctly. However, if the rotary axis is not at zero, the same gcode fragment is rejected by TinyG with the following error "Probing error - A axis cannot move during probing." TinyG is mistakenly thinking we want to move the rotary axis even though no command was sent to do so. I believe the bug is in this section of TinyG code in cycle_probing.c:

// error if the probe target requires a move along the A/B/C axes
for ( uint8_t axis=AXIS_A; axis<AXES; axis++ ) {
    if (fp_NE(pb.start_position[axis], pb.target[axis]))
        _probing_error_exit(axis);
}

I think this code needs to check to see if the rotary axis was actually specified in the command before checking to see if the start and target are not equal like this:

// error if the probe target requires a move along the A/B/C axes
for ( uint8_t axis=AXIS_A; axis<AXES; axis++ ) {
    if (fp_NOT_ZERO(pb.flags[axis]) && fp_NE(pb.start_position[axis], pb.target[axis]))
        _probing_error_exit(axis);
}

I currently don't have an environment setup for me to compile TinyG so I can't verify this for certain. mark@makr.zone could you compile this change? While at it, I think this section also has the logic inverted:

// trap no axes specified
if (fp_NOT_ZERO(flags[AXIS_X]) && fp_NOT_ZERO(flags[AXIS_Y]) && fp_NOT_ZERO(flags[AXIS_Z]))
    return (STAT_GCODE_AXIS_IS_MISSING);

Should be:

// trap no axes specified
if (fp_ZERO(flags[AXIS_X]) && fp_ZERO(flags[AXIS_Y]) && fp_ZERO(flags[AXIS_Z]))
    return (STAT_GCODE_AXIS_IS_MISSING);

Notes

See the discussion here: https://groups.google.com/g/openpnp/c/K2iajulHVys/m/6fDxnUxdBQAJ

This PR is based on #258 and therefore changes will appear here as well. To just see the changes of this PR, press here.

ril3y commented 3 years ago

Looking at merging this a question, however.

Why did you change 201.3 to 204.3? I am hesitant to change this as others might have UI's already relying on 201.3.

markmaker commented 3 years ago

Hi @ril3y Thanks for looking into this.

Why did you change 201.3 to 204.3?

I changed it the other way around: https://github.com/synthetos/TinyG/pull/260/commits/aab670e923640fea82525e20b2af98c21e8b2239

The reason is that TinyG does not seem to have a toolpath related jerk limit, only per axis related limits. Existing M204 implementations however, are primarily toolpath oriented i.e. they take the whole move i.e. the Euclidean distance over all the axes as the basis (actually it is quite a mess of printing vs. extruder vs. positioning moves). Some controllers do support per axis limits (e.g. Smoothieware), but that's the exception.

What swayed me is this quote:

Use M201 to set per-axis accelerations and extruder accelerations.

https://www.reprap.org/wiki/G-code#M204:_Set_default_acceleration

It seems M201 was always defining per axis limits for 2nd order/acceleration, so I believe that's a much better match to do the same for 3rd order/jerk.

https://www.reprap.org/wiki/G-code#M201:_Set_max_acceleration

others might have UI's already relying on 201.3.

I have never seen the .3 subcode before, I "invented" it myself for lack of an existing 3rd order/jerk M command. So I don't think it is issued in existing UIs.

Regards.

_Mark

ril3y commented 3 years ago

Hey long time no pull :) Could you document this feature in the wiki? I will go ahead and pull it then.

markmaker commented 3 years ago

Hi @ril3y

Just to make double sure: You are talking about merging both #258 and #260 (this one), right?

I'll go ahead with the Wiki, we can always undo this.

_Mark

markmaker commented 3 years ago

Ok, made Wiki entries:

_Mark

ril3y commented 3 years ago

Thanks @markmaker ! If you don't mind can you test out master to make sure its acting as you expected?

markmaker commented 3 years ago

Thanks, @ril3y

If you don't mind can you test out master to make sure its acting as you expected?

I have double-checked that the merged tinyg.hex is identical as mine (git hash).

However, you seem to have hard-wired/copied the firmware versions here:

http://synthetos.github.io/

This is referred to from here:

https://github.com/synthetos/TinyG/wiki/TinyG-Updating-Firmware

Your Updater app seems to equally enumerate the available binaries from there, i.e. it too is stuck at 440.20:

Updater

This seems beyond my reach to change.

_Mark

synthetos / TinyG

Fix/Probing Cycle #260

Description

Issues described by @tonyluken

Notes