Skip to content
This repository has been archived by the owner on Jan 14, 2024. It is now read-only.

Spindle sync discussion #50

Closed
terjeio opened this issue May 21, 2020 · 66 comments
Closed

Spindle sync discussion #50

terjeio opened this issue May 21, 2020 · 66 comments

Comments

@terjeio
Copy link
Owner

terjeio commented May 21, 2020

For implementing spindle sync for further drivers.
@terjeio terjeio mentioned this issue May 21, 2020
Open
@terjeio
Copy link
Owner Author

terjeio commented May 23, 2020

Two youtube videos of spindle sync in action. MSP432 driver.

G33 test.

G76 test.

@terjeio terjeio mentioned this issue Jun 19, 2020
Closed
@terjeio terjeio mentioned this issue Jul 21, 2020
Closed
@terjeio
Copy link
Owner Author

terjeio commented Jul 21, 2020

@hubbuis wrote here:

I have thought very long about your spindle sync implementation. I am still not sure what to expect. Maybe, after finishing the latency problem, it is time to implement G33 as well. Then, when I make an interconnection board for it, I can test this board for a long time on a lathe.

G33, G76 etc. are already implemented in the grblHAL core, they require driver support to be activated. The MSP432 MCU is the only driver that has code for that for now.

There is specific pulse start functions for normal motion and spindle synced motion, the driver will switch between them as needed by changing the HAL entry point that the core calls when it is time to start a pulse. The pulse start function gets a pointer to the current segment and and can use data contained in that to perform a lot more than just setting the direction pins and outputting step pulses if it so desires. The spindle sync version is a lot more complex than the "normal one:

grblHAL/drivers/MSP432/driver.c

Lines 351 to 443 in 0c0d6af

// Spindle sync version: sets stepper direction and pulse pins and starts a step pulse.
// Switches back to "normal" version if spindle synchronized motion is finished.
// TODO: add delayed pulse handling...
static void stepperPulseStartSynchronized (stepper_t *stepper)
{
static bool sync = false;
if(stepper->new_block) {
if(!stepper->exec_segment->spindle_sync) {
hal.stepper_pulse_start = spindle_tracker.stepper_pulse_start_normal;
hal.stepper_pulse_start(stepper);
return;
}
sync = true;
set_dir_outputs(stepper->dir_outbits);
spindle_tracker.programmed_rate = stepper->exec_block->programmed_rate;
spindle_tracker.steps_per_mm = stepper->exec_block->steps_per_mm;
spindle_tracker.segment_id = 0;
spindle_tracker.prev_pos = 0.0f;
spindle_tracker.pid.i_error = 0.0f;
spindle_tracker.pid.d_error = 0.0f;
spindle_tracker.pid.sample_rate_prev = 0.0f;
spindle_tracker.block_start = spindleGetData(SpindleData_AngularPosition).angular_position * spindle_tracker.programmed_rate;
#ifdef PID_LOG
sys.pid_log.idx = 0;
sys.pid_log.setpoint = 100.0f;
#endif
}
if(stepper->step_outbits.value) {
if(settings.steppers.pulse_delay_microseconds)
next_step_outbits = stepper->step_outbits; // Store out_bits;
else
set_step_outputs(stepper->step_outbits);
PULSE_TIMER->CTL |= TIMER_A_CTL_CLR|TIMER_A_CTL_MC1;
}
if(spindle_tracker.segment_id != stepper->exec_segment->id) {
spindle_tracker.segment_id = stepper->exec_segment->id;
if(stepper->new_block)
stepper->new_block = false;
else { // adjust this segments total time for any positional error since last segment
float actual_pos;
if(stepper->exec_segment->cruising) {
float dt = (float)hal.f_step_timer / (float)(stepper->exec_segment->cycles_per_tick * stepper->exec_segment->n_step);
actual_pos = spindleGetData(SpindleData_AngularPosition).angular_position * spindle_tracker.programmed_rate;
if(sync) {
spindle_tracker.pid.sample_rate_prev = dt;
// spindle_tracker.block_start += (actual_pos - spindle_tracker.block_start) - spindle_tracker.prev_pos;
// spindle_tracker.block_start += spindle_tracker.prev_pos;
sync = false;
}
actual_pos -= spindle_tracker.block_start;
int32_t step_delta = (int32_t)(pid(&spindle_tracker.pid, spindle_tracker.prev_pos, actual_pos, dt) * spindle_tracker.steps_per_mm);
int32_t ticks = (((int32_t)stepper->step_count + step_delta) * (int32_t)stepper->exec_segment->cycles_per_tick) / (int32_t)stepper->step_count;
stepper->exec_segment->cycles_per_tick = (uint32_t)max(ticks, (int32_t)spindle_tracker.min_cycles_per_tick);
stepperCyclesPerTick(stepper->exec_segment->cycles_per_tick);
} else
actual_pos = spindle_tracker.prev_pos;
#ifdef PID_LOG
if(sys.pid_log.idx < PID_LOG) {
sys.pid_log.target[sys.pid_log.idx] = spindle_tracker.prev_pos;
sys.pid_log.actual[sys.pid_log.idx] = actual_pos; // - spindle_tracker.prev_pos;
spindle_tracker.log[sys.pid_log.idx] = STEPPER_TIMER->BGLOAD << stepper->amass_level;
// spindle_tracker.pos[sys.pid_log.idx] = stepper->exec_segment->cycles_per_tick stepper->amass_level;
spindle_tracker.pos[sys.pid_log.idx] = stepper->exec_segment->cycles_per_tick * stepper->step_count;
STEPPER_TIMER->BGLOAD = STEPPER_TIMER->LOAD;
// spindle_tracker.pos[sys.pid_log.idx] = spindle_tracker.prev_pos;
sys.pid_log.idx++;
}
#endif
}
spindle_tracker.prev_pos = stepper->exec_segment->target_position;
}
}

Central to spindle sync is a way to determine the angular position of the spindle, grblHAL has a entry points that needs to be provided by the driver for this and other purposes. These are:

grblHAL/drivers/MSP432/driver.c

Lines 729 to 798 in 0c0d6af

static spindle_data_t spindleGetData (spindle_data_request_t request)
{
bool stopped;
uint32_t rpm_timer_delta = spindle_encoder.timer_value_last - RPM_TIMER->VALUE; // NOTE: timer is counting down!
// If no (4) spindle pulses during last 250mS assume RPM is 0
if((stopped = ((spindle_encoder.tpp == 0) || (rpm_timer_delta > spindle_encoder.maximum_tt)))) {
spindle_data.rpm = 0.0f;
rpm_timer_delta = (RPM_COUNTER->R - spindle_encoder.pulse_counter_last) * spindle_encoder.tpp;
}
switch(request) {
case SpindleData_Counters:
spindle_data.pulse_count += (RPM_COUNTER->R - spindle_encoder.pulse_counter_last);
break;
case SpindleData_RPM:
if(!stopped)
#ifdef SPINDLE_RPM_CONTROLLED
spindle_data.rpm = spindle_control.pid.enabled ? spindle_encoder.rpm : spindle_calc_rpm(spindle_encoder.tpp);
#else
spindle_data.rpm = spindle_calc_rpm(spindle_encoder.tpp);
#endif
break;
case SpindleData_AngularPosition:
spindle_data.angular_position = (float)spindle_data.index_count +
((float)(spindle_encoder.pulse_counter_last - spindle_encoder.pulse_counter_index) +
(spindle_encoder.tpp == 0 ? 0.0f : (float)rpm_timer_delta / (float)spindle_encoder.tpp)) *
spindle_encoder.pulse_distance;
break;
}
return spindle_data;
}
static void spindleDataReset (void)
{
while(spindleLock);
uint32_t systick_state = SysTick->CTRL;
SysTick->CTRL &= ~SysTick_CTRL_ENABLE_Msk;
uint32_t index_count = spindle_data.index_count + 2;
// if(spindleGetData(SpindleData_RPM).rpm > 0.0f) // wait for index pulse if running
// while(index_count != spindle_data.index_count);
#ifdef SPINDLE_RPM_CONTROLLED
if(spindle_control.pid.enabled)
spindle_control.pid_state = PIDState_Pending;
#endif
RPM_TIMER->LOAD = 0; // Reload RPM timer
RPM_COUNTER->CTL = 0;
spindle_encoder.timer_value_index = RPM_TIMER->VALUE;
spindle_encoder.pulse_counter_index = 0;
spindle_encoder.pulse_counter_last = 0;
spindle_encoder.tpp = 0;
spindle_data.pulse_count = 0;
spindle_data.index_count = 0;
RPM_COUNTER->CCR[0] = spindle_encoder.pulse_counter_trigger;
RPM_COUNTER->CTL = TIMER_A_CTL_MC__CONTINUOUS|TIMER_A_CTL_CLR;
if(systick_state & SysTick_CTRL_ENABLE_Msk)
SysTick->CTRL |= SysTick_CTRL_ENABLE_Msk;
}

The grblHAL core does not care about the underlying hardware employed to get the required data, only that the API contract is fulfilled.

These functions or rather parts of them could possibly be moved out to a plugin, but since they reference MCU specific hardware I have found it a bit tricky to so without adding processor specific code in the plugin or adding unneccesary overhead.

will be continued...

here.

@HuubBuis
Copy link

I am still waiting for DB25 connectors (got wrong once) to be able to easy change the lathe controllers and do some real testing!

In the mean time, I started analyzing the G33 code. For me, it is quit difficult to understand the code in driver.c, have to spent more time on that part

    gcode.c
 
       case MotionMode_SpindleSynchronized:
                {
                    protocol_buffer_synchronize(); // Wait until any previous moves are finished.

                    gc_override_flags_t overrides = sys.override.control; // Save current override disable status.

                    status_code_t status = init_sync_motion(&plan_data, gc_block.values.k);
                    if(status != Status_OK)
                        FAIL(status);

                    plan_data.condition.spindle.synchronized = On;

                    mc_line(gc_block.values.xyz, &plan_data);

                    protocol_buffer_synchronize();    // Wait until synchronized move is finished,
                    sys.override.control = overrides; // then restore previous override disable status.
                }
                break;

The G33 is executed between to waits for moves to finish. Doesn't this block all requests for status updates (Command ?)?

@terjeio
Copy link
Owner Author

terjeio commented Sep 1, 2020

Code in driver.c changes the timing of segments in order to keep the position correct at all times. Segment time from stepper.c is a nearly constant 5ms so that is used as the basis for the PID sampling time.

If logging is enabled log data can be fetched and plotted, nice for visualizing the PID response. My g-code sender has a tab for that.

The G33 is executed between to waits for moves to finish. Doesn't this block all requests for status updates (Command ?)?

No, it does not. protocol_execute_realtime() is called while waiting.

@HuubBuis
Copy link

HuubBuis commented Sep 9, 2020

No, it does not. protocol_execute_realtime() is called while waiting

I had to find this answer my self!

Next week I will implement G33 for the SAMD21 driver. I can not test it on my lathe (yet) because my lathe has a 5 volt logic interface. I will test it on the bench.
Maybe I will do the STMF103C8 next (has 5 Volt tolerant pins (not all)) and test it on a breadboard connected to the lathe.

@HuubBuis
Copy link

HuubBuis commented Oct 8, 2020

I finally have a working spindle sync version of the SAMD21. I have (only) tested it on the bench and it performed the same as the GRBL-L-Mega version (probably because I implemented it this way). I wasn't "a walk in the park". However I think that if we make this part more HAL, than it would take just a few hours per board to add this feature to the existing boards.
I run into a lot of issues and that raised a lot of questions. I would like to make a separate issue for each question/modification/implementation and make a link to the issue here.

@hitchhiker85
Copy link

hitchhiker85 commented Oct 24, 2020 •
edited
Loading

hi!

your work is great!
i have a unimat 3 and converted to a cnc.
now i am looking for threading.
i use a china mach3 board..with only 1tick/rev and it works!

but i like more grbl.
i have a nucleo f411, do you think you can port it to this board?

and what about backlash? is there a idea to use backladh comp?
thanks

@terjeio
Copy link
Owner Author

terjeio commented Oct 24, 2020

@hitchhiker85 The iMXRT1062 and STM32F4xx drivers are on my todo list for spindle sync.

FYI a breakout board for Nucleo-64 is nearing completion: 4-axis, Polulu, Trinamic 2130 or external drivers, optocoupled input and outputs++. Preliminary rendering of board:

Nucleo64 CNC top
Nucleo64 CNCbottom

I want this to be able to handle encoder input (minimum 2 pins) as well - and if possible a SD-card header...
What is great about the Nucleo boards is they can be programmed by drag&drop of a precompiled image.

Backlash compensation is already in the core, but I have not tested it - uncomment this line to enable:

grblHAL/grbl/config.h

Line 284 in 511ef39

//#define ENABLE_BACKLASH_COMPENSATION

@hitchhiker85
Copy link

hitchhiker85 commented Oct 24, 2020 •
edited
Loading

this is great!

breakoutboard is not important for me!

i use tb6600 stepperdrivers and ky003 hall ic (3144 IC).

i have make a own pcb.... its mote a adapter to use screwclamps for each pin.

you write drivers in progress for the 411?

its only the spindle sync what must done?

if only the spindle sync is missed i can test all other operation.. right?

thanks

for g76 threading what is realy required? spindke sync via 1 hall ic and 1 magnet?

thanks

@terjeio
Copy link
Owner Author

terjeio commented Oct 24, 2020

you write drivers in progress for the 411?

Driver is here.

its only the spindle sync what must done?

Yes, and perhaps a pin mapping file for your pcb.

if only the spindle sync is missed i can test all other operation.. right?

Yes.

for g76 threading what is realy required? spindke sync via 1 hall ic and 1 magnet?

The current implementation (for MSP432) uses two inputs, an index pulse and n-pulses per revolution (PPR) input.
It uses a PID loop that requires positional input so a bit of code other than simply handling the input interrupts is required. The MSP432 implementation has a timer clocked by the PPR input at its core.

@hitchhiker85
Copy link

hitchhiker85 commented Oct 24, 2020 •
edited
Loading

ok i understand. i must wait but g76 is in progress.

i need 2 hall ic right?

1 hole for the rpm
and N holes on a other holecircle for better reading and calculating the Rpm/position?

what do you think is the best value for N holes?

then it give us the chance to use the lathe as AAxis? if we use a servo (nema23) we can drive to a position? right? its far away but possible?

Swap Axis! like a extra Command to switch Spindle to AAxis and back. if this is possible you go near to mach3!

but how can grbl see if its turn forward or reverse?
a third hall IC on a third holecircle?

my pinout is direct to the pins on the f411.
great job!

@terjeio
Copy link
Owner Author

terjeio commented Oct 24, 2020

i need 2 hall ic right?

Good question, maybe it could work with only one like Mach3?

what do you think is the best value for N holes?

More is better? Up to a point?

I have an optical encoder disc on my lathe with 120 holes in it so 120PPR. See the videos linked above.

then it give us the chance to use the lathe as AAxis? if we use a servo (nema23) we can drive to a position? right? its far away but possible?

You mean a stepper for the spindle motor? grblHAL can set any axis to infinite length by setting the max travel distance to 0. Using a stepper for the spindle could possibly simplify the spindle sync code, but I have not looked into that.

but how can grbl see if its turn forward or reverse?

If the spindle motor is under grbl control then it knows already. If not then a quadrature encoder can be used or, if available, direction input from the motor controller.

If you are thinking of hand tapping by turning the spindle manually then that would require a high resolution encoder and code changes - not something I have considered for grblHAL.

@HuubBuis
Copy link

HuubBuis commented Oct 24, 2020 •
edited
Loading

You mean a stepper for the spindle motor? grblHAL can set any axis to infinite length by setting the max travel distance to 0. Using a stepper for the spindle could possibly simplify the spindle sync code, but I have not looked into that.

I started GRBL threading by using a stepper (Y-axis) on the spindle. Very accurate and I still use it to "repair" threads. It makes it easy to pickup the thread because you can stop (pause) at any point.
Spindle sync threading can use the "full power" of the spindle and does threading much faster. It has become my preferred way of threading on the lathe.
I have an 8 pulse encoder on the small lathe (500W servo motor, closed loop) and a 4 pulse encoder on the bigger lathe (1.5 kW DC motor, open loop). They perform equally good.
I will change the bigger lathe to a 16 pulse encoder, just to see if grbl (mega) can keep up and if not, test some other grblHAL boards.

@hitchhiker85
Copy link

hello again ;)

i test arround and... its great.. now i am waiting for spindle sync!
any idea how long it will take?? i dont know about the msp.... i will wait for the f411re version.. mach3 is good but it goes to my nuts...

thanks

@HuubBuis HuubBuis mentioned this issue Oct 26, 2020
Closed
@HuubBuis
Copy link

HuubBuis commented Oct 26, 2020 •
edited
Loading

any idea how long it will take

I am working on an easier way to implement spindle sync on supported hardware. When it is done, it would only take a few hours to implement it for an other board. I scheduled it to be ready this year.
I have setup a development environment for the MSP432 (just compiling), STM32, SAMD21, ESP32 and made the first suggestion. Other suggestions depend on this first one.

@hitchhiker85
Copy link

ok... which msp432 would work?msp432p401r?

the i will give it a try!
thanks

@terjeio
Copy link
Owner Author

terjeio commented Oct 27, 2020

any idea how long it will take??

Not too long, but I will have to put it on top on my todo list to get it done quickly. If you are able to test then this could be soon as I want to verify my ST morpho breakout design for spindle sync if there is interest (verify with my simulator).

ok... which msp432 would work?msp432p401r?

This is the one I have in my lathe: MSP432P401R LaunchPad

I have setup a development environment for the MSP432 (just compiling), STM32, SAMD21, ESP32 and made the first suggestion.

Any code made public yet?

@hitchhiker85
Copy link

hitchhiker85 commented Oct 27, 2020 via email

@HuubBuis
Copy link

Any code made public yet?

I have not published the code because it has a lot of (now needless) changes introduced to get things working. Only the SAMD21 is tested on the bench and just for G33. The changes could have introduced new bugs.
Now I have an idea on how things could be easier, I started from scratch.
Tomorrow I will make a fork and put the code there so every one can check it out. I will add a warning that it is just a test version , the final version will be substantially different and that this version will be removed as soon as the implementation in grbHALL is ready.

@terjeio
Copy link
Owner Author

terjeio commented Oct 27, 2020

all work with 5v.

Do not forget that most ARM processors do not have 5V tolerant GPIO...

@hitchhiker85
Copy link

hitchhiker85 commented Oct 27, 2020 via email

@hitchhiker85
Copy link

hitchhiker85 commented Oct 27, 2020 via email

@terjeio
Copy link
Owner Author

terjeio commented Oct 27, 2020

all my sensors work with 5v supply voltage.
not good?

Not good for connecting directly to GPIO pins - a fair chance that the processor will let out the magic smoke and die.

However, a level shifter will fix that - can be as simple as two resistors for input signals, or if a bit of signal conditioning is desirable an IC with a schmitt-triggered input such as 74LVC1G17.

@hitchhiker85
Copy link

hitchhiker85 commented Oct 27, 2020 via email •
edited
Loading

@terjeio
Copy link
Owner Author

terjeio commented Oct 27, 2020

and pure 3.3v supplyed sensors?

That will be ok.

can you please show me a pinout for the msp432?
where can i find which pin is used for what?

Here are mappings for my CNC BoosterPack, also linked to from the driver page. The index and PPR (RPM_COUNTER) pin assignments are somewhat hidden in driver.h:

#define RPM_COUNTER_PN        7 (port)
#define RPM_COUNTER_PIN       2

#define RPM_INDEX_PN    6 (port)
#define RPM_INDEX_PIN   3

@hitchhiker85
Copy link

hitchhiker85 commented Oct 27, 2020 via email

@HuubBuis
Copy link

HuubBuis commented Nov 2, 2020 •
edited
Loading

Found the PID option in the report log. It was disabled because I use compatibilitylevel 1 to get UGS working.
I changed the compatibilitylevel to 0 and made a copy of the log. Beware, the log is updated every encoder pulse, not so many points to plot.
Result for the command G91 G33 Z5 K1 running at 60 RPM:
SAMD21 PID
Result for the command G91 G33 Z-15 K1.5 running at 60 RPM:
SAMD21 PID2

@terjeio
Copy link
Owner Author

terjeio commented Nov 3, 2020

Beware, the log is updated every encoder pulse, not so many points to plot.

You mean every index pulse? You are only using input from the index pulse for your calculations?

Tell me a bit about your simulator. Mine reads the commanded RPM from the PWM output after RC filtering it and passing it to an ADC. It has a command interface where I can change parameters such as adding an offset to to the RPM input. Adding an offset can be used to check how the control loop behaves. Both with a constant offset and a step change.
I'll have to check the index pulse vs. step pulse timing for mine - after implementing spindle sync for the 600 MHz iMXRT1060 I got issues with noise - from the interrupts for step pulse and index pulse firing in random order.

So a robust way of handling encoder input is required. There are a number of options available for this depending on processor peripherals. I order to support encoders with a wide range of PPR I currently feed the encoder pulse to a counter and generate interrupts from that only after a predefined number of pulses has been counted (using a compare register). This effectively acts as a prescaler, but without loosing the accuracy of high PPR encoders, this since the actual count is available when called for.

A "simple" input pin generating interrupts directly is also an option, but IMO that should be avoided if possible as high PPR encoders may cause too much overhead.

Some processors have quadrature decoder peripherals that can be used as well, the encoder interface should support that option too. It is on my todo list for the iMXRT1060 to explore that. Down the line this could be used for tapping and hand turning of the spindle and have the Z-axis tracking the spindle movement...

I have cleaned up the spindle sync code a bit and moved data structure definitions to the core. I will commit those changes soon so you can have look.

BTW I get this result from the MSP432 driver after tweaking the encoder pulse handling code, don't know if I should believe this...

bilde

@HuubBuis
Copy link

HuubBuis commented Nov 3, 2020

You mean every index pulse? You are only using input from the index pulse for your calculations?

Every encoder pulse the z-axis speed is calculated to be (try) in sync at the next encoder pulse. The setup used is to minimize floating points calculations to suit low speed processors without FPU.

Tell me a bit about your simulator.

It is a simple arduino program that generates the pulses. On the ATTiny85 I used, I can't get the speed below 60 RPM, I don't know why. For the next setup i will try the arduino nano I used before.
SpindleSimulatorATTiny85.zip

So a robust way of handling encoder input is required

My greatest worry is picking up noise on the index or encoder signals. To avoid having to make an electronic filter, a debounce filter in software is needed (like on other inputs) that fits most users that use a simple low resolution encoder.

BTW I get this result from the MSP432 driver after tweaking the encoder pulse handling code, don't know if I should believe this...

On a simulated environment, these results are probably good. A 0.01 mm error is totally acceptable for threading.

quadrature decoder peripherals, rigid tapping

That is on my "maybe" list because it requires ball screws and this results in no manual control of the lathe.

I have cleaned up the spindle sync code a bit and moved data structure definitions to the core. I will commit those changes soon so you can have look.

I will do that

By the way, I like your approach to visualize grbl data.

@terjeio
Copy link
Owner Author

terjeio commented Nov 3, 2020

@HuubBuis updated code just commited.

Every encoder pulse the z-axis speed is calculated to be (try) in sync at the next encoder pulse. The setup used is to minimize floating points calculations to suit low speed processors without FPU.

I see that you do more calculations in the spindle pulse ISRs than I do. I wonder if some of these could be moved to the foreground process? The latest iterations of the core has a new entry point for enqueuing a callback to be executed from the foreground protocol loop. However, it could well be that the delay incurred using this is too large for spindle sync.

By the way, I like your approach to visualize grbl data.

Perhaps tuning a PID loop for spindle sync is not so scary then? ;-)

@hitchhiker85
Copy link

@HuubBuis I had look at the diff for the changes you have made - but gave up for now since it is insanenly large. Will it be possible to create a commit resulting in a diff that where it is possible to see your changes only?

I am designing a breakout board for STM32F4xx Nucleo-64 and as part of the verification for that design I am adding spindle sync capability in the same way I did for the MSP432. I will create documentation for that approach while doing so.

hi! have you some news about the stm port of your grbl version?? thanks!

@terjeio
Copy link
Owner Author

terjeio commented Nov 3, 2020

@hitchhiker85 The breakout board is ready and I will order a small batch of 10 initially, tomorrow? And I have spindle sync working with simulator input - code just committed for that.

The pin map that supports spindle sync is in st_morpho_map.h, note that the pin used for the spindle encoder pulse is fixed (SPINDLE_PULSE_PORT -> D.2) as it is wired internally in the MCU to a timer clock input. This means that spindle sync is currently limited to Nucleo-64 boards, the Blackpill boards does not have this pin brought out.

If you want to test this code then be aware that I regard it as alpha quality. Be extremely careful if doing so when connected to a lathe! Dry runs with a hand on the estop button recommended.

My breakout board has 5V tolerant input buffers for the spindle encoder signals, the dev board on its own is limited to 3.3V.

@HuubBuis
Copy link

HuubBuis commented Nov 3, 2020

I see that you do more calculations in the spindle pulse ISRs than I do. I wonder if some of these could be moved to the foreground process?

In the stepper pulse interrupt there is a float addition and In the Encoder interrupt, there a few float calculations. These can sure be moved to the foreground process (where the new z-axis rate is set).

Perhaps tuning a PID loop for spindle sync is not so scary then? ;-)

Still scary but I am a bit more assured. The real test starts when using this with the default values.

Example of log from very early test with the STM32F411 driver, simulated pulse inputs:

Looking at the graph, increasing the I value 5 times, would bring the error fluctuating around 0 in about 2 mm z-axis travel.

@terjeio
Copy link
Owner Author

terjeio commented Nov 4, 2020

It is a simple arduino program that generates the pulses. On the ATTiny85 I used, I can't get the speed below 60 RPM, I don't know why. For the next setup i will try the arduino nano I used before.

Buy yourself a MSP-EXP430G2 LaunchPad and use my code?

I have just finished tuning the simulator code, most of the noise seen was not due to ADC jitter, it was bad programming. I use two PWM generators for generating the pulse outputs, what I did wrong was that I updated PWM registers asynchronously to pulse output generation leading to random changes in pulse width. Changing to synchronous updates cured that. I have also changed the main pulse output to be 50% duty cycle. With a bit of tweaking it should be possible to simulate a quadrature encoder as well?

My greatest worry is picking up noise on the index or encoder signals. To avoid having to make an electronic filter, a debounce filter in software is needed (like on other inputs) that fits most users that use a simple low resolution encoder.

Adding software debounce is not what I would do - as it will add more jitter to pulse timing? IMO it is better to clean the signals electronically if required. Ferrites on the cables is the first ting to try.

Looking at the graph, increasing the I value 5 times, would bring the error fluctuating around 0 in about 2 mm z-axis travel.

I have not spent much time on PID loop tuning yet - my main focus has been on encoder pulse handling. Counting the main pulse should be done on the rising edge and index pulse on the falling edge? Or vice versa depending on the encoder? Or perhaps taking care to get the related interrupt priorities right? As I wrote above I want robust encoder pulse handling.

@HuubBuis
Copy link

HuubBuis commented Nov 4, 2020 •
edited
Loading

Buy yourself a MSP-EXP430G2 LaunchPad and use my code?

It would make things a lot easier if I could also test grblHAL using your board. I have looked for a MSP432P401 board but in the Netherlands I have to order 3 of them to reach the minimum order limit. As soon as I can reach this order limit by adding other things I need, I will order one.
The MSP-EXP430G2 is easier to get but for simulating index and encoder pulses it is a bit overkill?

Adding software debounce is not what I would do - as it will add more jitter to pulse timing? IMO it is better to clean the signals electronically if required. Ferrites on the cables is the first ting to try.

I think most grbl users have no tools or skills to solve glitches this way. Software debounce solved problems on both of my lathes.
The timing of the stepper pulse, running at the highest priority interrupt shouldn't be effected by debouncing.
At the receive of an pulse, all data needed can be saved but the processing could ignored if after debouncing the pulse has gone.

Counting the main pulse should be done on the rising edge and index pulse on the falling edge?

As long as the time between the index and the encoder pulse is long enough to do the threading preparations, it doesn't matter. The location of the index magnet is between the encoder magnets on my lathes, so I can change the spindle direction for left hand threading!

As I wrote above I want robust encoder pulse handling.

me to!

@terjeio
Copy link
Owner Author

terjeio commented Nov 4, 2020

The MSP-EXP430G2 is easier to get but for simulating index and encoder pulses it is a bit overkill?

I do not think so, it is a 16 bit processor with limited RAM (up to 512 bytes) and flash. IMO perfect for the simulator. I could have used a 8 bit processor and the Arduino IDE. But I want to be able to properly debug so not an option for me. Did you follow the link I provided?

As long as the time between the index and the encoder pulse is long enough to do the threading preparations, it doesn't matter. The location of the index magnet is between the encoder magnets on my lathes, so I can change the spindle direction for left hand threading!

That is your encoder, some commercial encoders are build differently, as is mine that is based on photo interrupters and where the index pulse coincides with a count pulse. Limiting spindle sync to a specific type of encoder is not what I want, grblHAL should support both home made and commercial ones. There are commercial encoders out there capable of delivering > 300K pulses per second at 6000 RPM and above (not that I am aiming to support those). At least some of the grblHAL drivers should be able to handle 1024 PPR as a minimum, or perhaps up to 4096 PPR?

@terjeio
Copy link
Owner Author

terjeio commented Nov 4, 2020

As I wrote above I want robust encoder pulse handling.

me to!

I am considering bringing back the error flag and adding it to the status report so it can be displayed in the sender.
It is set when the configured PPR does not match the actual count. What do you think?

MSP432 index pulse handler:

    if(iflags & RPM_INDEX_BIT) {

        if(!spindle_encoder.error && spindle_data.index_count)
            spindle_encoder.error = (RPM_COUNTER->R - (uint16_t)spindle_encoder.counter.last_index) != spindle_encoder.ppr;

        spindle_encoder.counter.last_index = RPM_COUNTER->R;
        spindle_encoder.timer.last_index = RPM_TIMER->VALUE;
        spindle_data.index_count++;
    }

@HuubBuis
Copy link

HuubBuis commented Nov 4, 2020

That is your encoder, some commercial encoders are build differently, as is mine that is based on photo interrupters and where the index pulse coincides with a count pulse.

That shouldn't be a problem if you trigger on the same edge and the interrupt for the encoder pulse has a higher priority than the interrupt for the index pulse. Than after waiting for the index pulse, followed by waiting for next encoder pulse, all threading starts at the same point.

MSP432 index pulse handler:

That could give a problem when there is an encoder pulse during the processing of the report. But that depends on the interrupt priority of the index pulse.
I have considered comparing the time between index pulses and the last encoder pulse times ppr at start of threading. I dropped it because the most problems are to expected during threading setup. If there is ever a missing encoder pulse (magnet) , it will be seen immediately at threading start.

When the Z-axis is running in sync, I can hear that the encoder magnets (lathe original rpm measurement) are not placed well. I am making a new UHMPE encoder disk where the 16 magnets (3 mm) are placed in drilled holes. The holes will be tight enough to hold the magnets while running but the magnets can still be moved away or towards the sensor. This will allow adjusting the timing a bit.
I can use my oscilloscope to measure the pulses but not everyone has this. The time between encoder pulses are measured pretty accurate. If we fill the log buffer with these values, one for every encoder pulse (starting at the first encoder pulse after an index pulse) the pulse time for every magnet can be showed using your sender. That only requires a few lines of code and it can show misaligned and missing magnets.
The trigger for filling the buffer could be the command that reads this buffer. As long as there is no threading going, it would show the alignment of the magnets.
I could adjust my SAMD21 version tomorrow!

@terjeio
Copy link
Owner Author

terjeio commented Nov 4, 2020

That is your encoder, some commercial encoders are build differently, as is mine that is based on photo interrupters and where the index pulse coincides with a count pulse.

That shouldn't be a problem if you trigger on the same edge and the interrupt for the encoder pulse has a higher priority than the interrupt for the index pulse. Than after waiting for the index pulse, followed by waiting for next encoder pulse, all threading starts at the same point.

I'll go for the pulse edges as I cannot guarantee that the pulses arrive at the same time with sub microsecond accuracy. E.g the STM32F446 runs at 180 MHz...

I can use my oscilloscope to measure the pulses but not everyone has this. The time between encoder pulses are measured pretty accurate. If we fill the log buffer with these values, one for every encoder pulse (starting at the first encoder pulse after an index pulse) the pulse time for every magnet can be showed using your sender.

I am not going to add this to my sender unless it is just a sequence of numbers to be picked up from the console. A better alternative could to add it to my spindle linearization application that is built on top of the sender libraries. This since the task at hand is to be performed once and does not require full gcode sending capability. I have not yet published this application, should perhaps have done so earlier as I moved the data points required for spindle linearization from #defines to $-settings.

MSP432 index pulse handler:

That could give a problem when there is an encoder pulse during the processing of the report. But that depends on the interrupt priority of the index pulse.

I do not understand this, the report is generated in the foreground and does not block interrupts. Transmitting the characters does but not for long enough to be an issue.

I have considered comparing the time between index pulses and the last encoder pulse times ppr at start of threading. I dropped it because the most problems are to expected during threading setup. If there is ever a missing encoder pulse (magnet) , it will be seen immediately at threading start.

That will depend on the PPR. I timestamp both pulse events with at least microsecond resolution (but not accuracy) and use this when calculating the angular position. By doing so the resolution of this calculation is better than the resolution that the encoder provides. For some processors it may even be possible to timestamp the events with absolute accuracy by routing the pulse signals to trigger capture events for the timestamp timer. However that is likely to be an overkill - but would be fun to try one day...

FYI the SAMD21 has a 32 bit timer for the real-time clock that can be used to generate timestamps with ~30us resolution.

@HuubBuis
Copy link

HuubBuis commented Nov 4, 2020

I am not going to add this to my sender unless it is just a sequence of numbers to be picked up from the console.

There is no need to change the sender. I will modify the SAMD21 version tomorrow.

I do not understand this, the report is generated in the foreground and does not block interrupts. Transmitting the characters does but not for long enough to be an issue.

You are right,

it will be seen immediately at threading start.

I mean that the threaded bar will show the missing pulse at the first cut because one of the pulses will be twice as long, the z-axis speed will drop and the thread will look awful (for low resolution encoders).

@HuubBuis
Copy link

HuubBuis commented Nov 5, 2020

There is no need to change the sender. I will modify the SAMD21 version tomorrow.

I have modified the SAMD21 version to measure the pulse width from the encoder. I also changed the spindle simulator to make the first pulse after the index pulse 10 us longer (pulse time 250 us). When requesting the PID log a second time, without doing a new threading pass, the measured pulse width is send (actual position). The first pulse width after the index pulse is used as reference (target).
The two graphs showing the result of the synchronization and the measured pulse width. Compared to the previous graph where the pulse width are equal, there is a noticeably difference.
I have committed the changed SAMD21 code https://github.com/HuubBuis/grblHALtest

Result for the command G91 G33 Z-15 K1.5 running at 60 RPM, one pulse 10 ms longer:
MisallignedMagnetThreading
The graph showing the misalligned second pulse:
MisallignedMagnet

@terjeio
Copy link
Owner Author

terjeio commented Nov 6, 2020

Your code with my 120PPR simulated encoder and a MKRZERO board:

bilde

Spindle speed does not report live at all times as it should since you have modified the real time reporting. Also the RPM reported when it does is off by some 20% - don't know yet if this is the PWM output beeing off spec or your RPM calculation beeing incorrect. The missing debugger in the Arduino IDE is a pain...

If you want me to do a code review the please update to the latest version of grblHAL and add your changes to that so I can find them without hunting around.

@HuubBuis
Copy link

HuubBuis commented Nov 6, 2020 •
edited
Loading

Spindle speed does not report live at all times as it should since you have modified the real time reporting.

The change should show the measured RPM if the RPM is not controlled by grbl. This is done because the RPM shown on my lathe have a slow response.

Also the RPM reported when it does is off by some 20% - don't know yet if this is the PWM output beeing off spec or your RPM calculation beeing incorrect.

The index pulse frequency measured on my oscilloscope is 0.988 Hz, your sender reports 59 RPM. I can't reproduce this error!

If you want me to do a code review the please update to the latest version of grblHAL and add your changes to that so I can find them without hunting around.

I have cleaned up the spindle sync code a bit and moved data structure definitions to the core. I will commit those changes soon so you can have look.

Have you committed these changes yet?

The missing debugger in the Arduino IDE is a pain...

Would it help if i added the complete Atmel Studio 7 project in a zip file?

@terjeio
Copy link
Owner Author

terjeio commented Nov 6, 2020

The index pulse frequency measured on my oscilloscope is 0.988 Hz, your sender reports 59 RPM. I can't reproduce this error!

Well, as I wrote it could be the PWM beeing off. I am interested in seeing the changes you have made to the core before I am spending more time on your implementation. It looks like you are not using the HAL API as you should. E.g spindle sync and spindle at speed capabilities are not communicated to the core, and it seems to me that you have added another way to detect the initial index pulse before starting a synced move? IMO we need to discuss the reasons for any core changes since it is shared among all drivers, perhaps you can use the existing API for some of them?

I have cleaned up the spindle sync code a bit and moved data structure definitions to the core. I will commit those changes soon so you can have look.

Have you committed these changes yet?

Yes.

Would it help if i added the complete Atmel Studio 7 project in a zip file?

No, there is no way to debug the MKRZERO that I have that I am aware of. And I need a new development machine - disks are near capacity so I am not going to install another IDE on it. I will order one soon, but do not look forward to setting it up...

@HuubBuis
Copy link

HuubBuis commented Nov 6, 2020

I made a lot of changes, not because they are better but just to get things working. In the end, I tried threading the way it is implemented in grbl-L-Mega. I now have a "working" totally messed up threading implementation. This code is not suitable to merge in grblHAL but the exercise helped me understand (a bit) how you have done it.

perhaps you can use the existing API for some of them?

I think in general your implementation is OK and on some points better than mine in grbl-L-Mega. I am concerned about the CPU load for processors that don't have a FPU and the debouncing of signals. The only way to check this is by "fully" implementing you API, measure how long calculations are on different boards and do a real test on a lathe.
Implementing threading should be easier and I think it must be possible without changing your approach. I start from scratch and try to accomplish this.

Have you committed these changes yet?
Yes.

I will have a look at the changes. Are these changes in the Master or the Test branch?

there is no way to debug the MKRZERO that I have that I am aware of

There are smd pads for a debug header on the bottom. In general I don't use boards that don't have a debug header.

disks are near capacity

I have the same problem. I have to reserve a week to do a clean install but I try to postpone this as long as possible.

@terjeio
Copy link
Owner Author

terjeio commented Nov 7, 2020

I start from scratch and try to accomplish this.

Start with implementing the basics first. This is the code needed to calculate RPM, the angular position and to query the pulse counts. hal.spindle.get_data (-> spindleGetData), hal.spindle.reset_data (->spindleDataReset) and the related ISRs is at the core of that. The ISRs are simple, only a few lines of code and no floating point calculations.
Perhaps opening an issue covering this bit could be useful?

I will have a look at the changes. Are these changes in the Master or the Test branch?

In the master. Use the STM32F4xx or iMXRT1062 driver as a reference, not the MSP32 as this has code for closed loop spindle RPM handling that may be confusing.

There are smd pads for a debug header on the bottom. In general I don't use boards that don't have a debug header.

Thanks, I was not aware of that.

Implementing threading should be easier and I think it must be possible without changing your approach. I start from scratch and try to accomplish this.

What could be useful is somehow moving the floating point calculations to the foreground. Eg. the ESP32 crashes if a floating point variable is even accessed in an ISR... And performing floating point calculations in an ISR using a software library is IMO not desirable.

@HuubBuis
Copy link

HuubBuis commented Nov 7, 2020

Start with implementing the basics first. This is the code needed to calculate RPM, the angular position and to query the pulse counts. hal.spindle.get_data (-> spindleGetData), hal.spindle.reset_data (->spindleDataReset) and the related ISRs is at the core of that. The ISRs are simple, only a few lines of code and no floating point calculations.

The RPM is calculated based on the time between pulses (tics) and the encoder revolution. The calculation itself is the same for all drivers and could be part of the core. This could also be applied to the angular position.
Querying the driver for pulse and timer counts could be initiated by the core so the driver just needs to provide the basic data and signal the core the receive of events. It is basically the same as how other events (door, probe, etc) are handled.

Example: On the receive of an Index pulse, the driver (ISR) signals the core (API call) the receive of an Index pulse. The core decides what information should be requested (current time tics) and query's the driver for this information. The core keeps track of the index pulse interrupt count and timer tic count.

What could be useful is somehow moving the floating point calculations to the foreground.

I think this is possible and it fits my idea to separate tasks:

  • ISR, driver: signalling the core the receive of interrupts
  • ISR, core: gathering (int) data by polling the driver
  • foreground, core: gathering data by polling the driver and calculating required information (RPM, feed rate, etc)
  • foreground, core: updating the segments (feed rate)

Correct me if I am wrong:

  • Calculations (for updating segments) should be done at least at every encoder pulse and preferably at every new segment.
  • Calculations at every new segment should not take more than 5 ms.
  • If these calculations take longer than 5 ms, they can only be done at every encoder pulse.

performing floating point calculations in an ISR using a software library is IMO not desirable.

I aggree.

@terjeio
Copy link
Owner Author

terjeio commented Nov 8, 2020

The calculation itself is the same for all drivers and could be part of the core.

It could but I have decided to do this is the driver as the code is small and MCU registers are accessed. Moving it to the core will add a call to get the register data too. I have decided against adding spindle sync as a plugin for the same reason.

ISR, driver: signalling the core the receive of interrupts

I had a callback in an early version but removed it since it went unused. Can be added back, but I believe it is not needed since it is not possible (or neccesary) to adjust the feed rate for a segment once it is executing.

Calculations (for updating segments) should be done at least at every encoder pulse and preferably at every new segment.

Every encoder pulse: this is perhaps ok for low PPR encoder but IMO not for high PPR ones. Every encoder pulse interrupt could be ok if "prescaling" is used by routing the pulse signal to the input of a counter and use compare to trigger the interrupt. This allows a wide range of encoder PPR to be handled with a similar interrupt load. The spindle_encoder.counter.tics_per_irq variable is used for that and it should be calculated from the PPR value (on my todo list). If PID is used the sampling rate should be constant, then at the start of every segment is better.

"Prescaling" will limit pin selection for the main pulse input and may not even be possible to add if a suitable timer is not available. I do not see that as a problem, IMO it is not neccesary to add spindle sync capability to all drivers as the user numbers are going to be low. In fact I prefer to not add spindle sync to all, support beeing the main reason.

Calculations at every new segment should not take more than 5 ms.

I do not get this, no way the calculations are going to take 5 ms.

@HuubBuis
Copy link

HuubBuis commented Nov 8, 2020 •
edited
Loading

Calculations at every new segment should not take more than 5 ms.
I do not get this, no way the calculations are going to take 5 ms.

I haven't measured the calculation time yet but if so, calculations at the start of every new segment could be the way for all processors.

I found this for FPU on ESP32 https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/freertos-smp.html
float cannot be used in interrupt service routines.
A link that claims it can be done: https://github.com/espressif/esp-idf/issues/722.
Double seems not use the FPU, changing float to double could also be a slow way to do this in ISR.

@terjeio
Copy link
Owner Author

terjeio commented Nov 8, 2020

Calculation time should be less than 100 microseconds even without a FPU? And there is always an option of using fixed point math in the driver. Another reason to do the calculations there?

Double seems not use the FPU, changing float to double could also be a slow way to do this in ISR.

Interestingly all of the other 32-bit ports of grbl I have looked at uses the double variants of library calls, IMO for no good reason. A spillover from 8-bit? BTW 8-bit Arduino libraries does not support double precision, they use single precision for both.

ARM M7 processors such as iMXRT1060 has a FPU capable of handling double, at some point the core should get a compile time option for using that instead of float?

@HuubBuis
Copy link

HuubBuis commented Nov 8, 2020

Calculation time should be less than 100 microseconds even without a FPU? And there is always an option of using fixed point math in the driver. Another reason to do the calculations there?

It was not to do the calculations there but the trigger when to do a new calculation.

@Harvie Harvie mentioned this issue Nov 24, 2020
Closed
@drf5n
Copy link

drf5n commented Apr 21, 2021

Why not bypass the encoder steps/sec -> RPM -> feed/rpm calcs and calculate a Bresenham slope directly between spindle encoder counts and feed steps? Then the float calc for the gear ratio/Bresenham slope only need to happen once per G-code, and the encoder can trigger steps or not based on the Bresenham deviation while in spindle sync mode.

@terjeio
Copy link
Owner Author

terjeio commented Apr 21, 2021

Why not bypass the encoder steps/sec -> RPM -> feed/rpm calcs and calculate a Bresenham slope directly between spindle encoder counts and feed steps?

By using a software controlled PLL to generate the step interrupts?

FYI I am only using the measuered RPM to set the initial feed rate, angular position is used in the feedback loop.

@drf5n
Copy link

drf5n commented Apr 21, 2021

Thanks. I was misreading the code. I think I see it around

grblHAL/drivers/MSP432/driver.c

Line 362 in 93c1294

int32_t step_delta = (int32_t)(pidf(&spindle_tracker.pid, spindle_tracker.prev_pos, actual_pos, dt) * spindle_tracker.steps_per_mm);
etc. and where the cycles_per_tick is modified.

@terjeio terjeio closed this as completed Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@drf5n @terjeio @HuubBuis @hitchhiker85