[gnea/grbl-Mega Issue#125] PIC32 port for grbl and hints needed to add servo support also rare stepper ISR issue

hi
i want to get ideas specially from @chamnit if you give some input as you have more knowledge for grbl then others might have but i welcome other people also to give feedback but who have good knowledge on grbl core
currently i have ported grbl to PIC32 i am able to test all basic functionality in my port and this port will be opening door to mostly all pic32 mcu family even pic32mx250f128 which is in dip package for most of diy hobbyist i have studied grbl core and its very nice easy to understand here i have some question (before messing with original grbl core) from experts
in stepper ISR for step set there are room to improve ISR execution time …….. original grbl core was implemented on ATMEGA328 which had limited hardware resources ONLY three timers 1 16-bit and two 8-bit where one was for step reset one for pwm and on for step set but on grbl mega ATMEGA2560 we have 4 16-bit timers and 2 8-bit timers
if we look in ISR which is enabled with “stwakeup() ” function and disabled in the ISR call itself
every time
here
NOTE: This interrupt must be as efficient as possible and complete before the next ISR tick,
which for Grbl must be less than 33.3usec (@30kHz ISR rate). Oscilloscope measured time in
ISR is 5usec typical and 25usec maximum, well below requirement.
NOTE: This ISR expects at least one step to be executed per segment.
what is the cause to go to 25usec is this code

// If there is no step segment, attempt to pop one from the stepper buffer
if (st.exec_segment == NULL) {
// Anything in the buffer? If so, load and initialize next step segment.
if (segmentbufferhead != segmentbuffertail) {
// Initialize new step segment and load number of steps to execute
st.execsegment = &segmentbuffer[segmentbuffertail];

#ifndef ADAPTIVEMULTIAXISSTEPSMOOTHING
With AMASS is disabled, set timer prescaler for segments with slow step frequencies (< 250Hz). TCCR1B = (TCCR1B & ~(0x07<prescaler<segment->cyclesper_tick;
st.stepcount = st.execsegment->n_step; // NOTE: Can sometimes be zero when moving slow.
// If the new segment starts a new planner block, initialize stepper variables and counters.
// NOTE: When the segment data index changes, this indicates a new planner block.
if ( st.execblockindex != st.execsegment->stblock_index ) {
st.execblockindex = st.execsegment->stblock_index;
st.execblock = &stblockbuffer[st.execblock_index];

// Initialize Bresenham line and distance counters
st.counterx = st.countery = st.counterz = (st.execblock->stepeventcount >> 1);
}
st.diroutbits = st.execblock->directionbits ^ dirportinvertmask;
#ifdef ENABLEDUALAXIS
st.diroutbitsdual = st.execblock->directionbitsdual ^ dirportinvertmask_dual;
#endif

#ifdef ADAPTIVEMULTIAXISSTEPSMOOTHING
// With AMASS enabled, adjust Bresenham axis increment counters according to AMASS level.
st.steps[XAXIS] = st.execblock->steps[XAXIS] >> st.execsegment->amass_level;
st.steps[YAXIS] = st.execblock->steps[YAXIS] >> st.execsegment->amass_level;
st.steps[ZAXIS] = st.execblock->steps[ZAXIS] >> st.execsegment->amass_level;
#endif

#ifdef VARIABLE_SPINDLE
// Set real-time spindle output as segment is loaded, just prior to the first step.
//spindlesetspeed(st.execsegment->spindlepwm);
SPINDLEOCRREGISTER = st.execsegment->spindlepwm; // Set PWM output level.
#endif

} else {
// Segment buffer empty. Shutdown.
//stgoidle();
// Disable Stepper Driver Interrupt. Allow Stepper Port Reset Interrupt to finish, if active.
TIMSK1 &= ~(1<idlelocktime != 0xff) || sysrtexecalarm || sys.state == STATESLEEP) && sys.state != STATEHOMING) {
// Force stepper dwell to lock axes for a defined amount of time to ensure the axes come to a complete
// stop and not drift from residual inertial forces at the end of the last movement.
delayms(settings.stepperidlelocktime);
pin_state = true; // Override. Disable steppers.
}
if (bitistrue(settings.flags,BITFLAGINVERTSTENABLE)) { pinstate = !pinstate; } // Apply pin invert.
if (pinstate) { STEPPERSDISABLEPORT |= (1<DISABLE_BIT); }
else { STEPPERSDISABLEPORT &= ~(1<DISABLEBIT); }
#ifdef VARIABLE_SPINDLE
// Ensure pwm is set properly upon completion of rate-controlled motion.
if (st.execblock->ispwmrateadjusted) {
//spindlesetspeed(SPINDLEPWMOFF_VALUE);
SPINDLEOCRREGISTER = SPINDLEPWMOFF_VALUE; // Set PWM output level.
}
#endif
systemsetexecstateflag(EXECCYCLESTOP); // Flag main program for cycle end
return; // Nothing to do but exit.
}

this was ok when ATMEGA328 was used because we have no other timers and has to sacrifice overall performance to synchronize step_segment buffer so feed rate should be as exact as possible

here is the main point
if we adjust in “config.h”
#define ACCELERATIONTICKSPER_SECOND 1000

so in my understanding in this case step_segment is going to be 1/1000 = 0.001 second long and above code is going to be executed at this interval rate which is calculated in
stprepbuffer()
please correct if my understanding is wrong
what if we use another timer @ ACCELERATIONTICKSPERSECOND rate to execute above code for synchronized stepsegment buffer and free up stepper set ISR execution from this code so this way stepper ISR is going to be 5usec and we can go straight to 50khz step rate bench mark
here i have discovered a bug in above code also it is not noticeable in 16-bit timer for stepper_isr but showed in 32-bit timer for stepper set isr
// Initialize step segment timing per step and load number of steps to execute.
OCR1A = st.execsegment->cyclesper_tick;
how this impact during acceleration phase OCR1A value is going to be reduced every time which is the top count valve of TIMER1 and if somehow previous counter value is greater than new OCR1A value it will miss the exact match and counter has to wraparound to max 0xFFFF value as stated in datasheet of atmega328 page no 132
“If the new value written to OCR1A or ICR1 is lower than the
current value of TCNT1, the counter will miss the compare match. The counter will then have to count to its
maximum value (0xFFFF) and wrap around starting at 0x0000 before the compare match can occur”
its random and does not happens continuously vary rare but in 32-bit timer affect is very bad

next thing is what is the optimal way to read stepsegment buffer data to extract information for every axis target position for external position control loop code which i also plan to execute in this third Timer ISR i knew this is not the part of grbl but i need idea for it previously i had build position control servo drives with XMEGA mcu separately but now i want this portion control to integrate in the same controller here idea is very simple get the target position for every axis for every stepsegment and apply position control loop for closed loop position control
idea suggestion are welcome