search

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.15k stars 19.21k forks source link

[BUG] M106 not working as expected on printer with SINGLENOZZLE #24065

Open Graylag-PD opened 2 years ago

Graylag-PD commented 2 years ago

Did you test the latest bugfix-2.0.x code?

Yes, and the problem still exists.

Bug Description

Ok, so this is not really a bug, but more like feature that was not thought out very well. Here is the story: If you have printer with several M106 controlable fans (NUM_M106FANS >1) and SINGLENOZZLE, then M106 produces weird results, that although you can see all your fans in menu and you can edit them all, it will just push it to fan0. Meaning it is utterly pointless to even have those additional fans. When digging through code, the culprit is this line in temperature.cpp: TERN(SINGLENOZZLE, if (fan < EXTRUDERS) fan = 0); // Always fan 0 for SINGLENOZZLE E fan To fix this issue, all you need to do is comment this line.

No matter how hard I think about this, I cannot figure out what was the idea behind making this line. I guess the idea was, that if I have just one nozzle, then I will always want to cool printed part with fan close to that nozzle and not by some other fan? But what would even be point in having those additional fans? If user has SINGLENOZZLE and several controlable fans, then he obviously wants to have those fans controlable and not permanently off. Perhaps the reason is to allow printability of files where slicer automatically assigns fan based on used extruder? But that seems as pretty weak excuse to limit legitimate use of additional fans. Especially considering that for SINGLENOZZLE you usually need specially configured slicer (and gcode - nozzle purging etc) anyway.

My proposal is to simply delete this line, but I do not want to simply make PR just in case I am missing something.

Bug Timeline

No response

Expected behavior

No response

Actual behavior

No response

Steps to Reproduce

No response

Version of Marlin Firmware

2.0.9.3

Printer model

No response

Electronics

No response

Add-ons

No response

Bed Leveling

No response

Your Slicer

No response

Host Software

No response

Additional information & file uploads

No response

thisiskeithb commented 2 years ago

No matter how hard I think about this, I cannot figure out what was the idea behind making this line.

Marlin will enable all fans as defined in a pins file, so NUM_M106_FANS was created to disable unused fans on printers that didn't have fans attached to every controllable fan port.

InsanityAutomation commented 2 years ago

That line predates NUM_M106_FANS, and when it was added was preventing boards that had a pin for T1 but no physical fan connected to it, as there was only a single hotend, from having no part cooling. Nowadays it should probably be

TERN_(SINGLENOZZLE, if (fan >= NUM_M106_FANS) fan = 0); // if setting fan above defined number, always fall back to 0 for single nozzle

I'm on mobile, so I'm assuming fan is the 0 based index for the fan. If the setting to fan is properly sanity checked earlier, even that may not be needed. NUM_M106_FANS should default to 1 for single nozzle if not explicitly overriden though.

Graylag-PD commented 2 years ago

No matter how hard I think about this, I cannot figure out what was the idea behind making this line.

Marlin will enable all fans as defined in a pins file, so NUM_M106_FANS was created to disable unused fans on printers that didn't have fans attached to every controllable fan port.

That is not the point, NUM_M106_FANS does what it should. It will correctly make as many fans as I want (and not more). But the M106 does not work afterwards. Let me describe my printer so it makes sense. I have Octopus Pro, so up to 6 fans. Single nozzle/2 extruder combination. I want to have three M106 fans, so NUM_M106_FANS 3. FAN0 is part cooling, FAN1 is chamber venting, FAN2 is internal air cycling through activated carbon filter. I can see all three fans on LCD and I can set any value to them (just as I can send M106 P1/2), but no matter what, it will set that value to FAN0 while keeping FAN1 and FAN2 at 0%. And that is because of code in temperature.cpp. It may make sense assuming all M106 fans are for part cooling. But it does not if they are used for something else.

Graylag-PD commented 2 years ago

That line predates NUM_M106FANS, and when it was added was preventing boards that had a pin for T1 but no physical fan connected to it, as there was only a single hotend, from having no part cooling. Nowadays it should probably be TERN(SINGLENOZZLE, if (fan >= NUM_M106_FANS) fan = 0); // if setting fan above defined number, always fall back to 0 for single nozzle

This kind of make sense, but at the same time it does not. There is no direct link between M106 and currently used tool and it is users responsibility that correct fan is used. If user tries to run fan which is not physically connected and as a result does not have any part cooling, then it is users fault and as such should not be "auto-corrected" by Marlin. Or better yet, should this be auto-corrected, then it should be corrected at M106 code, i.e. instead of if (pfan >= _CNT_P) return; it should be something like if (pfan >= NUM_M106_FANS) return;

Graylag-PD commented 2 years ago

And I noticed another part of this issue. Although commenting that one line in temperature.cpp makes it possible to control additional fans from LCD, it is not possible from M106. That is because M106_M107.cpp checks against number of extruders, not NUM_M106_FANS. This means like I suggested in previous comment, change needs to be done to both temperature.cpp and M106_M107.cpp

InsanityAutomation commented 2 years ago

Yes there is a link, as with no arguments M106 defaults to active tool fan. There is a totally seperate feature to assign a fan for chamber cooling. And M42 for arbitrary pin control for a pin not assigned to a specific feature. It sounds like you're trying to make part cooling fans do everything else, which is very far from the intended function.

Graylag-PD commented 2 years ago

I see, did not know that. It does not seem to be documented feature. Though my argumentation about users responsibility is still valid. There is no reason to call M106 without P parameter and if user decides to do that, than he bears the responsibility for his actions. But I think this boils down to GCode philosophy. You (and obviously Marlin code) consider M106 as strictly part cooling fan thing, while I consider it as general purpose fan control. So question is which one it should be? And is there any reason why it should not be considered general purpose fan control?

As for chamber cooling fan and M42, that is off topic, but let my write my opinion on it and why I do not want to use it and want to go with M106 instead. As for chamber cooling fan, thing is that for it to work, you need to be able to set chamber temperature. And to set chamber temperature, you need to have chamber heating pin assigned (otherwise printer reports chamber temperature, but does not do anything with it). If you don't have (active) chamber heating, then you can either fake it with unused pin, or you can discard chamber fan feature altogether and instead invert chamber pin logic and connect chamber fan to chamber heating pin. This works pretty well and I used it like this for long time, BUT here comes the trouble. If you do this, you need to switch off runaway protection for chamber. And if you switch off runaway protection, then Octoprint will start yelling at you every time you connect. This can be solved with hacking runaway protection (more like butchering it) by setting 0 to watched parameters. All great - but honestly this is way too much work just to simply switch on a fan. Especially considering that butchering runaway protection like that is a bad idea, even though I know what I am doing and I know that in my specific case there is no risk. As for M42, I suppose I could use it, but why? I am not trying to control some wieird hardware, I just want to run a fan. Ordinary fan that is already supported by Marlin. But let's say I do want to use M42. Is it even possible to pass 32bit board pin number in P parameter? And even if it is, including specific board pin number in generated GCode seems just wrong. I am not a fan of gcode reusability for different printers, but tying GCode to specific board setup (and therefore allowing it to do all kinds of mayham on different setup) is just a whole new level of bad.

Graylag-PD commented 2 years ago

There is no reason to call M106 without P parameter and if user decides to do that, than he bears the responsibility for his actions.

Scratch that, I just looked into how slicers generate GCode and indeed they generate just M106 without P. This means now we would have to add some code that defaults to FAN0 if no parameter is given and SINGLENOZZLE is present. Oh my, the rabbit hole goes deeper. I knew there was something that I was missing...

Graylag-PD commented 2 years ago

New idea, looking at M106_M107.cpp, it should be enough to modify this part:

#if ENABLED(SINGLENOZZLE)
  #define _ALT_P active_extruder
  #define _CNT_P EXTRUDERS
#else
  #define _ALT_P _MIN(active_extruder, FAN_COUNT - 1)
  #define _CNT_P FAN_COUNT
#endif

to 

#if ENABLED(SINGLENOZZLE)
  #define _ALT_P 0
  #define _CNT_P FAN_COUNT
#else
  #define _ALT_P _MIN(active_extruder, FAN_COUNT - 1)
  #define _CNT_P FAN_COUNT
#endif

plus above mentioned change in temperature.cpp (or more likely to delete _CNT_P constant and replace it directly with FAN_COUNT)

Change of _ALT_P to 0 does exactly same thing as that line in temperature.cpp, which is if no P parameter is given, default to 0. And change of _CNT_P unifies it with non SINGLENOZZLE behaviour. Only difference would be in that now it enables to explicitly call additional fans by calling P parameter.

But as before, I do not understand what was the idea behind this piece of code, so correct me if I am again missing something. Perhaps it used to make sense before and now is just left over?

Graylag-PD commented 2 years ago

An update, solution proposed by me works well enough for my purpose. So far I have not found any significant issues or interactions. I will do some more testing but If all is good, I will make a PR.

Mrnt commented 2 years ago

@InsanityAutomation / @thisiskeithb is M106 meant only for part cooling fans ie not manual control of chamber fans etc? It is not clear from the Marlin docs.

InsanityAutomation commented 2 years ago

My understanding is part coolers, not arbitrary fans. Final decision there would be up to @thinkyhead

Graylag-PD commented 2 years ago

My opinion, the M106 fans control is embedded into lot of other functions, including thermal control, so, as much as I hate to say this, without serious review of Marlin code I would agree with @InsanityAutomation . Using M106 as general purpose fan can have various side effects and although I have not observed any with my printer, it does not mean someone else would not have issues. BUT(!) two things need to be taken into account: a) there is no dedicated general purpose G-code command for general purpose fans and adding one can bring lot more trouble than to just making sure M106 won't have side effects b) in my opinion the Rubicon was already crossed with introduction of NUM_M106_FANS which does not make much sense if M106 was intended for part coolers only. Fan definition is no longer tied to tools, which means users will experience side effects regardless. Which is kind of proven by existence of this bug report.

Long term solution should be to fully separate M106 from heaters and tools, probably including piece of code to tie only specific fans to specific tools.

InsanityAutomation commented 2 years ago

The num fans option was added where single nozzle on a board with the second fan defined, but not used, was causing conflicts. That eliminated it by forcing it to a number lower than the number of extruders.

While M42 exists for arbitrary pin control, including fans, I can see where it's more complex than some would like.

On the flip side, adding tons of configuration options to make them all flexible adds that complexity to all users so I can also see that being undesirable.

Given how slicers operate it's been a long standing assumption, so would definitely need careful consideration before any change.

Graylag-PD commented 2 years ago

On the flip side, adding tons of configuration options to make them all flexible adds that complexity to all users so I can also see that being undesirable.

Given how slicers operate it's been a long standing assumption, so would definitely need careful consideration before any change.

No argument here

The num fans option was added where single nozzle on a board with the second fan defined, but not used, was causing conflicts. That eliminated it by forcing it to a number lower than the number of extruders.

Ok, maybe. But then it is fundamentally broken. If it were intended to limit number of used print cooling fans, it should have never let user define more fans then there is nozzles. This is exactly the point of this bug report. Marlin happily creates 3 separate M106 fans, but does not let me use them. Either a sanity check should have had stopped me, telling me that no more fans than nozzles should be defined. Or it should have worked as general purpose fan.

While M42 exists for arbitrary pin control, including fans, I can see where it's more complex than some would like.

Sorry mate, but suggesting to use M42 for this is not more complex, it is bad joke. For start, I cannot input STM32 pin number into P parameter, I need to use "Arduino" pin number. Which is impossible to figure out without using M43 I. None of this is of course documented on web. But even if I do that, my board will just crash if I don't use I1 parameter in M42, because that specific pin is defined in pins file (and yes, I know I can edit pins or do undef in config). So as a result I should embbed my sliced G-codes with hardware specific command with obscure pin identification and I1 parameter on top? Not only there is zero chance generic user will figure this out. But can you imagine what kind of mayham will happen if I either make typo in that "Arduino" pin number or if anyone tries to run this file on different printer? Would you be comfortable posting this into a tutorial on web? I respect M42 as debug and development tool. And yes, it can be made to work this way. But should it? I think not. BTW, how do you propose M42 controlled fan should be controlled from LCD?

thisiskeithb commented 2 years ago

I commented above, but you can check https://github.com/MarlinFirmware/Marlin/pull/18883 for a history of NUM_M106_FANS with some good comments from thinkyhead about future sanity checks.

InsanityAutomation commented 2 years ago

You can create custom user menus with the M42 commands for fans as desired, but it would be static speeds, not numeric entry.

I said M42 IS more complex, not is not. Im aware it has limitations on usability. I dont recommend it as typical practice. What youre doing is well beyond the typical user in the first place.

Keep in mind, even M42 is not much difference in complexity of doing the same in RRF or Klipper where its equally hardware pin dependent...

Graylag-PD commented 2 years ago

What youre doing is well beyond the typical user in the first place

True, and you are right, that I would be able to make it work with M42, including custom user menu. I am going to use M106 as it is easier and works equally well for me, but I could have used M42, point taken.

Nevertheless, I think this topic will still need to be discussed in near future - from typical users perspective. Prusa recently announced their enclosure with integrated air filtering (effectively just a fan with filters, just as I have) and they plan to introduce software control from board. Which means within few months half of the chinese manufacturers will clone their solution, resulting in massive ammount of users asking how should they configure their filtering in Marlin.

Mrnt commented 2 years ago

What if we use M106 with say C# as a parameter to indicate a chamber fan, leaving P# as the parameter for part cooling fans, and so on for future fan types?

So for chamber fans -

Mrnt commented 2 years ago

Using M42 with the possibility of entering the wrong port number and causing very bad things to happen seems like an experts only solution, and should be left for testing or one-off type solutions - isn't that the reason the command is disabled by default?

Graylag-PD commented 2 years ago

To be fair, as long you don't use the I1 parameter, the M42 is relatively safe. In fact I can imagine that with few changes the M42 could be suitable solution for typical user:

Where I can see real issue is in control from LCD. Although custom menu is suitable for one-off solutions, for general audience it would be necessary to make some new LCD menu with sufficiently flexible configuration options.