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

[FR] status message support for ender v2 dwin #20349

Closed
RustedAperture opened this issue Dec 1, 2020 · 28 comments
Closed

[FR] status message support for ender v2 dwin #20349

RustedAperture opened this issue Dec 1, 2020 · 28 comments
Labels
T: Feature Request Features requested by users.

Comments

@RustedAperture
Copy link

Looking to get support for status (m117) to be shown on the print screen of the creality dwin screen.

@rhapsodyv rhapsodyv added the T: Feature Request Features requested by users. label Dec 2, 2020
@guytas
Copy link

guytas commented Dec 2, 2020

Same here.... I do not use usb connection so the LCD messages are important for me.
I'm trying to follow the code but I can't see how the host_action_notify() gets in the dwin.cpp

@RustedAperture
Copy link
Author

The only thing I can think of is that we can incorporate the status message to be a module so that we can read m117 from a module and put the string on the info screen.

@guytas
Copy link

guytas commented Dec 2, 2020

We could. I would love it.

@guytas
Copy link

guytas commented Dec 6, 2020

Ok, i was able to get the status message on the dwin screen. But this is an ugly patch. How can I reach someone to get some tips on how to do it? Or is anyone planed to do it anytime soon?

@RustedAperture
Copy link
Author

Do you have the code on your GitHub? I wanna see what you had to do, I have some knowledge of c++ but I'm not too good.

@guytas
Copy link

guytas commented Dec 6, 2020

No, not on my github. I still have to learn more about github.
What i did is forced the Marlin to save the message in status_message by removing a couple of #if directives in marlinui.cpp. Then in dwin.cpp, I go get the status_message and display it if it has changed.
I couldn’t find proper condition to get marlinui to store the status_message without involving tons of modifications. I’ll keep looking.

@RustedAperture
Copy link
Author

That is interesting, if I'm not mistaken I saw an if statement that checks if you enable the creality dwin, by using that you can store it in a variable by adding an else if.

@RustedAperture
Copy link
Author

What is the line you used to store it in a variable? Just put it here in a comment using markdown.

@guytas
Copy link

guytas commented Dec 7, 2020

Yes but this is not right to do it that way. But anyhow... here it is...

In marlinui.cpp line 41 (Sorry not sure what you mean by using markdown...

//#if HAS_DISPLAY
#include "../module/printcounter.h"
#include "../MarlinCore.h"
#include "../gcode/queue.h"
#include "fontutils.h"
#include "../sd/cardreader.h"
#if EITHER(EXTENSIBLE_UI, DWIN_CREALITY_LCD)
#define START_OF_UTF8_CHAR(C) (((C) & 0xC0u) != 0x80U)
#endif
//#endif

I didn't turn on the HAS_DISPLAY because it triggers several other things.

also, in line 69,

//#if EITHER(HAS_WIRED_LCD, EXTENSIBLE_UI)
uint8_t MarlinUI::alert_level; // = 0
char MarlinUI::status_message[MAX_MESSAGE_LENGTH + 1];
//#endif

and then, in line 1615, we need to remove the following code:

//
// Send the status line as a host notification
//
/*
void MarlinUI::set_status(const char * const message, const bool) {
TERN(HOST_PROMPT_SUPPORT, host_action_notify(message), UNUSED(message));
}
void MarlinUI::set_status_P(PGM_P message, const int8_t) {
TERN(HOST_PROMPT_SUPPORT, host_action_notify_P(message), UNUSED(message));
}
void MarlinUI::status_printf_P(const uint8_t, PGM_P const message, ...) {
TERN(HOST_PROMPT_SUPPORT, host_action_notify_P(message), UNUSED(message));
}
*/

This will make the status_message to hold the messages.
Then, you can do whatever you want in the dwin module accessing MarlinUI::status_message directly

@RustedAperture
Copy link
Author

Instead of removing the line 1615 section what if we wrapped it in a #if has_display? Seems like the viable option that way we don't have to remove the code and people who have the display will still have access to it?

@guytas
Copy link

guytas commented Dec 7, 2020

None of this change is suitable to be release. This is a dirty patch. The real mod can’t be done that way. I need to talk to one of the contributors to see what would be the way to go.

@RustedAperture
Copy link
Author

Obviously the real mod cannot modify the code to the point it requires other people to change the config. Go a lack of words, we need a zero impact patch.

@RustedAperture
Copy link
Author

@thinkyhead might be able to help with this as I've seen he works on the creality printers alot. Even if he can't some advice on the idea would be great help I'm sure

@guytas
Copy link

guytas commented Dec 7, 2020

I was under the impression that if we define

#define GLOBAL_STATUS_MESSAGE

that it would be all. But no, it fails because START_OF_UTF8_CHAR isn't defined. This is defined if HAS_DISPLAY is on. Then it goes on and on requiring more and more. I'm afraid to change too much by defining all of them. Not sure why START_OF_UTF8_CHAR is under a condition at all.

@RustedAperture
Copy link
Author

another person I noticed that is working on E3V2 and sometimes on the DWIN is @sjasonsmith over the last few days he has done work on fixing DWIN menus. he should also be able to help with this.

@RustedAperture
Copy link
Author

I was under the impression that if we define

#define GLOBAL_STATUS_MESSAGE

that it would be all. But no, it fails because START_OF_UTF8_CHAR isn't defined. This is defined if HAS_DISPLAY is on. Then it goes on and on requiring more and more. I'm afraid to change too much by defining all of them. Not sure why START_OF_UTF8_CHAR is under a condition at all.

the reason that it appears to be separate is because it is also defined in 'lcdprint.h' which doesn't seem to be used by the DWIN class

@RustedAperture
Copy link
Author

is there a way we can get someone to look at this? its definitely possible but only with what seems like substantial code changes.

@RustedAperture
Copy link
Author

No longer going to pursue this as i have replaced my screen with a 12864 LCD and just compiled marlin using the ender 3 pro settings for the lcd.

@fbertu
Copy link

fbertu commented Jan 21, 2021

Hi! Any news??

@RustedAperture
Copy link
Author

RustedAperture commented Jan 21, 2021

I'm now running a BTT SKR mini e3 with a 12864 LCD since it was just too difficult to pursue adding things to the interface that just weren't meant to be there.

@jarfil
Copy link

jarfil commented Feb 7, 2021

I've decided to take a look at this, but there seems to be some general define cleanup required.

jarfil/Marlin@e265ab8

It also looks to me like the whole DWIN display should be able to work with many kinds of UIs, not sure why that isn't an option.

@guytas
Copy link

guytas commented Feb 7, 2021

Ho I’m so glad. If i can do something to help, or test, let me know.

@jarfil
Copy link

jarfil commented Feb 9, 2021

Got it to show a single line of status message at the bottom of the display.

jarfil/Marlin@fc6b2ee

This is good enough to show M117 messages, but there is still some conflict with the Marlin Core UI. Hopefully this will get solved if the Ender3v2 UI gets moved over to ExtUI.

Unfortunately this collides with #20983

@Jyers
Copy link
Contributor

Jyers commented Feb 9, 2021

The status area in #20983 is built to show M117 messages above it, in between the status area and the menus. This is how I have it build in my custom dwin firmware and it works well.

@jarfil
Copy link

jarfil commented Feb 9, 2021

@Jyers Yes, I see you've done quite a rewrite, with a different take on M117 processing. I've tried to reuse as much of MarlinUI as possible, but it seems like it all requires some rewrite and cleanup. What are your thoughts on moving all of this to ExtUI? It feels to me like there are already too many special cases for handling DWIN_CREALITY_LCD all over the place.

@Jyers
Copy link
Contributor

Jyers commented Feb 9, 2021

I totally agree, I hate how much extra needs to be added to the base marlin for the dwin display. I would love to move it over to ExtUI, the only issue is time. I started the rewrite before I even really knew anything about ExtUI, as all I really wanted was to clean up the dwin code. But I do believe with a little work, it wouldn't be the worst thing to move my code over and have it function seamlessly with ExtUI. For now I'm going to focus on the brunt of debugging and cleaning up my version, then I can start the work on moving it over.

@thisiskeithb
Copy link
Member

Added in #22422

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: Feature Request Features requested by users.
Projects
None yet
Development

No branches or pull requests

7 participants