Add PrusaMMU Support. Fixes #1176#5371
Add PrusaMMU Support. Fixes #1176#5371FieldofClay wants to merge 22 commits intoUnchartedBull:mainfrom
Conversation
|
Hey, thanks to both of you for working on this! I'll be giving this a proper review in the coming days, and I'll try to think up some ways to test it. Meanwhile, there's some linter issues that need to be fixed. You can run that check locally with |
|
Looks like |
Hillshum
left a comment
Hillshum
left a comment
There was a problem hiding this comment.
Thanks for working on this (both of you really)!
I still need to spend some time making sure some of the internals work so don't consider these findings to be exhaustive.
Also I need to do some testing
|
I've added in some error handling, with notifications and a close button. The close button does the same thing as the close button in the PrusaMMU modal within Octoprint, hides the dialog/modal and allows it to fallback to the printer for selection after the configured timeout. Let me know if there are anything else that needs tweaks, I'll try to sort it out. Node isn't something I've done much with in the past. |
Hillshum
left a comment
Hillshum
left a comment
There was a problem hiding this comment.
Okay, the error handling looks good.
I will keep an eye out for changes and approving the checks. When you have things ready, mark this as ready to review and I'll inspect things thoroughly.
| catchError(error => { | ||
| this.notificationService.error( | ||
| $localize`:@@prusammu-init-failed:Failed to load filament settings`, | ||
| error.message || 'Unable to fetch PrusaMMU configuration', |
There was a problem hiding this comment.
This needs to be localized
|
Oh another thing needed: In |
|
I've been using it for the past few weeks without issue, let me know if it needs any further changes. |
Hillshum
left a comment
Hillshum
left a comment
There was a problem hiding this comment.
Okay, this is a thorough review, but I haven't tested. Hopefully I can do that in a couple of days
| } | ||
| const sourceSpools = spools.selectedSpools?.some(spool => spool !== null) | ||
| ? spools.selectedSpools | ||
| : spools.allSpools?.slice(0, 5) || []; |
There was a problem hiding this comment.
So if no spools are selected, this just assumes the first five overall spools are selected? If that's truly correct behavior, I think some explanation should be included as a comment.
Additionally, I know the MMU plugin has a capacity setting to accommodate modded MMUs, and I think the count here should respect that.
| @@ -0,0 +1,40 @@ | |||
| export interface Filament { | |||
There was a problem hiding this comment.
Can this be named PrusaFilament or something similar?
| name: string; | ||
| } | ||
|
|
||
| export interface FilamentSource { |
There was a problem hiding this comment.
| export interface FilamentSource { | |
| export interface PrusaFilamentSource { |
| map((selection: FilamentManagerSelections): Array<FilamentSpool> => { | ||
| if (selection.selections.length > 0) { | ||
| return selection.selections.map((sel): FilamentSpool => this.convertFilamentManagerSpool(sel.spool)); | ||
| return selection.selections.map( |
There was a problem hiding this comment.
So prior to your changes, the array index represented the tool. Is there a need for all this or can the index be used instead when preparing the spools in the MMU service?
|
|
||
| return sourceSpools.length > 0 | ||
| ? sourceSpools | ||
| .map((spool, index) => (spool ? this.convertFilamentManagerSpool(spool, index + 1) : null)) |
There was a problem hiding this comment.
Can the id + 1 be done in the MMU service? Tweaks for MMU compatibility should be limited to the scope of MMU code.
| this.source = this.configService.isSpoolManagerPluginEnabled() ? 'SpoolManager' : 'Filament Manager'; | ||
| this.filaments = currentSpools.map(filament => { | ||
| return { | ||
| id: filament.tool, |
There was a problem hiding this comment.
While the types don't indicate this, the filament service can send null as a value indicating a tool has no filament selected, so this block needs to handle that case.
|
One other thing: |
| if (prusaMMUSettings.filamentSource === 'prusammu' && prusaMMUSettings?.filament?.length) { | ||
| this.filaments = prusaMMUSettings.filament; | ||
| this.source = 'PrusaMMU'; | ||
| } else if ( |
There was a problem hiding this comment.
What happens when these two conditionals both miss? What if the MMU plugin is configured to use Spool Manager but then OctoDash isn't using a filament manager?
Perhaps the solution is an else clause below that leads to the user being informed that the configs don't match up?
3 participants
Firstly full credit goes to @AndersWinther-Dahl, this is just their prusammu branch with a few tweaks to merge it into the current OctoDash code base.
This PR adds support for the PrusaMMU OctoPrint plugin. When enabled it will prompt the user to select which filament to use and sends through the selection to the PrusaMMU plugin. @AndersWinther-Dahl did a nice demo of the functionality in this comment: #1176 (comment)