search

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

[RFC] Organization of Extensible UI #13201

Closed marcio-ao closed 5 years ago

marcio-ao commented 5 years ago

@thinkyhead: Currently, the FTDI Touch Screen UI code is housed on the LulzBot code repo, where it is integrated into our FW. In order to make it more accessible for folks who may want to use it as part of Marlin for other printers, I think it makes sense for there to be a separate repo under the "MarlinFirmware" header.

Would it be possible for there to be such a repo added? And if so, could I be made a maintainer for it? If such a repo was available, I would keep the repo in sync with modifications made to our own FW and I could also add more documentation to it, and possibly address some of the concerns raised in https://github.com/MarlinFirmware/Marlin/issues/12096#issuecomment-464775869

Thoughts?

thinkyhead commented 5 years ago

Please use your existing fork of Marlin and make a new branch. Be sure to update your fork's copy of bugfix-2.0.x first, and create your new branch as a copy of bugfix-2.0.x. Once you have a basic integration completed, post a new PR. That PR will form the basis for further work on the feature.

InsanityAutomation commented 5 years ago

I guess the question stems more with how to handle examples for EXTui. Should the lib folder be placed in the machine example configuration with configuration and configuration_adv or should the EXTui chunks be managed in a separate repository to avoid growing size.

Which also brings up a question, should the config dir with default and examples be moved up above or into the Marlin folder for better visibility to end users?

thinkyhead commented 5 years ago

ExtUI-based devices should be adaptable to any machine with enough free pins, and should not be associated with configurations. I do believe they should live within the lcd folder as they are a part of the compiled sources.

should the config dir with default and examples

Probably, I have been considering it, since they are not part of the actually-compiled sources.

InsanityAutomation commented 5 years ago

FWIW we have several extui screens in the main code base now... Seems to have migrated away from this discussion a bit. I still have a long term concern for maintainability.

marcio-ao commented 5 years ago

@InsanityAutomation: Can you point me to the code for such screens? Just curious...

InsanityAutomation commented 5 years ago

In head we heave

//============================================================================= //========================== Extensible UI Displays =========================== //=============================================================================

// // DGUS Touch Display with DWIN OS // //#define DGUS_LCD

// // Touch-screen LCD for Malyan M200 printers // //#define MALYAN_LCD

And working mostly but needing another maybe 3-4 days dedicated work before ready to submit

https://github.com/InsanityAutomation/Marlin/tree/CrealityDwin_2.0

Note itll take awhile to get those dedicated days...

marcio-ao commented 5 years ago

It seems like most of these are relative small, about 2k lines of code max. Our UI is a behemoth in comparison, weighing in at 16k LOCs.

marcio-ao commented 5 years ago

It looks like most of these UIs off-load most of the actual work to a separate processor, with its own FW that draws the graphics and handles interaction. Ours is maybe the only one where the MCU itself is drawing the contents of the UI and handling the event loop, which is what makes it so large.

Still, if these other screens are making it into the source tree, I feel like ours should as well...

InsanityAutomation commented 5 years ago

Right, mine is bigger than these as well but the same DGUS hardware as the first above. I agree with youre statement there of they should be managed the same way yours is. Either in the machine example configs, seperate repo of Extui Libraries, or least optimal IMO in the main source. Consistency regardless.

marcio-ao commented 5 years ago

It also looks like the Malyan LCD is using some methods of the thermalManager, rather than using the abstraction provided by ExtUI. This is something I worried would happen, as there is nothing preventing folks from calling the underlying code.

marcio-ao commented 5 years ago

@InsanityAutomation: So it looks like there isn't very much consistency about where the source code for the various displays are stored. Right now, here's where things are located:

Maybe it would make more sense to have subdirectories under extensible_ui, one for each display:

InsanityAutomation commented 5 years ago

I would agree with that placement as the second best choice aside from the example configurations.

InsanityAutomation commented 5 years ago

Originally it was all internal and @thinkyhead converted it over. Seems some slipped through the cracks. Ill add it to my list to review the converted ones at some point.

InsanityAutomation commented 5 years ago

@boelle @shitcreek The decision was made and theyre merged in the main branch, this can be closed.

github-actions[bot] commented 4 years ago

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.