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

Fix MMU2S without LCD #20140

Closed
wants to merge 4 commits into from
Closed

Conversation

Minims
Copy link
Contributor

@Minims Minims commented Nov 14, 2020

Requirements

  • MMU2S Unit
  • No LCD (connected to EXP1/EXP2)
  • Activate PRUSA_MMU2_S_MODE

Description

  • Compilation fails if MMU2 is enable without LCD and PRUSA_MMU2_S_MODE (This last option use function declare only in the LCD mode)
  • Fix %u to %hu for warning about short int.

Benefits

Make MMU2 with PRUSA_MMU2_S_MODE working without a LCD (I use a BTT TFT on serial)

Configurations

Archive.zip

Related Issues

No Issue opened

@GMagician
Copy link
Contributor

GMagician commented Nov 15, 2020

I think this is already been addressed in #19912 by thinkyhead, but not

Fix %u to %hu for warning about short int.

@sjasonsmith
Copy link
Contributor

@GMagician will this change end up conflicting with your PR?

@GMagician
Copy link
Contributor

My PR was just to add more extruder to mmu (needed by SMuFF), I changed mmu support to a per model case. The fix for lcd menus has been done by thinkyhead. I don't think it's a problem, just a double fix

@sjasonsmith
Copy link
Contributor

@GMagician I think I inverted your comment in my mind. I thought you were saying that ONLY that warning was fixed. I missed the not 😀

@Minims can you test with #19912 (and without this change) to see if it solves your LCD problem?

@Minims
Copy link
Contributor Author

Minims commented Nov 15, 2020

@sjasonsmith is there an easy way to apply the pr #19912 to my current GitHub bug fix-2.0.x branch ? Or do I have to cherry pick the 21 commits ?

@sjasonsmith
Copy link
Contributor

@Minims it depends a lot on the tools you use. I usually add their fork as a remote and just merge their branch into my own using a GUI tool (I use SourceTree). I'm sure there are plenty of other ways to get the job done in git.

Without using git the easiest thing is probably to navigate to the source branch for the PR, and download it as a zip file. Then just copy your config files into that source and build it.

@Minims
Copy link
Contributor Author

Minims commented Nov 16, 2020

@sjasonsmith SourceTree a quite easy to use, thanks.

So I have merge the PR in the current bugfix with my configuration. The issue is not solved. First error while compiling is :

./Marlin/src/inc/SanityCheck.h:    #error "PRUSA_MMU2S requires an LCD supporting MarlinUI to be enabled."

The thing I want to fix.

@GMagician
Copy link
Contributor

I think it's because sanity check is still there. There is a "comment" to thinkyhead about it, try removing it and check if it compile

@Minims
Copy link
Contributor Author

Minims commented Nov 16, 2020

Marlin/src/feature/mmu/mmu2.cpp:114:36: error: 'MMU2_RAMMING_SEQUENCE' was not declared in this scope; did you mean 'MMU2_CAN_LOAD_SEQUENCE'?
Marlin/src/feature/mmu/mmu2.cpp:115:43: error: 'MMU2_LOAD_TO_NOZZLE_SEQUENCE' was not declared in this scope
Marlin/src/feature/mmu/mmu2.cpp:980:25: error: 'MMU2_FILAMENTCHANGE_EJECT_FEED' was not declared in this scope
  • warning I ha ve fix about short int
Marlin/src/feature/mmu/mmu2.cpp:187:29: warning: format '%u' expects argument of type 'unsigned int*', but argument 3 has type 'int16_t*' {aka 'short int*'} [-Wformat=]
  187 |         sscanf(rx_buffer, "%uok\n", &version);
      |                            ~^       ~~~~~~~~
      |                             |       |
      |                             |       int16_t* {aka short int*}
      |                             unsigned int*
      |                            %hu
Marlin/src/feature/mmu/mmu2.cpp:198:29: warning: format '%u' expects argument of type 'unsigned int*', but argument 3 has type 'int16_t*' {aka 'short int*'} [-Wformat=]
  198 |         sscanf(rx_buffer, "%uok\n", &buildnr);
      |                            ~^       ~~~~~~~~
      |                             |       |
      |                             |       int16_t* {aka short int*}
      |                             unsigned int*
      |                            %hu

@GMagician
Copy link
Contributor

@Minims your "unsigned fix" was not covered by PR, but now it is.
I also fixed missing cnf values, would you try it again?

@Minims
Copy link
Contributor Author

Minims commented Nov 16, 2020

@GMagician seems ok now. Compile with success and no more warning :-).

@GMagician
Copy link
Contributor

The problem is...does it work? I don't have a MMU yet to test with so if you can do that it will help a lot

Minims pushed a commit to Minims/Marlin that referenced this pull request Nov 16, 2020
- Add MarlinFirmware#20140 unsigned warning fix
- Fix non inluded cnf when no mmu2 menus
@Minims
Copy link
Contributor Author

Minims commented Nov 16, 2020

I may be able to test next week end I think

@Minims
Copy link
Contributor Author

Minims commented Nov 17, 2020

So I have a small test on my SKR1.4, It seems working. Communication between MMU<>SKR is OK. And I can load a filament with G-code T0-T4. I can't really make a test print for now as my MMU2s is still not calibrated.

@thinkyhead
Copy link
Member

Hopefully I didn't break anything in this last merge. There have been other changes in this area which had some overlap.

@GMagician
Copy link
Contributor

@thinkyhead theoretically all changes in this PR have been already done in PR #19912

@Minims
Copy link
Contributor Author

Minims commented Nov 18, 2020

I will make a new test this evening, if all is OK, I will close this PR.

@Minims Minims closed this Nov 18, 2020
@Minims Minims deleted the mmu2s branch November 23, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants