Request for Marlin/Arduino Expertise

[jsc]

The compiler flag suggestion was a long shot idea, and wouldn't fix the race condition anyway.

For a quick test, I suggest putting something like

Code: Select all

if (TCNT1 >= OCR1A) TCNT1 = 0;

at the end of the ISR and just rerun your tests. You should still see gaps in the timing, but they will be on the order of a single step instead of 32 ms. If that works, then 0 could be increased to "OCR1A - x" where x is enough clock ticks such that there is enough time for the ISR to return and have interrupts be enabled again, which would reduce any gaps to a minimum. Alternatively, you could put in that test and reset of TCNT1 at the end of a do-while loop to keep servicing steps until it stopped overrunning the timer, but I think it's generally considered bad form to tie things up in an interrupt handler like that.

Of course, this whole situation is pretty marginal, and if it turns out this is what is causing the problem, it's just because you're running at the ragged edge of what is feasible on the hardware.

[innkeeper]

Let us know the results of the SD only test. I've experienced other odd behavior running USB on marlin but was not repeatable.

[Josh]

jsc: The compiler flag was an easy test, so I ran it first. I did add your line of code ("if (TCNT1 >= OCR1A) TCNT1 = 0;") to the end of the ISR (line 761 in my testing version of Marlin) and it seems to have worked - I'm looking for gaps over 4mS, up to 60mS (can't go much lower as that's the gap time between deceleration/deceleration glitch steps, which I'm not looking for right now...). It's been running for ~45 minutes, long move at F90000, compiled in both 1.5.5 and 1.5.7, and verified that removing that line with no other changes shows the glitching behavior again. I'm going to keep testing this for a few hours, and then implement and start printing with it, but initial/preliminary results are very positive.

What exact changes are you recommending for this solution, if it tests well? I rather obviously trust your judgement on this...

innkeeper: I posted about the SD test before, no change when powered from USB or from the PSU. You may have been seeing a buffer underrun on print moves - especially on small curves with lots of facets, the communication speed between your computer and the RAMBo may be such that the time to transmit a movement command, is longer than it actually takes to perform the move; if that happens often enough in a short period of time, the move buffer will diminish, and Marlin will slow down to try to avoid stopping due to lack of moves. Heavy activity on your computer can contribute to this as well.

[jsc]

As I outlined above, I see three options. In more detail:

1. Just leave the testing line in there and call it done. You will occasionally have a delayed step, but it will just be one step period instead of big gaps, and they only happen during rapid travel moves. This is the only way to completely beat this race condition.
2. Reset TCNT1 to OCR1A minus some experimentally determined value, in order to reduce the delay. The value should be high enough that there is enough time for the ISR to complete, then get triggered again from the top as TCNT1 gets up to OCR1A. This has the advantage of allowing other interrupts to fire, but
3. Since motion control is the #1 duty of the firmware anyway, you can minimize any step delay by ensuring that steps are serviced as early as possible: wrap the entire body of the ISR in

   Code: Select all

   while (true) {
      ... body of ISR
   
     if (TCNT1 < OCR1A) {
       break;
     }
   
     // Already missed next scheduled interrupt; reset counter and repeat immediately.
     TCNT1 = 0;
   }
   

This doesn't entirely fix the race condition, as there is a possibility that TCNT1 could be slightly less than OCR1A, enough to make it past the test but close enough to miss the next interrupt, but that should be pretty rare.

[Josh]

jsc: testing on your modification continues. https://github.com/MarlinFirmware/MarlinDev/issues/35 has some updates, but the short version is that, so far, it works X) I've tested both TCNT1 = 0 and TCNT1 = (0.75*OCR1A) and both work well. That post has some information on the timing.

For Issue 2, which I haven't been focusing on quite as much, I'm not sure your first point is valid - it looks like

Code: Select all

   if(step_rate > acc_step_rate) { // Check step_rate stays positive
        step_rate = current_block->final_rate;
      }
      else {
        step_rate = acc_step_rate - step_rate; // Decelerate from aceleration end point.
      }

is checking to make sure an underflow doesn't happen, before changing the step_rate. I still don't have any code that causes the decel curve issue reliably, so that's my first step in further attacking this issue.

[jsc]

Sweet, I'm glad to have helped. I would avoid floating point operations in an interrupt, though, as they're surprisingly expensive.

Re: issue 2, that was another stab in the dark and I was skimming so I didn't really take in and appreciate the first part of the test. Maybe final_rate is being set inappropriately? As you say, I think the first step will be to get a repeatable case, then start putting some debugging instrumentation in.

[Josh]

Good call on the floating point operation - it adds ~10µS to the execution time of the ISR. I think I've settled on 175 as the reset value - in a newer version of Marlin, it keeps the extra gap between step sets to below 20µS.

[Kulturfolger]

Is this bugfix included in the current marlin firmware download on this site? http://makergear.wikidot.com/m2-firmware

Ingo

[Levi8than]

Is this bugfix included in the current marlin firmware download on this site? http://makergear.wikidot.com/m2-firmware

Ingo

No, I don't see the fix in there.
I have a model which was reproducing this bug, so I read up on it and added the fix from the linked marlin dev forum thread.

Just open up Marlin/stepper.cpp, look for the line that reads "// If current block is finished, reset pointer" and add this line just before it:

Code: Select all

OCR1A = (OCR1A < (TCNT1 +16)) ? (TCNT1 + 16) : OCR1A;

Sourced from these pages:
//https://github.com/MarlinFirmware/MarlinDev/issues/35
//http://forums.reprap.org/read.php?397,5 ... msg-549268

And I've confirmed it works.
My previous print was shifting in the X direction every 2 to 3 layers. Currently it's past 100 layers with no shifting and looks good.
I haven't yet researched what the side effect of taking the hack is, but if I understand it as well as I think I do (I don't) then it should be harmless and just slow down the print a little sometimes..