Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to configure or disable PRINTCOUNTER save interval #20856

Conversation

ConstantijnCrijnen
Copy link
Contributor

Description

PRINTCOUNTER's saveStats() will cause SKR1.3 boards and possibly similar boards to interrupt all movements for around half a second, which causes the printer to lose steps whenever that function is called. Specifically once per hour according to saveInterval = 3600 and once the printcounter timer is stopped. @sjasonsmith suspects that this might have something to do with how EEPROM is handled with these boards.

Changes

The PrintCounter::saveStats() function is called with PrintCounter::tick(), PrintCounter::stop(), PrintCounter::init() and PrintCounter::resetServiceInterval().

tick()
tick() calls saveStats() every hour and thus causes step loss basically every hour. My fix is to stop tick() from saving to EEPROM entirely for all affected boards. For now, I just disabled it for LPC1768 boards, assuming that at least all of those are affected.

stop()
stop() calls saveStats() whenever the stopwatch has been running. There are a couple of issues here:

  1. If a print is aborted within the first hour, no stats will be saved to EEPROM at all, as saveStats() is only called once one hour has elapsed. My fix is to always call saveStats() during stop(), even when the stopwatch is not running.
  2. When print_job_timer.stop() is called by MarlinUI::abort_print(), it is again called by MarlinCore's abortSDPrinting(), causing a jarring step-loss twice in a row. My fix is to remove it from MarlinUI::abort_print() and to wait for the (already cleared) buffer to really clear before calling print_job_timer.stop() during abortSDPrinting().
  3. I also stopped the stopwatch at the start of abortSDPrinting(). Otherwise the aborted print would have been counted as a successful print, as the stopwatch was still running.

init()
I don't see any issues with init().

resetServiceInterval()
I haven't used any service intervals so I didn't test it and left it alone.

Other Changes

I couldn't find any way to group all boards by their processor, so I created one in boards.h. The _DO_ macros had to be extended by one for that.

Benefits

No more step loss, while being able to use the printcounter. The only disadvantage is, that affected boards won't save their progress to eeprom until the print is finished or aborted.

Configurations

Choose any LPC1768 board and enable PRINTCOUNTER.
LPC1768 boards: RAMPS_14_RE_ARM_EFB, RAMPS_14_RE_ARM_EEB, RAMPS_14_RE_ARM_EFF, RAMPS_14_RE_ARM_EEF, RAMPS_14_RE_ARM_SF, MKS_SBASE, MKS_SGEN_L, AZSMZ_MINI, BIQU_BQ111_A4, SELENA_COMPACT, BIQU_B300_V1_0, GMARSH_X6_REV1, BTT_SKR_V1_1, BTT_SKR_V1_3, BTT_SKR_V1_4

Related Issues

Issue #20785

To Do

What other boards are affected by this issue?

I'm sorry for any stupid mistakes. I've never done this before and I only have some Java experience from uni 😊

@thisiskeithb
Copy link
Member

thisiskeithb commented Jan 23, 2021

Was this an issue on multiple LPC1768 boards? I ran an SKR 1.3 for a year (and a some LPC1769-based boards) and they don’t suffer from this step loss issue.

@ConstantijnCrijnen
Copy link
Contributor Author

Was this an issue on multiple LPC1768 boards? I ran an SLR 1.3 for a year (and a some LPC1769-based boards) and they don’t suffer from this step loss issue.

I have tested it with three SKR 1.3 boards, two of which were bought around 10 months after the first one. All of them had the same issue. But the step loss only seldomly starts to occur at speeds over 40mm/s with 1.8deg steppers, 16x microstepping and 20t gt2 pulleys. Speeds over 150mm/s almost always (every 2 hours max.) cause a loss of steps. I haven't tested it on any other boards, so it's just an assumption that this is an issue with other LPC1768.

I did a lot of testing in #20785 which shows what the step loss looks like and this video shows what happens when I abort over LCD. There might be an underlying issue for which I unfortunately lack the knowledge to understand, but moving saveStats() around like I did fixed it for me on all three boards.

You can try to replicate the issue by setting saveInterval = 3600 in the header file to a couple of seconds. I tried 60 seconds and the step loss occured basically like a clockwork once per minute at a speed of 190mm/s.

@thisiskeithb
Copy link
Member

step loss occured basically like a clockwork once per minute at a speed of 190mm/s.

I don't print anywhere near that speed, so that's likely why I never saw this issue.

If it's planned to move to only save at the end of a print/abort, then I'd like to make the suggestion (to Thinkyhead) that we remove sanity checks like these so PRINTCOUNTER can be used on all boards with emulated EEPROM:

#if defined(STM32F4xx) && BOTH(PRINTCOUNTER, FLASH_EEPROM_EMULATION)
#warning "FLASH_EEPROM_EMULATION may cause long delays when writing and should not be used while printing."
#error "Disable PRINTCOUNTER or choose another EEPROM emulation."
#endif

@thinkyhead thinkyhead force-pushed the Printcounter-Fix-(issue-#20785) branch from 6b95ed5 to a0a94d3 Compare January 25, 2021 08:51
@thinkyhead thinkyhead changed the title Printcounter Fix (issue #20785) Option to configure or disable PRINTCOUNTER save interval Jan 25, 2021
thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Jan 25, 2021
thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Jan 25, 2021
@thinkyhead thinkyhead force-pushed the Printcounter-Fix-(issue-#20785) branch from d752f4a to 71a4294 Compare January 25, 2021 09:53
@thinkyhead
Copy link
Member

The loss of steps issue is most likely just due to the EEPROM write happening at a time when the stepper ISR is running a move, and possibly interrupts are being disabled or superseded somewhere in the EEPROM writing process. If it's not a big problem to have a small pause once per hour, then a planner.synchronize() before writing to EEPROM should be enough to prevent the shift.

@ConstantijnCrijnen
Copy link
Contributor Author

I'll give it a try later tonight (Europe) and report back. Thanks a lot for your help!

@ConstantijnCrijnen
Copy link
Contributor Author

Thanks for your help! I tested your edits and it works just as expected. The short delay after aborting the print is nearly imperceivable and the printcounter seems to properly count aborted prints.
As you said, there is a tiny stutter when saving to EEPROM. With a PRINTCOUNTER_SAVE_INTERVAL of 1, I managed to get one of those stutters on the outside perimeter of a print:

IMG_0216

I think this is a good compromise.

@ConstantijnCrijnen ConstantijnCrijnen deleted the Printcounter-Fix-(issue-#20785) branch January 29, 2021 01:09
Jyers pushed a commit to Jyers/Marlin that referenced this pull request Feb 3, 2021
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants