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

Allow bulge detector without encoder in filament detector #783

Merged
merged 1 commit into from Dec 3, 2015

Conversation

Projects
None yet
3 participants
@leonmf
Contributor

leonmf commented Nov 29, 2015

Allows the module to be instantiated if the bulge detector exists but
the encoder does not. Inhibits attachment to events for the encoder if
the encoder doesn’t exist.

I was able to test this with the bulge detector and it works as expected. I am not able to test the encoder functionality but that functionality should be completely untouched by this pull request.

Allow bulge detector without encoder in filament detector
Allows the module to be instantiated if the bulge detector exists but
the encoder does not. Inhibits attachment to events for the encoder if
the encoder doesn’t exist.
@wolfmanjm

This comment has been minimized.

Contributor

wolfmanjm commented Nov 30, 2015

As far as i can see this will do nothing more than the switch module would do. If the bulge detector fires it will issue a suspend regardless of whether it is printing. This is because you completely by passed the code that checks if there were any extruder movements, which is what stops this module from false alarming when not printing. The bulge detector was not protected from this because it was unlikely to trigger when not printing, unlike the encoder check, it also was added to provide a button to do a manual suspend when needed.
In order for this to do what you want (to not trigger when not printing) you need to do a bit more work. You need to add another detector (because the bulge detector is not what you want here) and check it at the same time as the encoder would be checked, using the same logic, so it only gets checked when it has printed (in this case the extruder has moved since the last check).

@leonmf

This comment has been minimized.

Contributor

leonmf commented Nov 30, 2015

I'm happy to go back and refactor this but let me explain my underlying assumptions because I think this is still valid code.

  • I was operating under the assumption that every byte counts and wanted to add as little new code as necessary to perform the job.
  • Allowing the bulge detector to be checked when there is no encoder doesn't hurt anything and may actually be desirable to someone with a real bulge detector. At the very least, it isn't worse.
  • It does actually do more than a switch does because it allows me to inhibit triggering of the suspend command by executing an M405 at the end of the script. A switch module will execute the suspend operation at any time and will cause crashes into the home switches any time it gets triggered when printing or not. I realize the changes you are suggesting allow me to leave the filament monitor enabled when not printing.
  • By only changing the bulge detector, I risked no breakage to the filament monitor which I have no way to test.

Do I need to remove the changes I've made and implement it as suggested or are the current changes also valid? I see a digital filament sensor as complementary to this change and will implement it but need to know whether I have to revert back to stock and start over or start from where I am.

@wolfmanjm

This comment has been minimized.

Contributor

wolfmanjm commented Nov 30, 2015

your current changes are fine, however as I said they do exactly what switch would do, except the addition of being able to activate/deactivate the detection. If that is all you need to do then leave it as it is and I'll merge it.

@leonmf

This comment has been minimized.

Contributor

leonmf commented Nov 30, 2015

That's all the functionality I need but I'm happy to make it better. Let me take a stab at adding digital_detector_pin and the associated code this evening.

@leonmf

This comment has been minimized.

Contributor

leonmf commented Dec 1, 2015

This shouldn't affect my solution as the system I use monitors both extruders with a single pin but I don't see how the filament detector works with multiple extruders.

It seems that we can monitor only one encoder but the get_emove() is effective for the active tool (right?). Multiple instances of this module won't work because it has no way of determining which instance it should be handling it. Is the intention to eventually add a second extruder to this monitor (then how would we handle n?) or is there a way to modify this to be able to determine it should be monitoring?

@leonmf

This comment has been minimized.

Contributor

leonmf commented Dec 1, 2015

OK, I did my modifications tonight and it doesn't trigger like it should. Can we merge this code and I'll see about making the suggested improvements in another pull request?

@wolfmanjm wolfmanjm closed this Dec 2, 2015

@wolfmanjm wolfmanjm reopened this Dec 2, 2015

wolfmanjm added a commit that referenced this pull request Dec 3, 2015

Merge pull request #783 from leonmf/fix/FilamentDetectorAllowBulgeOnly
Allow bulge detector without encoder in filament detector

@wolfmanjm wolfmanjm merged commit f5eb2bc into Smoothieware:edge Dec 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MrPozitiff

This comment has been minimized.

MrPozitiff commented Oct 19, 2016

Hello everyone! Does anybody know how to apply filament monitor to dual extruders on smoothieware? E.t. if first extruder working - then checked first filament monitor, and if worked secont extruder - then checked second filament monitor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment