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

🚧 filament runout: only allowed if E-axis is active #4325

Closed
wants to merge 1 commit into from

Conversation

gudnimg
Copy link
Collaborator

@gudnimg gudnimg commented Aug 13, 2023

Only allow filament runouts when the extruder motor is moving. We do this for FINDA runouts, so why not Fsensor as well?

I think this may improve situations like #4323

Change in memory:
Flash: -6 bytes
SRAM: 0 bytes

@github-actions
Copy link

github-actions bot commented Aug 13, 2023

Target ΔFlash (bytes) ΔSRAM (bytes)
MK3S_MULTILANG -6 0
MK3_MULTILANG 6 0

leptun
leptun previously approved these changes Aug 14, 2023
Copy link
Collaborator

@leptun leptun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a good idea, but I have a few notes:

What about triggerFilamentInserted()? Do we change to e_active() also there? I'm not sure what such a change would have as a consequence. Do we want to allow filament autoloads when axes are moving, but E is idle?

Also, e_active() can be sped up by breaking out of the loop early when an E move is found in the first block.

@leptun
Copy link
Collaborator

leptun commented Aug 14, 2023

#3885 will also need rebasing and rechecking after this is merged

@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 14, 2023

What about triggerFilamentInserted()? Do we change to e_active() also there? I'm not sure what such a change would have as a consequence. Do we want to allow filament autoloads when axes are moving, but E is idle?

For this scenario I think it makes sense to not allow autoloading if the axis are moving. I don't see why a user would need to load while the motors are moving 🤔

Also, e_active() can be sped up by breaking out of the loop early when an E move is found in the first block.

I tried it and saw having two return's seems to increase code size. But perhaps the speed improvement is worth it.

Comment on lines +131 to 132
e_active()
|| printJobOngoing()
Copy link
Collaborator Author

@gudnimg gudnimg Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking back at this... shouldn't this be e_active() && printJobOngoing()? I don't know of any situation where we can recover from filament runout except when printing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is first layer calibration a printJobOngoing()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's only true if card.sdprinting = true, which happens only when a gcode file is printing. Or when usb_timer.running() is true, which only happens if the user is sending commands via Serial

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filament runout during First Layer Calibration is ignored explicitly. Similar to how we disable it when MMU is enabled.
image

@3d-gussner 3d-gussner added this to the FW 3.14.0 milestone Aug 22, 2023
@gudnimg gudnimg marked this pull request as draft September 12, 2023 09:28
@gudnimg gudnimg changed the title filament runout: only allowed if E-axis is active 🚧 filament runout: only allowed if E-axis is active Sep 12, 2023
@3d-gussner 3d-gussner requested a review from leptun September 12, 2023 10:11
@3d-gussner 3d-gussner dismissed leptun’s stale review September 12, 2023 10:12

More changes are coming and back to draft

@3d-gussner 3d-gussner added the in progress We are working on solving this one label Sep 12, 2023
We do this for FINDA runouts, so why not Fsensor as well?

Change in memory:
Flash: -6 bytes
SRAM: 0 bytes
@gudnimg gudnimg force-pushed the filament-runout-improvement branch from 8678366 to 70a22d4 Compare September 14, 2023 20:52
@gudnimg
Copy link
Collaborator Author

gudnimg commented Sep 14, 2023

Rebased to sync with MK3 branch (123 commits)

Copy link
Collaborator

@DRracer DRracer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about the e_active(). Technically, it seems to be correct, but I don't see all the consequences.

This part has been a bit sketchy in the past...

@3d-gussner 3d-gussner modified the milestones: FW 3.14.0, FW 3.14.1 Dec 1, 2023
@gudnimg
Copy link
Collaborator Author

gudnimg commented Jan 21, 2024

Probably not going to finish this PR.

@gudnimg gudnimg closed this Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress We are working on solving this one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants