MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.23k stars 19.22k forks source link

Time indicator on LCD does not work with RC4 #3275

Closed marvin42blue closed 8 years ago

marvin42blue commented 8 years ago

I just noticed that, while the printer runs, the time indicator (hh:mm) keeps displaying "--:--" on my LCD display, everything else seems to work normal, as with RC3.

I'm using the REPRAP_DISCOUNT_SMART_CONTROLLER.

marvin42blue commented 8 years ago

I'm confused by the code in print_job_start() : if this is called without args () then print_job_start_ms is loaded with the value 0 because the function prototype is

bool print_job_start(millis_t t = 0); 

I see no function call to print_job_start with args - and I also found no place that modifies the variable print_job_start_ms, other than in print_job_start().

Since the LCD code only counts time when

     if (print_job_start_ms != 0) {

But I cant find any code that changes the value away from 0 - this cannot work, can it? Do I miss something, or how is this supposed to work?

thinkyhead commented 8 years ago

@marvin42blue Are you using the latest RCBugFix code? That appears to have already been fixed.

print_job_start_ms = (t) ? t : millis();
bool print_job_start(millis_t t /* = 0 */) {
  for (int i = 0; i < EXTRUDERS; i++) if (degTargetHotend(i) > 0) return false;
  print_job_start_ms = (t) ? t : millis();
  print_job_stop_ms = 0;
  return true;
}
jbrazio commented 8 years ago

Could you please post your GCode ?

marvin42blue commented 8 years ago

my gcode is like this

M107
M190 S50 ; set bed temperature
M104 S215 ; set temperature
G28 ; home all axes
G1 Z15 F5000 ; lift nozzle
G1 X-50 ; outside start pos
M109 S215 ; wait for temperature to be reached ---> should print_job_start() here
G21 ; set units to millimeters
G90 ; use absolute coordinates
M82 ; use absolute distances for extrusion
G92 E0

I debugged the code in print_job_start() and of course, degTargetHotend(i) is always > 0 (because I usually pre-heat the bed and extruder, but also when a previous print left the printer hot), so it returns without setting print_job_start_ms to any value.

This code seems to say : do not start a print job, when the extruder is (already) hot - I think this makes no sense at all, even more, since the return code is never used, would you agree ?

jbrazio commented 8 years ago

It's too early in the morning for my brain to be functioning correctly but your analysis seems correct.. Ain't also a print_job_start() hook on M104 ?

-edit- The issue is with M104.

jbrazio commented 8 years ago

@marvin42blue do you mind testing if the proposed PR fixes your issue ?

marvin42blue commented 8 years ago

@jbrazio yes, I can test this later today, after work, I'm in germany

fbarcenas commented 8 years ago

I also have the problem, i am using VIKI2. I am using the actual (in this hour) RCBUGFIX What some info? or than i test something?

jbrazio commented 8 years ago

Do you know how to apply a patch using git ? My PR haven't been merged try so you have to apply it manually on your local source.

fbarcenas commented 8 years ago

Actually i have this:

// Print job timer related functions
millis_t print_job_timer();
bool print_job_start(millis_t t = 0);
bool print_job_stop(bool force = false);

Do you want than i change for?

jbrazio commented 8 years ago

Maybe it's easier to download a zip from my repo: https://github.com/jbrazio/Marlin/archive/bugfix/m104-timer-wont-start.zip

Download, unzip, copy your config files, burn and test.

thinkyhead commented 8 years ago

3282 has been merged. So give RCBugFix a test when you have a chance.

marvin42blue commented 8 years ago

I changed my code like this (removed the "return false") because I don't see any reason why I want to abort when any extruder is already hot - this works

bool print_job_start(millis_t t /* = 0 */) { print_job_start_ms = (t) ? t : millis(); print_job_stop_ms = 0; return true; }

@jbrazio : I loaded your zip, but this has still the "...return false" and I see no timecode on my display

jbrazio commented 8 years ago

Please do not manually change the code.. we want to validate the Pull Requests/patches.. @thinkyhead merged the patch into the latest RCBugfix, could you please give it a try ? Thanks.

marvin42blue commented 8 years ago

@jbrazio: don't get me wrong, I mean I changed my own code just to test the behaviour and. well, yes I just checked the RCBugFix (29 min old) and this code still contains the line

7698 : for (int i = 0; i < EXTRUDERS; i++) if (degTargetHotend(i) > 0) return false;

jbrazio commented 8 years ago

Yes.. That line wasn't changed. The patch applies a change to M104 which was only stopping the timer and not straying it as it should. Please have a look in the referenced PR diff to more information on what exactly was changed.

marvin42blue commented 8 years ago

..ok, I have seen the M104 change, but my point is: why not change this line ? As long as no code checks the return value and nobody can explain why a start is not valid when extruders are already hot, I see no sense - and deleting the line makes my timer work nicely...

Blue-Marlin commented 8 years ago

If there is already a hot extruder the timer should already run. You don't want to reset the timer when heating a second extruder.

jbrazio commented 8 years ago

And on top of what @Blue-Marlin has said, we do not have an "exact" way to detect that a print job has started (from serial) so we apply the following logic:

During the print job you may set as many different temperatures you want for the same or any other extruder and this will never reset the start time due to that line.

The reverse logic is applied to detect a finished print job.

marvin42blue commented 8 years ago

@blue-marlin: ok I see - but my use case is this: I usually switch on power to my printer and PC and start pronterface to connect via USB, then pre-heat the extruder and bed, because I know it will take a minute to heat up. When I start printing a few minutes later the extruder is hot and because of this, the timer is never started. For me, the timer is associated with "start printing" - what is your opinion on this ? In case we have conflicting ideas about the timer, I would vote for a way to configure it via '#define'

jbrazio commented 8 years ago

Could you please confirm that the latest RCBugFix is now working on the test condition provided on this issue first ? We will address any other issues after this confirmation.

M107
M190 S50 ; set bed temperature
M104 S215 ; set temperature
G28 ; home all axes
G1 Z15 F5000 ; lift nozzle
G1 X-50 ; outside start pos
M109 S215 ; wait for temperature to be reached ---> should print_job_start() here
G21 ; set units to millimeters
G90 ; use absolute coordinates
M82 ; use absolute distances for extrusion
G92 E0
marvin42blue commented 8 years ago

confirmed I just tested it successfully

jbrazio commented 8 years ago

Great, thanks !

I usually switch on power to my printer and PC and start pronterface to connect via USB, then pre-heat the extruder and bed, because I know it will take a minute to heat up. When I start printing a few minutes later the extruder is hot and because of this, the timer is never started.

I would suggest to keep the same behaviour but change your start GCODE to the following one:

M107
M190 S50 ; set bed temperature
M104 S0 ; temp reset so the timer can start
M104 S215 ; set temperature -- timer starts
G28 ; home all axes
G1 Z15 F5000 ; lift nozzle
G1 X-50 ; outside start pos
M109 S215 ; wait for temperature to be reached
G21 ; set units to millimeters
G90 ; use absolute coordinates
M82 ; use absolute distances for extrusion
G92 E0
thinkyhead commented 8 years ago

Great! Looks like all the wait-for-heating issues are pretty well fixed now.

@marvin42blue I suggest when preheating before printing, only preheat to 2 degrees below your print temperature, instead of heating all the way. Perhaps in future we will have a better way to distinguish a print job from immediate commands. There are still some other cues we could look for....

marvin42blue commented 8 years ago

Hello Joao,

since this issue #3275 is closed now, maybe you can help me how to continue with my issue. I havent used GitHub before, so I not aware of how to handle things properly...

I understand why you suggest to change my gcode, but my toolchain is CAD > Slic3r > Pronterface and Slic3R wont allow me to modify its gcode in the desired way.

I also agree, that with thousands of gcode flavours, its not clear for the Marlin Firmware to determine the "print start"

On the other hand, I'm not happy that this behaviour changed between RC3 and RC4.

In addition, I think that a function print_job_start() returning a value of "false" shall be checked by the caller - and this does not happen in the Marlin code.

I modified my own code now this way:

bool print_job_start(millis_t t /* = 0 */) {

if ENABLED (PRINT_START_ALWAYS)

// MDf:160329: just go on and start...

else

for (int i = 0; i < EXTRUDERS; i++) if (degTargetHotend(i) > 0) return false;

endif

print_job_start_ms = (t) ? t : millis();

SERIAL_ECHO_START; SERIAL_ECHOPAIR("*\ JobStart ", print_job_start_ms); SERIAL_ECHOLNPGM("\n");

print_job_stop_ms = 0; return true; }

...do you think it might be feasible to suggest a solution like this, and have an additional #define PRINT_START_ALWAYS in the configuration.h file - or what would be an appropriate way to deal with this ?

thanks for your help best regards Matthias (marvin42blue)

----- Original Message ----- From: João Brázio To: MarlinFirmware/Marlin Cc: marvin42blue Sent: Wednesday, March 30, 2016 10:54 PM Subject: Re: [MarlinFirmware/Marlin] Time indicator on LCD does not work with RC4 (#3275)

Great, thanks !

I usually switch on power to my printer and PC and start pronterface to connect via USB, then pre-heat the extruder and bed, because I know it will take a minute to heat up. When I start printing a few minutes later the extruder is hot and because of this, the timer is never started.

I would suggest to keep the same behaviour but change your start GCODE to the following one:

M104 S0 ; temp reset so the timer can start M107 M190 S50 ; set bed temperature M104 S215 ; set temperature -- timer starts G28 ; home all axes G1 Z15 F5000 ; lift nozzle G1 X-50 ; outside start pos M109 S215 ; wait for temperature to be reached G21 ; set units to millimeters G90 ; use absolute coordinates M82 ; use absolute distances for extrusion G92 E0 — You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

jbrazio commented 8 years ago

My only concern is this is a very "specific option" that most people looking at it will not understand the meaning nor the purpose. I take the opportunity to open the discussion @Blue-Marlin @Roxy-3DPrintBoard @thinkyhead @AnHardt: What's you feeling to start the print timer by hooking into one of the following G-Codes:

Vs. the option to add an additional parameter to M104 and M109 to force the timer to start independent of the nozzle temperature ?

marvin42blue commented 8 years ago

I checked all my gcode-files (approx 100), and as far as I'm concerned both G21 and G90 always appear only once, right after M109 - so this would work for me.

thinkyhead commented 8 years ago

@jbrazio Nope, I don't think any hooks for "print time" should be added to any other commands. Commands should do what they are specified for with no extra effects in normal usage.

A better long-term solution might be to add GCodes specifically for "Starting print job", "Pausing print job", and "Stopping print job" with an option to fall back to watching M109 if the "Starting print job" command isn't present before M109. (These "print job" commands to Marlin would just be cues, not in any way telling Marlin itself to start or pause or stop a job. The timer would be one effect. Maybe there might be others.)

Since we only need to do special heuristics for non-SD printing, the tricks we're using really only need to apply to GCode sent from a host. And in that case, we may be able to discern that a print job is in progress by using other tricks. For example, do hosts ever send N line numbers greater than 10 when sending immediate commands from a console? Or do hosts only ever send N and * when running GCode from a file? Perhaps we only need to look for the line number increasing to determine that a print job is in progress…?

jbrazio commented 8 years ago

@thinkyhead I was trying to avoid the option to create specific commands to start/stop print timer because it would be kinda difficult to make them main stream don't you believe ? The heuristics option seems more adequate as a permanent solution for this issue.

Other pattern I can recall could be planner buffer usage percentage.

fbarcenas commented 8 years ago

I continue with this problem... And i have the actual (this minute) RC BUGFIX installed. The time not work... Want info?

jbrazio commented 8 years ago

Could you post your gcode header ?

marvin42blue commented 8 years ago

I think some custom start/stop/pause codes would be great, since that would be a clean solution - and maybe this will find its way into main stream sooner or later.

On the other hand, a heuristic solution seems also to be ok - would you guys think it makes sense to start a vote for people sending us G-code snipplets, so we could evaluate different g-code flavours ?

fbarcenas commented 8 years ago

I attach:

G90 M82 M106 S0 M140 S60 M104 S215 T0 M109 S215 T0 M104 S0 T1 G21 ;metric values M218 T1 X28.5 Y0.0 G1 Z10 G28 ; HOME XY - Z G92 E0 START GCODE LINES OF FIRST LAYER....

I start another file now and nothing.... the time not work and also happen a thing: I must start 3-4 times the file for this start, fail show heating.....

fbarcenas commented 8 years ago

In this moment i run a new file, (with same code start) now work the timer and start at first time. Incredible?

jbrazio commented 8 years ago

This is not a solution for your errors with the print timer but the following G-Code lines are redudant:

M104 S215 T0
M109 S215 T0

You should either remove 104 or M109 depending if you want or do not want to wait for the nozzle to get hot. Also if you do not want to heat the second extruder you don't have to be specific it by using M104 S0 T1, by default it is off.

This could explain the heating error you're having.

@marvin42blue OK I will work it out and submit as experimental PR.

fbarcenas commented 8 years ago

Very interesting, but this is a thing automatically in the software simplify, i attach photo:

captura de pantalla 2016-04-02 a las 21 33 47

Some ideas?

thinkyhead commented 8 years ago

it would be kinda difficult to make them main stream don't you believe?

Yes it would. So we should not require them. But if they exist, some may use them. If they don't exist, no one ever will.

thinkyhead commented 8 years ago

@jbrazio I'm not entirely confident in the logic of only starting the print job timer if no temperature has been set. Instead it would be better to —yes— start the timer if setting a temperature for the first time, but also set it if setting a temperature —even if not the first time— when the print job timer hasn't already been set. If need be, only set it if the temperature being set is higher or the same as the current target…. We can play with the logic there….

I was also thinking the print job timer could be set when you get the first G1 command (containing N and * and thus from a host) after starting the heaters. It might retroactively track the time since it started heating and tack that on.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.