Spindle sync: Error 41 - Githubissues

nickshl commented 1 year ago

When I tried to make a thread using ioSender, I got Error:41 on G76 command.

To make it work I have to do couple things:

in function spindle_on() comment out spindleDataReset() - with this call it return 0 RPM and G76 fails
in function spindleGetData() comment out "|| (rpm_timer_delta > spindle_encoder.maximum_tt)" - something weird going on here

Another thing I noticed: SPINDLE_INDEX pin is pulled up, but SPINDLE_PULSE is not. This does not sense for me. To use NPN(open drain) encoder both pins should be pulled up. I think external pull-up is preferable, but still make sense to configure both pins same way.

I use Nucleo-F401, BOARD_MORPHO_CNC and SPINDLE_SYNC_ENABLE.

Is there a plan to add SPINDLE_SYNC_ENABLE into Web Builder?

terjeio commented 1 year ago

The $SD command shows the different count values and the angular position, is both the index and pulse counts incrementing when the spindle is running? Format is [SPINDLE:<index count>,<pulse count>,<error count>,<angular position>]. $SR resets the counters and angular position to 0. The error count should stay at 0.

Another thing I noticed: SPINDLE_INDEX pin is pulled up, but SPINDLE_PULSE is not.

SPINDLE_PULSE is "wired" as a clock input to a timer - I have to check if it is possible to enable pullup for such an input. Anyhow, external pullups is recommended as the internal ones are weak - about 50K. Lower value pullups will likely increase noise immunity.

Is there a plan to add SPINDLE_SYNC_ENABLE into Web Builder?

Eventually yes - but the spindle sync code has to be verified first. I have recently switched to a F411 (from a MSP432) in my lathe - need some time in the workshop to run tests.

nickshl commented 1 year ago

I will try $SD and $SR later today.

SPINDLE_PULSE is "wired" as a clock input to a timer - I have to check if it is possible to enable pullup for such an input.

It is definitely possible:

terjeio commented 1 year ago

Another thing I noticed: SPINDLE_INDEX pin is pulled up, but SPINDLE_PULSE is not.

Try with changing this line: https://github.com/grblHAL/STM32F4xx/blob/0b4e6c21310c5e2ca1b4528eb30810c53a33fd64/Src/driver.c#L2086 to: GPIO_Init.Pull = GPIO_PULLUP;

nickshl commented 1 year ago

This line in spindleGetData() function can stay as is: https://github.com/grblHAL/STM32F4xx/blob/0b4e6c21310c5e2ca1b4528eb30810c53a33fd64/Src/driver.c#L1297 but in order to work last_pulse should be also reset here(just compare with iMXRT1062 driver): https://github.com/grblHAL/STM32F4xx/blob/0b4e6c21310c5e2ca1b4528eb30810c53a33fd64/Src/driver.c#L1349

https://github.com/grblHAL/iMXRT1062/blob/6030db70ec51b678ef8667647a3fa20541858d99/grblHAL_Teensy4/src/driver.c#L1378

Also spindleDataReset() call still should be commented out in function spindle_on() https://github.com/grblHAL/STM32F4xx/blob/0b4e6c21310c5e2ca1b4528eb30810c53a33fd64/Src/driver.c#L1109 Otherwise function spindleGetData() gets pulse_length as 0 and it is fails.

terjeio commented 1 year ago

but in order to work last_pulse should be also reset here(just compare with iMXRT1062 driver):

A typo - I'll fix that.

Also spindleDataReset() call still should be commented out in function spindle_on()

I do not think so... It fails with error 41 if not? You have to let the spindle spin up before attempting spindle synced motion. Either set $340 to a resonable value (it is for spindle at speed tolerance) - if non-zero it will wait for the spindle to reach the programmed speed before continuing, or program a delay with G4.

nickshl commented 1 year ago

My lathe doesn't have spindle control. I set it manually to around 300 RPM - I can see it in RPM window in ioSender. It spins whole time. But when I run threading program generated by ioSender it throw Error:41 on G76 command. I will try investigate further.

nickshl commented 1 year ago

G4 didn't help. Also looks like P is in seconds, no milliseconds. RepRap.org says it should be P for milliseconds and S for seconds, ioSender doesn't know S parameter, it says "P is missing".

Anyway what happens when G76 executes:

gc_execute_block() calls spindle_sync()
spindle_sync() calls set_state()
set_state() calls spindle->set_state()
spindle->set_state() calls spindleSetStateVariable()
spindleSetStateVariable() calls spindle_on()
and spindle_on() calls spindleDataReset()

At this point pulse_length set to 0. Then(sorry, I don't want go step-by step trough whole sequence, I placed breakpoint into spindleGetData()):

gc_execute_block()(_case MotionModeThreading) calls protocol_buffer_synchronize()
protocol_buffer_synchronize() calls protocol_execute_realtime()
calls protocol_exec_rt_system()
protocol_exec_rt_system() calls report_realtime_status()
report_realtime_status() calls spindle_0->get_state()
calls spindle_0->get_state() calls spindleGetData(SpindleData_RPM)->rpm;

And pulse_length is 0 at this point because it was previously set to 0 by spindleDataReset(). It returns 0 RPM and G76 fails with Error:41. Commenting out spindleDataReset() call in spindle_on() helps exactly with that - without this call pulse_length is valid. Obviously it isn't solution. It is quick and dirty temporary fix. G76 should not call spindle_on() I guess. Spindle should be on earlier by M3 command.

terjeio commented 1 year ago

RepRap.org says it should be P for milliseconds and S for seconds, ioSender doesn't know S parameter, it says "P is missing".

RepRap is 3D printer frimware - all 3D printer firmware I've seen often divert from standard gcodes/parameters. G4 takes a P parameter in seconds according to section 3.5.4. grblHAL implements many linuxCNC extensions though - and I use their documentation as a reference, most of which is copied from more or less verbatim the NIST standard. G4 is documented here as well,

Did you set $340 to a non null value (tolerance is in percent of RPM)? And use S300 in the gcode to match the actual spindle speed?

nickshl commented 1 year ago

Did you set $340 to a non null value (tolerance is in percent of RPM)? And use S300 in the gcode to match the actual spindle speed?

This seems to work. Still don't understand why it works if spindleDataReset() commented out. Anyway, it works now. Note for $340 "Spindle at speed tolerance as percentage deviation from programmed speed, set to 0 to disable" kind of misleading. My expectations was: if it disabled - firmware should ignore any mismatch between requested and actual RPM.

Could you add "Spindle Sync" into "Advanced" portion of Web Builder?

I attached zipped driver.c - I made some minor changes by comparing it with other drivers. driver.zip

terjeio commented 1 year ago

My expectations was: if it disabled - firmware should ignore any mismatch between requested and actual RPM.

It does. However, if commanding spindle on and starting spindle synced motion immediately before it has had a chance to rotate enough to allow RPM to be calculated error 41 is returned. There is also a check for feed rate, which is dependent on the RPM, beeing less than the max Z rate configured (minus some headroom). Your best option to ensure that the spindle is running at the correct RPM is to set $340 to a reasonable value.

Could you add "Spindle Sync" into "Advanced" portion of Web Builder?

Yes, I'll do so when I am confident that it works as it should.

nickshl commented 1 year ago

Yes, I'll do so when I am confident that it works as it should.

You can make a tab "Experimental" with a big red warning "THIS FEATURES DOES NOT WELL TESTED. USE ON YOUR OWN RISK!". Threading on a lathe is a must have operation, lathe without it is just a toy. grblHAL without threading isn't useful for a lot of hobbyists - not many of those know how to install STM32CubeIDE, how to get code from GitHub(it doesn't includes submodule source code to zip which is sucks), how to configure it. And even a step by step guide will take a lot of effort to explain. I don't want to do that to every person who asks me about my lathe. Much simpler to tell "Go to this website, select a couple options and you are done".

nickshl commented 1 year ago

Now I have weird RPM values in ioSender. I using version built by Web Builder. This is the log:

<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,71|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,71|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,68|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,68|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,65|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,63|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,60|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,60|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,58|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,56|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,56|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,56|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,54|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,52|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,48|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,45|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,38|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,33|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,29|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,23|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,39|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,43|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,44|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,43|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,41|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,29|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,23|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,22|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,21|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,20|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,20|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,19|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,19|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y|WCO:0.000,0.000,0.000>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,19|Pn:Y|Ov:100,100,100>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,19|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,7|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y>
<Idle|MPos:0.000,0.000,0.000|Bf:35,1024|FS:0,0,0|Pn:Y|Ov:100,100,100>
$SD
[SPINDLE:1832,3661293,2,1832.474]
ok

3661293 / 1832 = 1998.52 Sounds about right - my encoder is 2000 PPR. Only two errors. But RPM definitely is wrong.

terjeio commented 1 year ago

I committed a fix last night but a misplaced space in the commit comment caused it to not show up here.

nickshl commented 1 year ago

Another question about sync: why is the timer used in external clock mode, not in encoder mode? It required two lines, but encoder mode is more robust: if noise is picked up by the direction line it is just ignored, if noise is picked up by the clock line it will count up on one edge and count down on another, as a result this noise will be cancelled out. Noise will affect counting only if noise is picked by both lines simultaneously. And Encoder mode also allows to check the rotation direction.

terjeio commented 1 year ago

why is the timer used in external clock mode, not in encoder mode?

Pins are typically a precious commodity.
grblHAL does not have support for ridgid tapping (maybe later).
Spindle synced motion for lathes can work with two inputs, pulse and index.
Perihpheral(s) for quadrature encoders are not always available in the MCU.

It required two lines, but encoder mode is more robust: if noise is picked up by the direction line it is just ignored

I was not aware of that. Having had issues with interrupt driven quadrature encoder input before I assumed that the hardware implementations also would prefer relatively clean inputs. But then you are talking about a type of encoders that outputs a pulse and direction signal? The one you have is like that? Does it have differential outputs as well?

nickshl commented 1 year ago

I was not aware of that. Having had issues with interrupt driven quadrature encoder input before I assumed that the hardware implementations also would prefer relatively clean inputs.

This image from ST documentation show how jitter on one line affects counter: Basically jitter does +1 -1(or -1 +1) every time when it happens.

But then you are talking about a type of encoders that outputs a pulse and direction signal? The one you have is like that? Does it have differential outputs as well?

Any quadrature encoder can be considered as pulse/dir output: If we look on diagram from left to right we will notice that channel A will have rising edge when channel B is high. And if we look on diagram from right to left it is opposite: channel A will have rising edge when channel B is low. You can connect A & B encoder outputs directly to stepper driver Step/Dir inputs and it will work(I tried). With fixed ratio of course: one pulse - one step in corresponded direction, but you can slow down movement by using microstepping.

terjeio commented 1 year ago

I have to look into the use of timers as quadrature decoders later on. An issue might be that the timers capable of this is only 16 bit wide, they will quickly overflow with a 2000 PPR encoder.

BTW I have now fixed a regression related to spindle sync and added a bit of documentation.

andrewmarles commented 1 year ago

Yes, I have looked at this a bit though not really done the software work at this point. But using the quadrature counters does rely on managing the overflow so this will limit the ultimate counting speed. From what I could gather from various STM32 examples and discussion rates up to about 5 MHz should be possible. That would be around 150K RPM with a 2000 PPR encoder so I think that ultimately the platform is capable of supporting this based on the above napkin calculations. But it is a big change in GRBLHAL from the existing implementation.

nickshl commented 1 year ago

added a bit of documentation.

Nice! However latest version doesn't work. Steps: 1) Connect controller to USB 2) Open PuTTY 3) Send G33K1Z-10 4) Result Error:41 as expected 5) Turn on spindle around 100-300 RPM(my lathe has manual spindle control) 6) Send G33K1Z-10 again 7) Controller stop responding

Firmware build using WebBuilder, BlackPill Alt. 2(STM32F411), biggest EEPROM setting, EEPROM is FRAM, and Spindle sync option enabled. Log for tunning option doesn't affect this behavior.

If I try to run threading or test program using ioSender it just stuck on G33/G76 command,

nickshl commented 1 year ago

I figured it out: there some code is missing. EXTI4_IRQHandler() should be modified, this piece of code should be added(I copied it from EXTI3_IRQHandler):

#elif SPINDLE_INDEX_PIN == 4
        if(ifg & SPINDLE_INDEX_BIT) {
            uint32_t rpm_count = RPM_COUNTER->CNT;
            spindle_encoder.timer.last_index = RPM_TIMER_COUNT;

            if(spindle_encoder.counter.index_count && (uint16_t)(rpm_count - (uint16_t)spindle_encoder.counter.last_index) != spindle_encoder.ppr)
                spindle_encoder.error_count++;

            spindle_encoder.counter.last_index = rpm_count;
            spindle_encoder.counter.index_count++;
        }
#endif

@terjeio could you commit it?

terjeio commented 1 year ago

could you commit it?

Soon, I am currently reworking the IRQ handlers to support all possible pin assignments.

grblHAL / STM32F4xx

Spindle sync: Error 41 #116