-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Unify M600 and M125 pause features #6407
Unify M600 and M125 pause features #6407
Conversation
Marlin/Marlin_main.cpp
Outdated
} | ||
} | ||
|
||
static bool pause_print(float retract, float z_lift, float x_pos, float y_pos, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For float
arguments on AVR, const float &
is preferred whenever possible.
Marlin/Marlin_main.cpp
Outdated
#if FILAMENT_CHANGE_LOAD_LENGTH > 0 | ||
+ FILAMENT_CHANGE_LOAD_LENGTH | ||
const float load_length = code_seen('L') ? code_value_axis_units(E_AXIS) : 0 | ||
#if defined(FILAMENT_CHANGE_LOAD_LENGTH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifdef
should be used instead of #if defined(...)
for cases like this.
Marlin/Marlin_main.cpp
Outdated
#endif | ||
stepper.synchronize(); | ||
const int beep_count = code_seen('B') ? code_value_int() : | ||
#if defined(FILAMENT_CHANGE_NUMBER_OF_ALERT_BEEPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifdef
…
This would be a good opportunity also to unify this with |
I could see how it could make use of that feature for moving to the park location. I'll look into the idea. |
@thinkyhead Can you provide any guidance on what you're thinking with the NOZZLE_PARK_FEATURE? |
We can get into it more after the release. |
@thinkyhead I understand if you want to push this off to the next release. However, I think there are some fixes in this pull request that would be good to include in this release. For example, this pull request fixes an issue with the Filament Reload screen that was modified to show the extruder temperature, but didn't actually update. Also, I fixed several issues related to cancelling a SD print, such as not shutting off the heaters or fans. Would you like for me to extract some of the smaller bug fixes and submit them as separate pull requests? |
@tcm0116 I've been super busy looking for an elusive bug. But I think I have that one figured out and fixed. What can I do to help? |
@Roxy-3D not sure. If @thinkyhead wants me to extract the few small bug fixes from this pull request, I can get that taken care of no problem. Maybe it's about time to start having an architecture discussion. I'm thinking that @thinkyhead is wanting to move some of these types of features into the Nozzle class, which makes a lot of sense. However, it might be helpful if we put together a diagram of sorts to help us determine which features and capabilities will go into which classes. With that as an end point, we can start refactoring the code one piece at a time into a fully object-oriented implementation. Furthermore, I'm stewing on an idea to support different tool heads without having to re-load the firmware. If you consider LulzBot's TAZ-6 design with their various tool heads, it could be a compelling feature. In my case, I'm going with the E3D Chimera & Cyclops Legendary Pack, and I just like the idea of not having to re-flash the firmware when I switch between the two. The Nozzle class seems like a good place to handle this, but it's going to make Configuration.h rather interesting. |
All that is fine and good... But if we can do a couple small tweaks and make this suitable to be included in the 'Golden Master' I think we should do it. And we can have an architectural discussion about how to organize things better for v1.1.1 after this Release Candidate gets promoted. |
@thinkyhead Are we at a point where we can resume the discussion about this? With the PR for M701 and M702 (#6493) making yet another version of the filament change code, I think we're starting to see a strong case for unifying all of these very related features in order to cut down on the amount of duplicate code floating around. There were also some heater pausing features added in #6622 for BLTouch that overlap some of the work in this PR related to turning off the heaters during an extended pause. I still don't exactly follow your comment about unifying this with the NOZZLE_PARK_FEATURE, so I'd like to have some discussion about what you're thinking with that. |
@tcm0116 Are there any pieces of code that can be combined to shrink redundancy? That is not an architectural discussion... But it probably will kick of some discussion in that direction. |
@Roxy-3D when M125 was added, the code for M600 was duplicated and massaged a bit. The aim of this PR was to consolidate them back into a single back-end that was shared between the M125 and M600 commands. I believe it wouldn't be much more effort to make use of the same shared functionality for the M701 and M702 filament load and unload commands from #6493. |
I've updated this PR to advance it to the current bugfix-1.1.x branch. As part of that, I've merged the heater pause capability that @bgort added in #6622 with the heater idle timeout capability I had originally added. I'd still like to merge in M701 and M702 from #6493, but I'd like to get some more feedback on what I've done so far before I proceed. I still need someone to test the target temperature flashing on the DOGM display when the heaters and/or bed are paused. That can be done by starting a print from SD and then pausing it via the LCD. After the configured timeout, the target temperatures for the extruders should start flashing and the actual temperature will start dropping. Testing the bed temperature flashing should be able to be accomplished by enabling the pause while probing feature and then it should flash during each probe event. |
Looking good.... |
With this Merge you have overwritten two entries in many language files with the english one!! #define MSG_FILAMENT_CHANGE_HEADER _UxGT("PRINT PAUSED") |
@floyd871 that was by design. The original text was specific to the M600 filament change, but don't make sense to the M125 pause since it now uses some of those screens when the print is resumed. It's my understanding that there are people that periodically update the translations as necessary. If you want to provide a translation for your language, I'm sure we could get it incorporated. |
I see, that makes sense, but then it would be better if you have renamed the label as well. |
I thought the same thing when you posted them. I tried to change
everything, but I guess I missed those.
|
Well... Now that the bulk of the changes are merged.... It is easy to followup with a couple of small patches. Somebody is helping! #6871 |
@tcm0116 I've been using the Advanced Pause and new Filament Change for 2 days now. I have yet to get the print I wanted done. (It is a very difficult print) But the new Pause code seems to do exactly what it is supposed to do. I have paused it several times and changed the filament several times. Unfortunately... I thought the last print was going to succeed, but I paused it for a couple of hours that I had to be away from the printer. When I cane back, the ABS plastic I was using broke away from the glass and lost adhesion. Soooo... Restarting it with PLA plastic on my FrankenStein printer.... |
@Roxy-3D That's great to hear! Did you happen to remember to check for the
flashing target temperature on the filament change LCD screens?
|
No! I forgot... But I'll try to remember to do that next time I pause something... |
As it turns out... We still have a little bit of work to do! :) When giving the printer a M600... The second number doesn't blink. |
@Roxy-3D does the first number update as the temperature is dropping without you having to do anything like move the encoder? |
Yes! It is slow on my printer... but after 30 or 40 seconds, it is clear that it is dropping.... |
@Roxy-3D I put together a change at tcm0116@f3528b9 if you have a chance to give it a try. |
I can grab your pause_flash branch, right? But... should we edit this? We have nested #if ENABLED(ADVANCED_PAUSE_FEATURE) #if ENABLED(ADVANCED_PAUSE_FEATURE)
static void lcd_implementation_hotend_status(const uint8_t row) {
row_y1 = row * row_height + 1;
row_y2 = row_y1 + row_height - 1;
if (!PAGE_CONTAINS(row_y1 + 1, row_y2 + 2)) return;
u8g.setPrintPos(LCD_PIXEL_WIDTH - 11 * (DOG_CHAR_WIDTH), row_y2);
lcd_print('E');
lcd_print((char)('1' + active_extruder));
lcd_print(' ');
lcd_print(itostr3(thermalManager.degHotend(active_extruder)));
lcd_print('/');
#if ENABLED(ADVANCED_PAUSE_FEATURE)
if (lcd_blink() || !thermalManager.is_heater_idle(active_extruder))
#endif
lcd_print(itostr3(thermalManager.degTargetHotend(active_extruder)));
}
#endif // ADVANCED_PAUSE_FEATURE |
@Roxy-3D Oops. Updated in the branch https://github.com/tcm0116/Marlin/tree/pause_flash |
Grabbing it now... |
It works on a Graphical Display!!! I don't have a way to test a 20x4 right now. |
I can test it on my printer.
|
* Unify M600 and M125 pause features * Cleanup per thinkyhead's comments * Rename filament_change_menu_response to advanced_pause_menu_response * Include HAS_BED_PROBE in QUIET_PROBING * Update gMax example file * is_idle() is out of scope without the braces * Convert FT-i3-2020 to Advance Pause names... * Allow pause even if not printing
Seems there is a problem. The filament change feature was broken by this PR, apparently. If the nozzle is cooling down when you start Let's restore that code to // Show "wait for heating"
lcd_filament_change_show_message(FILAMENT_CHANGE_MESSAGE_WAIT_FOR_NOZZLES_TO_HEAT);
wait_for_heatup = true;
while (wait_for_heatup) {
idle();
wait_for_heatup = false;
HOTEND_LOOP() {
if (abs(thermalManager.degHotend(e) - temps[e]) > 3) {
wait_for_heatup = true;
break;
}
}
} |
Moreover, the original code I believe took into account that you can break out of a wait for temperature loop prematurely via I would advise combing the previous |
Maybe we should revert this? And then move forward with that in mind? The main purpose of the commit was to collapse duplicate code. |
I think at this point it will be less work to plough forward. |
I don't believe this is a true statement. As can be seen in the snippet below from
I'm 100% certain that the old M600 code would do the exact same thing.
The
The solution to the first issue is to ensure that the target temperature for the active extruder is set to a reasonable value when starting M600 in addition to verifying that the nozzle is actually at a reasonable value. To correct the second issue, we just need to insert |
* Unify M600 and M125 pause features * Cleanup per thinkyhead's comments * Rename filament_change_menu_response to advanced_pause_menu_response * Include HAS_BED_PROBE in QUIET_PROBING * Update gMax example file * is_idle() is out of scope without the braces * Convert FT-i3-2020 to Advance Pause names... * Allow pause even if not printing
* Unify M600 and M125 pause features * Cleanup per thinkyhead's comments * Rename filament_change_menu_response to advanced_pause_menu_response * Include HAS_BED_PROBE in QUIET_PROBING * Update gMax example file * is_idle() is out of scope without the braces * Convert FT-i3-2020 to Advance Pause names... * Allow pause even if not printing
@Roxy-3D requested the ability to prime the nozzle when resuming a print. While looking into this request, it occurred to me that the recently added M125 code was was almost copy-and-pased from M600. As such, I decided it would be helpful to merge them together into a single implementation in order to have the flexibility to provide the benefits of both without a bunch of code duplication.
I tried to give this a fair amount of testing, but I only have one printer with a 20x4 LCD. As such, it would be helpful if this could get some more testing on different configurations, especially the target temperature idle flashing on the DOGM display when the heaters have become idle after pausing.