MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.28k stars 19.24k forks source link

[FR] Integrate low cost DWIN Touch panel #12096

Closed ModMike closed 5 years ago

ModMike commented 6 years ago

Feature request

Integrate the DWIN DMT48270M043_02WT used in the Wanhao and Monoprice touch screen printers.

This LCD interface is being used in several private Marlin builds. It would be a very nice low cost touch panel that compares favourably with standard displays at about $26.00 USD

Andrivet has built a fantastic interface and integrated the panel into Marlin 1.18 on his GitHub. Unfortunately it is specific to Wanhao and requires the enabling of additional serial ports in Marlin 1.18.

Would it be possible to build on his work and leverage his libraries to add it Marlin 2.0.x?

Andrivet Marlin GitHub

Andrivet DWIN DGUS GitHub

marcio-ao commented 6 years ago

@ModMike: You probably already know my opinion about closed-source Chinese hardware... :)

Anyhow, I encourage you to take a look at the work we've been doing with an open-source UI (that video is actually a bit old, it looks quite a bit better now). The entry point is just $25 and the code has already been written. We plan to release a commercial product using this new display in 2019.

marcio-ao commented 6 years ago

Although I have to say, I like the look of Andrivet's interface quite a bit...

ModMike commented 6 years ago

@marcio-ao Very nice and if thats the price point, amazing! I always admired your products and what the company stands for, unfortunately, for me, I need to deal with the hardware I have.

Although I have to say, I like the look of Andrivet's interface quite a bit...

As do I and at least the display portion is open source.

It would also help bring Wanhao and Monoprice owners into the Marlin mainstream. I believe a few other manufacturers are also using the same Dwin panel, just don't know who.

I will tell you one thing, I would much prefer your panel to the Panel Due I am putting on my Voron 2.1, that thing is a rank POS on several levels.

marcio-ao commented 6 years ago

@ModMike: Thanks for mentioning Adrivet's work. One of the things I am trying to implement is a standardized API for allowing custom touch UIs to talk to Marlin. It looks like what Adrivet has done might have a lot of overlap. I should reach out to him and see if there is any possibility for collaboration.

marcio-ao commented 6 years ago

unfortunately, for me, I need to deal with the hardware I have.

I fully understand. For all my criticism of Chinese printers, they have swooped in and added features that were really missing in 3D printers. I think we in the open-source world are trying to play catch up!

I will tell you one thing, I would much prefer your panel to the Panel Due I am putting on my Voron 2.1, that thing is a rank POS on several levels.

chuckle I've never been a fan of the Panel Due...

ModMike commented 6 years ago

You can try but he is very uh.... cantankerous. I offered him money to bring his whole project into Marlin and got a rather rude response. I think he is disappointed with the amount of Patreon supporters he got.

His whole approach seems a little misguided as well. Instead of developing a fork of Marlin 1.19 to support the Wanhao and panel, I suggested he work on integrating the panel libraries into 2.0.x seeing as it had the serial support and Wanhao owners could be mainstreamed.

I even tried communicating with him in French (I am in Montreal) and no dice. Maybe you will have better luck.

ModMike commented 6 years ago

@marcio-ao

I fully understand. For all my criticism of Chinese printers, they have swooped in and added features that were really missing in 3D printers. I think we in the open-source world are trying to play catch up!

You know something, that is an impressive attitude. Very few realists in this space.

marcio-ao commented 6 years ago

You can try but he is very uh.... cantankerous. I offered him money to bring his whole project into Marlin and got a rather rude response. I think he is disappointed with the amount of Patreon supporters he got.

He has about 209 more supporters than I ever managed to get on Patreon (on my personal account, on non-3D printer related things).

Instead of developing a fork of Marlin 1.19 to support the Wanhao and panel, I suggested he work on integrating the panel libraries into 2.0.x seeing as it had the serial support and Wanhao owners could be mainstreamed.

There is something to be said for sticking to a stable release -- certainly targeting 1.1.9 is the "safer" option. We were forced onto Marlin 2.0.x because the printer we are developing is using a 32 board :)

ModMike commented 6 years ago

@marcio-ao I just found this prior implementation of the DGUS_LCD.

https://github.com/haydendonald/Cocoon-Create-Touch-Plus

Can you layout the steps I need to follow to add this library? I don't mean exactly but general steps.

Once it's done I can import his work a little more easily.

I know you are against it but if it overlaps and everybody benefits, it may be a win win. I completely understand if you are not up for it.

marcio-ao commented 6 years ago

Can you layout the steps I need to follow to add this library? I don't mean exactly but general steps.

If you want to try implementing a new UI, take a look at the files in "Marlin/src/lcd/extensible_ui". You will need to write code for your LCD interface in the "lib" directory. Look at the "dummy.cpp" for a few stub functions that can be replaced with your own code. The file "ui_api.h" provide a bunch of setter and getter methods for things you may want to show and control from your display.

marcio-ao commented 6 years ago

Oh, and also enabled EXTENSIBLE_UI in "Configuration.h". That will ensure the stub functions in "dummy.cpp" get called from Marlin (you can rename that file to something suitable to your project).

ModMike commented 6 years ago

You know, I think I'll get the one you are working on when it's released. So much less work.

ModMike commented 6 years ago

@marcio-ao Do you have to use the case or can it be mounted in a custom enclosure? Do you know when the code will be merged into marlin?

marcio-ao commented 6 years ago

@ModMike: The display module from hotmcu can be put in a custom enclosure. All they really provide is a laser-cut acrylic frame, not anything more. If you end up buying from HotMCU, I recommend the larger one. Not only does it have more pixels, it also supports the portrait mode (which the other one does not).

You could also wait until our next TAZ is in production. We will be making a better LCD module which will include a speaker for sounds (something the hotmcu version does not have).

@Thinkyhead preferred not to merge the actual UI code into Marlin. What will happen is that we will maintain the code for the UI separately, and if someone wants it, they can just copythe source files into the "extensible_ui/lib/" directory.

ModMike commented 6 years ago

@marcio-ao The 4.3 looks like it may be a perfect drop in replacement for the one in the Creatbot. By the way, I see it can use a flex cable to connect to the controller. How many pins is it? It may be a truly drop in replacement if the order is the same.

I don't understand why the code is not merged, we are all dying for a nice touch screen. What were the issues?

marcio-ao commented 6 years ago

The 4.3 looks like it may be a perfect drop in replacement for the one in the Creatbot.

The only reason I advise against the 4.3 from hotmcu is because it comes with the FTDI FT800 GPU, which is less capable, has less RAM, and does not support portrait mode. With it, you will be limited to a landscape interface and you will never be able to do portrait mode. The only way to get the more capable FTDI FT810 GPU from hotmcu is by buying their bigger display.

But, if you are using it for a Creatbot, then perhaps the landscape option is all you need, as it looks like the Creatbot uses a landscape orientation for the LCD. You could also ask hotmcu whether they would be willing to build you a 4.3 inch module with the better FT810 chip. The board is electrically compatible, and it's really just a matter of swapping the LCD panel, so perhaps they would be willing to do it for you if you ask.

Our module will be a 5 inch display with a FT810 chip, plus an audio amplifier and a speaker and additional pins for an USB or SD adapter. It will use one of the ribbon cables from the RepRapDiscount display.

I don't understand why the code is not merged, we are all dying for a nice touch screen. What were the issues?

I don't think there were any issues. What did get merged is the API necessary for making the UI code a drop in replacement. Having it be a user choice as to what code they want to use for the UI opens up the possibility of different people designing different UIs for different purposes. It also allows vendors to come up with themed UIs particular to their printers, which is harder if there is one official UI in upstream Marlin. More competition, more choices, that's the idea.

marcio-ao commented 6 years ago

How many pins is it? It may be a truly drop in replacement if the order is the same.

Are you talking about the ribbon from the LCD panel to the board that drives the LCD panel? Or from the board that drives the LCD to the controller board on the 3D printer?

The FTDI FT800/810 use an interface called SPI. It uses general purpose I/O pins on the microcontroller. The DMT48270M043_02WT seems to use serial communications? That's a bit odd. Do you have a datasheet with protocol information?

Roxy-3D commented 6 years ago

And... as soon as Marlin users start using the touch panel... It will make sense to merge the code. The problem is, it is too much baggage to carry forward if only 1 or 2 people actually use a feature. (Marlin hardly ever drops support for a feature. So adding something as complicated as a touch panel display should be done with care...)

ModMike commented 6 years ago

@marcio-ao Here is the user manual with protocols:

http://www.ampdisplay.com/documents/pdf/DGUS%20v2.0%20(08-09-12.pdf

Frankly, if the API is merged, hopefully Andrivet will just port it over. Way easier than what he is doing now.

ModMike commented 6 years ago

@marcio-ao Sorry, here is the proper link for the data sheet:

http://www.victorvision.com.br/images/Datasheets/4.3/DMT48270M043_02W_DATASHEET.pdf

ModMike commented 6 years ago

@marcio-ao

Are you talking about the ribbon from the LCD panel to the board that drives the LCD panel? Or from the board that drives the LCD to the controller board on the 3D printer?

From the LCD Driver to the controller board.

InsanityAutomation commented 6 years ago

@Roxy-3D unless the menu and display was broken out to an API where an added menu option is parsed for all display types, it'd be a beast to maintain it all...

@marcio-ao I'm watching for that, the 32b board and switching extruder setup I saw on your Dev repo are interesting to me, along with the filament runout. A touchscreen I can actually add too is intreguing but also risky to turn into a headache especially with the tendency to somewhat slim ui at lulzbot. It would look nice next to my 3 Taz 6's though :)

marcio-ao commented 6 years ago

unless the menu and display was broken out to an API where an added menu option is parsed for all display types, it'd be a beast to maintain it all...

I don't think it is feasible to have a way to add options to all display types. With graphical displays, you want to control the layout so it looks nice and be usable and that is far too dependent of human judgement to be done automatically as new options are added.

Instead, I think if we define an API with callbacks, individual folks can come up with competing interfaces. Some interfaces may be for power users and have all of the options on the LCD (something like the Due Panel), but others may be have fewer controls in order to be easy to use. I don't think there is one interface that will please everyone, so I say, let it up to the user and the community to provide whatever they want. Marlin's responsibility is simply to maintain the API and add new hooks as features get implemented.

InsanityAutomation commented 6 years ago

@marcio-ao It would have to be done where a specifically designed button would override a generated one, and a toggle to turn off generated buttons and use manual defines exclusively. I agree it wouldnt be pretty. I also dont think anyone will take on such a time consuming task. It was just a thought of the only way I see it making sense to incorporate directly, feasibility aside :)

marcio-ao commented 6 years ago

It was just a thought of the only way I see it making sense to incorporate directly, feasibility aside :)

Currently Marlin supports text displays and the small 128x64 graphical LCDs. The menus are able to be common between the two only because we are limiting things to pretty much a menu of stacked textual options and a few numeric value adjusters. When you make the leap to a touch panel, it would be far too restrictive to limit the UI design like that. There are just so many opportunities for creative designs, plus add the fact that there are thousands of LCD panels sizes and resolution, each which might require different backend code. This is far too much to be included in standard Marlin. I think it's time the UI portion be separated from Marlin and allowed to thrive on its own. The requirement for that to happen is an set of common APIs people can depend on with each release of Marlin. If that is in place, I'm sure plenty of people will step up with UI code.

coldtobi commented 5 years ago

I'd be also interested to support DGUS / DWIN* displays, as I bought for my CL-260 a FSYETC F6 with their 4.3 LCD. They have provided Marlin code at their githhub repo (https://github.com/FYSETC/Marlin-F6), but well that code looks a bit hacky to me and certainly will cause me problems when I like to upgrade to a newer Marlin... So maybe we can talk about how to define a better abstraction for such intelligent displays which could be used by UI frontends.

I think the DGUS is interesting for Marlin example, because those displays allow to define most of the UI logic in the display, e.g you store a bmp on the display's flash, define touch sensitive areas and program the display what to do when the user taps on it, eg. "display another bmp", "increment/decrement value", generate an event/keycode. It can also automatically display variables at a predefined position and you just tell the display the value of the variable and it will update if for you... So I guess this type of Displays may be suitable for Marlin and can be done generic... (But for me I cannot yet confirm that before digging more into the code..)

I'd like to hack on it, but I will need a bit guidance as the Marlin codebase is quite huge and I'd like to avoid to come up with a design that you do not like :)

InsanityAutomation commented 5 years ago

As an FYI, creality uses this for the CRX and the 10s pro. I have a 1.1.9 branch that I have gotten this working on it is much more stable than the creality stock firmware which was incredibly buggy. I am planning on getting creality's implementation working in Marlin 2 at some point, obviously that means taking all of the hackish code and converting it to the EXT UI API. With any luck I'll have it done before mrrf :)

coldtobi commented 5 years ago

... to the EXT UI API...

Do you have a pointer for this new API? I Could not find it...

coldtobi commented 5 years ago

Ok, I've hacked now a bit and I've got some PoC running: https://github.com/coldtobi/Marlin in branch 1.1.x… Realising too late that I've should have used bugfix-2.0.0 or at least bugfix-1.1.x..

I'm started to the PoC to 2.0, but it would be great to get some feedback, though, to avoid that I'm heading the wrong direction.

What I have so far:

One formal question: Should I ask for an review using a PR (even if the overal feature is not ready yet) or is it OK to discuss this in this issue?

InsanityAutomation commented 5 years ago

@coldtobi I have a working implementation here - https://github.com/InsanityAutomation/Marlin/tree/Creality_DWINTest that's based around the creality factory screen code. I've already started work bringing it to 2.0 as well.

ModMike commented 5 years ago

Guys this is awesome! I have a Creatbot with a Dwin that is dying for an updated Marlin. I have everything working except the panel and USB key. I can live without the USB. As soon as 2.0 is ready, I can release the pin files for Creatbot DX.

InsanityAutomation commented 5 years ago

@ModMike keep in mind that every manufacturer implementation of the dwin screen will be different. It's very likely that the stock screen from them will need a different implementation. If we have the original source I can probably map it over fairly quickly.

ModMike commented 5 years ago

I have SOME of the source, they are using a closed source Marlin. I would love to be able to use Andrivet's screens. One of my friends did get it to work with the screen on 1.19 but it's a one off and I want to run a supported Marlin version. The board is a Rumba clone.

coldtobi commented 5 years ago

@InsanityAutomation thanks for the link, still browsing through it.

In the meantime I've ported my PoC to Marlin 2.0. However, I did not opt to use the EXT_UI interface as I ran into limitations very soon, so I decided to "derive" from MarlinUI...

I'm currenlty ovewhelmed by the many different compile time options, so I think I first need to get a plan what to hack next..

I've placed the display resources here: https://github.com/coldtobi/Marlin_DGUS_Resources (They are made from scratch to be sure that they are using Open Source stuff as resources.)

marcio-ao commented 5 years ago

In the meantime I've ported my PoC to Marlin 2.0. However, I did not opt to use the EXT_UI interface as I ran into limitations very soon, so I decided to "derive" from MarlinUI...

@coldtobi: Could you let me know what the limitations are? Since I am the original implementer of the EXT_UI, I can work to make the API better.

InsanityAutomation commented 5 years ago

@marcio-ao The two things I see right now is sending leveling data (either complete mesh or as points are probed) and any method to store LCD config. For example, creality touchscreens have a unit8 for volume and bool for language selected that they stored to eeprom. Also, the example could be more complete. I went to your development repo to get a better idea myself. If you get to any of that before I do, great! If not, ill make my way to it eventually... lol

marcio-ao commented 5 years ago

@InsanityAutomation: I see. Yes, those are two areas where the API has not been implemented. Our printers use linear leveling, so we didn't have a need for an API for other types of leveling so I decide to leave that for someone with a better understanding of the particular use cases.

As for the EEPROM access, this was a hard problem that I admit to have punted on. The Archim board has a SPI flash chip, so I opted to use that for storage in our UI instead of coming up with an interface to Marlin's EEPROM. Yes, that is cheating, because it only works on the Archim board. The biggest challenge I see with a generic interface to EEPROM is that Marlin already defines specific values that are stored in the EEPROM and from what I know, this space is very limited. Adding APIs to EXT_UI would either involve guessing what sort of specific values EXT_UIs will need and implementing those, or allowing a generic "block write" routine that would write or read a block of RAM to EEPROM. The block write approach would be the most flexible from an EXT_UI's perspective, but there would be the question about whether space would always be reserved in EEPROM at a specific size (potentially wasting space if EXT_UI is not being used) or whether there would be some mechanism to use "free" space dynamically, which brings up all sorts of interesting questions about what to do if the EXT_UI needs more storage than is available.

Also, the example could be more complete. I went to your development repo to get a better idea myself.

Yeah, better documentation is one area that was in the back of my mind as being necessary. My repo isn't particularly the best example because it has a lot of complexity which doesn't need to be there for a reference (such as variable display resolution and supporting both landscape and portrait mode). The FTDI chip is also a rather complex beast, so there is a lot in there to handle the idiosyncrasies of that particular chip. And as I said, I cheated in a couple areas, like using Archim specific SPI flash for storage, which I would not encourage.

So yes, I agree with you @InsanityAutomation. Those areas need to be addressed. Here is the start of a list of areas that need work:

@coldtobi: Are there any other areas you found lacking?

marcio-ao commented 5 years ago

Here is the user manual with protocols:

http://www.ampdisplay.com/documents/pdf/DGUS%20v2.0%20(08-09-12.pdf

Frankly, if the API is merged, hopefully Andrivet will just port it over. Way easier than what he is doing now.

@InsanityAutomation: I just reviewed the document @ModMike shared to get an idea of how this DGUS display works. This seems to be a bit simpler than the FTDI since it operates on the concept of pages, which are just plain old images that are stored to the SD card. The FTDI requires you to manually arrange and draw widgets with SPI commands, which complicates things a lot. With the DGUS, you would not need to do this. It appears most of the work with the DGUS is telling the display which areas are hot-spots. This is analogous to the FTDI's use of tags, with the difference being that in the FTDI there is a pixel-based tag buffer which assigns tag values to individual pixels as they are drawn, while in the DGUS the touch regions seem to be all rectangles.

Overall, I don't see any huge obstacles to getting the DGUS working with the EXT_UI; in fact, I suspect it would be far simpler than the FTDI. Unfortunately, since their FW does not appear to be open-source, I would not be able to work with it as an employee of Lulzbot, but I could offer assistance if anyone is wanting to take up the project.

coldtobi commented 5 years ago

@coldtobi: Could you let me know what the limitations are? Since I am the original implementer of the EXT_UI, I can work to make the API better.

Sorry if that sounded snappy, that was not my intention. (apologizes if it did)

I think its not a problem of the EXT_UI. It might needs some functions to control stuff (e.g "home X") but I think is is perfect for types of displays where you need to do everything, like draw your menues, going betwen the individual menues or drawing the data yourself. However, the DGUS displays are quite smart in this aspect so that you can offload most of the display logic to the display itself, and the display will take care to display your data, go through the menu structure and call you when the user has interacted with the display. So it is like a "push" vs "pull".

But I can also push data to the display and it will make sure to properly display it for me, the display is like a (serial) RAM for that purpose and in my implementation I'm using this for a more generalized approach: When there is a variable I can access, the ui/display driver does not need to know what is currently sending to the display, it just needs to know how to send it. It would be even possible to attach a subscriber/observer pattern to it so that the ui code can be quite dumb. EXT_UI uses a concept of Getter functions that needs to be called and this makes it for the abstraction I have choosen quite cumbersome, at least with the subset of C++ we can sensibly can use on those tiny MCUs...

At least data display this makes life easier; of course trigger some action from the display some more code is needed, but even here the display can assist: The controls will send you information when touched and there is also quite convenient functions like it can increment/decrement values for you and just send you the result…

This is why it felt much more natural for me to directly go to the MarlinUI level...

Regarding the EEPROM: Probably a more invasive change, but maybe some kind of simple EEPROM filesystem would help here? This would also help when vendors screw up EEPROM stuff, as e.g FYETC did on my new F6 and would also allow to encapsualte things better.. But I'm getting off-topic..

With the DGUS, you would not need to do this. It appears most of the work with the DGUS is telling the display which areas are hot-spots.

Yes, the design process is tedious and unfortunately requiring non-free (Windows) software*.… But once done and flashed to the display, you dont care anymore from the Marlin side.

(* though they have documented how the files the display reads are structured, so a free GUI generator would be possible)

marcio-ao commented 5 years ago

The controls will send you information when touched and there is also quite convenient functions like it can increment/decrement values for you and just send you the result…

@coldtobi: But the display has to send you data back to you in some form, such as an ID of the button that was pressed. So you can implement the entire display driver using just onStartup and onIdle. Not being an expert in the DGUS, forgive me if the following is wrong, but I suspect it would look something like this:

namescape ExtUI {
  void onStartup() {
    // Do whatever is necessary to initialize the DGUS
  }

  void onIdle() {
    // Check to see if there is response from the DGUS display
    action = getActionFromDGUSDisplay();
    value = getValueFronDGUSDisplay();

    switch(action) {
       case EXTRUDE_ACTION: setAxisPosition_mm(value, E0); break;
       case EXTRUDE_X: setAxisPosition_mm(value, X); break;
       case EXTRUDE_Y: setAxisPosition_mm(value, Y); break;
       case EXTRUDE_Z: setAxisPosition_mm(value, Z); break;
       case SET_TEMP: setTargetTemp_celsius(value, E0); break;
    }

    // Update variables for things which are shown on the status screen
    writeDGUSVariable(E0_TEMP_VARIABLE, getActualTemp_celsius(E0));
    writeDGUSVariable(E1_TEMP_VARIABLE, getActualTemp_celsius(E1));
    writeDGUSVariable(BED_TEMP_VARIABLE, getActualTemp_celsius(BED));
  }
} // namespace ExtUI

I think its not a problem of the EXT_UI ... I think is is perfect for types of displays where you need to do everything, like draw your menues, going betwen the individual menues or drawing the data yourself.

There is nothing in EXT_UI that says you have to draw the UI yourself. This is why I said implementing DGUS would be easier, because you don't need to do all that. In that case, the onIdle would simply respond to events from the display module.

coldtobi commented 5 years ago

The controls will send you information when touched and there is also quite convenient functions like it can increment/decrement values for you and just send you the result…

@coldtobi: But the display has to send you data back to you in some form, such as an ID of the button that was pressed. So you can implement the entire display driver using just onStartup and onIdle. Not being an expert in the DGUS, forgive me if the following is wrong, but I suspect it would look something like this:

Of course one could wrap code around ExtUI for an implementation sure… However I've chosen in my code another approach to reduce the amount of code required to handle $stuff; as a side effect I would only need a quite small subset of ExtUI (Init and Idle) and still need to hook into lower levels of Marlin, which then will make the architecture nasty. Or the design would not be as efficient as it could be (using more RAM, Flash and CPU) (Admitted, as usual in the engineering world, there is bikeshedding involved)

One design decision I've taken was to try describe the display logic in data, not in code; so there are are a few PROGMEM tables that define the (display) variables, the screens and the connection between the screen and the variables,so that one can only send the variables to the currently displayed screen. (Striclty this is an optimization, but the bandwith of the serial line is limited so I try to limit the chatting with the display)

But the key is that the Variables are declarative as well: They map the display variable's VP to a pointer where the Marlin data is and two functions that can be used to send the data to the display resp. to be called when the display tells that the value of the variable has changed. (this struct would be here: https://github.com/coldtobi/Marlin/blob/eaaebf483b8939c7ccf4dbff56c66032c404f664/Marlin/src/lcd/dgus/DGUSVPVariable.h#L15 ; the operator= is currently not used but could be the starting point to employ an observer pattern to inform the LCD when a value has changed to get a complete event based implementation; but there I need to investigate more to see if that would be beneficial)

In code, this is the main struct that describes the interaction between Marlin and the Display: https://github.com/coldtobi/Marlin/blob/eaaebf483b8939c7ccf4dbff56c66032c404f664/Marlin/src/lcd/dgus/DGUSDisplayDefinition.cpp#L80; This table would also allow easy adaption to tweaks on the display design firmware *. E.g If you decide for whatever reason that you prefer to have one digit less for the current machine X position it is just replacing the template parameter <2> with <1> in line 114 of the above file.

(Downside is of course that by using pointers to the variables makes encapsulation bad.)

* I'm not sure how to call the display part of it... Maybe firmware, but this is not really catching it, as there is no programming...

I hope my design ideas make somehow sense, but its getting late for today.. I hope to be able to continue hacking on this tomorrow; after all temperature control is now working and when implemented homing support, I guess I'm ready to do the first print on it ;-))

Cheers, tobi

marcio-ao commented 5 years ago

However I've chosen in my code another approach to reduce the amount of code required to handle $stuff; as a side effect I would only need a quite small subset of ExtUI (Init and Idle) and still need to hook into lower levels of Marlin, which then will make the architecture nasty.

The init and idle are only part of it. The major motivation behind the ExtUI is to make getter and setter methods to hide the complexity of actually reaching into the guts of Marlin the way UltraLCD does. However, you are saying you would still need to do so -- I guess until I look at your code I won't really understand why.

They map the display variable's VP to a pointer where the Marlin data is and two functions that can be used to send the data to the display resp. to be called when the display tells that the value of the variable has changed.

Well, if you are accessing data via pointers, I see why getter and setters will be difficult. The problem you will run into is that if those variables change around in Marlin, or if their semantics change so you can't just change their values without calling some other function, then your approach will break.

UltraLCD faces a similar problem because it also uses pointers to fixed length types. A lot of the UltraLCD ends up having to call a callback function after tweaking values to apply and validate the changes, so you will need not only a pointer to a variable in memory, but also a pointer to callback function. So... the complexity starts to add up. As they say, the devil is in the details.

Sounds like you may have an approach in mind, though. I won't discourage you further, but at least I thought I would let you know of the potential pitfalls.

coldtobi commented 5 years ago

However I've chosen in my code another approach to reduce the amount of code required to handle $stuff; as a side effect I would only need a quite small subset of ExtUI (Init and Idle) and still need to hook into lower levels of Marlin, which then will make the architecture nasty.

The init and idle are only part of it. The major motivation behind the ExtUI is to make getter and setter methods to hide the complexity of actually reaching into the guts of Marlin the way UltraLCD does. However, you are saying you would still need to do so -- I guess until I look at your code I won't really understand why.

They map the display variable's VP to a pointer where the Marlin data is and two functions that can be used to send the data to the display resp. to be called when the display tells that the value of the variable has changed.

Well, if you are accessing data via pointers, I see why getter and setters will be difficult. The problem you will run into is that if those variables change around in Marlin, or if their semantics change so you can't just change their values without calling some other function, then your approach will break.

Sure, that's why pointed to the bad data encapsulation that might indeed be come problematic if Marlin refactors a lot. On the other hand in larges part of Marlin direct (read) access to them is not uncommon. For writing access I expected that I will usually will need special handlers (those set_by_display_handlers). With your input, I'll consider to use the setters provided by your API.

UltraLCD faces a similar problem because it also uses pointers to fixed length types. A lot of the UltraLCD ends up having to call a callback function after tweaking values to apply and validate the changes, so you will need not only a pointer to a variable in memory, but also a pointer to callback function. So... the complexity starts to add up. As they say, the devil is in the details.

Sounds like you may have an approach in mind, though. I won't discourage you further, but at least I thought I would let you know of the potential pitfalls.

You don't discourage me, your thoughts and input are very appreciated and it will for sure influence the choices I make e.g for using the setters as much as possible...

Thanks a lot for your feedback!

tobi

ModMike commented 5 years ago

@coldtobi Have you considered leveraging the code available here:

https://github.com/andrivet/ADVi3pp-Marlin/tree/advi3++dev

He also has an exquisite menu system that I installed.

https://github.com/andrivet/ADVi3pp-LCD

All (and that's a big all) you would need to do is modify Marlin 2.0 to work with it. One of my friends patched the whole thing together for Marlin 1.19 and it ran great on his Creatbot D600.

coldtobi commented 5 years ago

@ModMike yes, I saw this but I saw also your comment above about the author. If he do not like to be ported to Marlin this is a sign for me to stay away, even if the license would permit. (this is also some experience I take out from my work in Debian). Also his codebase diverged a lot from Marlin, so

(On a side, he is using a slightly different type of the display I have, so his artwork won't work on my display, as the variable adress map he is using is colliding with control registers on my display. My design considered to support both displays but first things first. (I do not own one of the other types, but I hope the datasheets are okish...)

coldtobi commented 5 years ago

@marcio-ao

@coldtobi: Could you let me know what the limitations are? Since I am the original implementer of the EXT_UI, I can work to make the API better.

I think I'll start a list too. Let me know if you prefer an extra issue for this... Thos are things I came across when I hacking on the display support and I will edit it it when I come accross new things:

- M0/M1 -- (Userfeedback) I'd like to show a popup here with an OK button, so I'd need some kind of OnUserFeedbackRequired() and something to set the user feedback (if I should avoid to manipulate wait_for_user directly. (#13327) PR has been merged, so it is available now - M73 support. also fixed...

jamesarm97 commented 5 years ago

I have a few of these screens sitting around and also a few printers that use them and was just looking to do the same thing. Just catching up on what everyone has been doing.

InsanityAutomation commented 5 years ago

I've got the Creality implementation about 80% within EXTui at this point. From there should be able to take and convert more slowly. Just need to keep extending the API as needed.

As mentioned about a boot screen earlier, Creality has an int that's incremented and set in the on startup section and when it completes sends the next screen. This can probably accomplish what you wanted there.

andrivet commented 5 years ago

I plan to port my DWIN DGUS Mini code to Marlin 2 at some point in the future. I will use the new API if possible (looks like it is). See https://github.com/andrivet/ADVi3pp/issues/178

InsanityAutomation commented 5 years ago

@boelle @shitcreek this is done too