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

Refactoring DGUS screens code #17142

Closed
wants to merge 9 commits into from

Conversation

Desuuuu
Copy link
Contributor

@Desuuuu Desuuuu commented Mar 12, 2020

Description

This PR aims to tidy up DGUS code by re-arranging and refactoring it.

Here are the main changes:

  • Splitting the two classes in DGUSDisplay.cpp/.h into separate files.
  • Moving display-specific files into separate folders.
  • Changing the VPHELPER defines which were calling sizeof on pointers. As a result, I re-added the size check in DGUSDisplay::ProcessRx.

Functionality should be unchanged after these changes.

@GerogeFu maybe you could take a look at this considering you've been working on this code recently.

@thinkyhead thinkyhead force-pushed the dgus-refactor branch 2 times, most recently from a60a7ea to 5ddc11f Compare March 13, 2020 00:39
@thinkyhead
Copy link
Member

I've done a rebase and squash to resolve conflicts. I merged the "DUGS" / "DGUS" patch earlier so that we can focus on the other changes here. To update your working copy use the Git console and type the following commands:

git fetch origin
git checkout dgus-refactor
git reset --hard origin/dgus-refactor

thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Mar 13, 2020
Isolate the rename part of MarlinFirmware#17142.
Co-Authored-By: Desuuuu <contact@desuuuu.com>
@GerogeFu
Copy link
Contributor

I will check. Give me some time.

@Desuuuu
Copy link
Contributor Author

Desuuuu commented Mar 14, 2020

I added a few useful functions for the screen in my last commit. They're documented in the T5UID1 guide (there's a copy available here).

@GerogeFu
Copy link
Contributor

GerogeFu commented Mar 14, 2020

Compilation not finished , link errors happened . But not the DGUS code part cause this. Maybe my pio or vscode cause that. I need time to check . It can't find <Servo.h> file for me. Weird. I am testing FYSETC S6 . Or you can try to compile to see if it's only my issue.

@Desuuuu
Copy link
Contributor Author

Desuuuu commented Mar 14, 2020

Compilation succeeds for me on the latest version of this branch with the example FYSETC S6 (with DGUS_LCD_UI_FYSETC defined) and the FYSETC_S6 PIO environment.

Do you have anything special in your configuration?

@GerogeFu
Copy link
Contributor

GerogeFu commented Mar 17, 2020

Forget about the compilation. My git cause issue .And i check and test with S6 board. I think the code is good.

@Desuuuu
Copy link
Contributor Author

Desuuuu commented Mar 21, 2020

I added two more functions to control the state of touch controls.

It can be useful for example when you have a touch control with a click sound over a button that can be disabled/hidden.

@ModMike
Copy link
Contributor

ModMike commented Apr 29, 2020

I have been waiting for this for a long time. This was the last piece in fully upgrading A creatbot to Marlin 2.0.

What DGUS interface do we use? ADVi3++?

Nul characters sometimes cause issues with GBK (2-byte) encoding.
@thinkyhead
Copy link
Member

thinkyhead commented May 10, 2020

The DGUS code has been significantly restructured, so the changes in this PR will need to be reexamined in light of the latest code. It might be a bit of a manual process, but it would be good to know what we have covered already versus what gaps this PR still fills in.

@thinkyhead thinkyhead added the Needs: Patch A patch is needed to fix this label May 10, 2020
@Desuuuu
Copy link
Contributor Author

Desuuuu commented May 10, 2020

I do think that's the best way to go.

The current DGUS code would probably benefit from some re-organization at least (which was the point of this PR pretty much).

I'm not really working on this anymore though so if anyone wants to pick this up, feel free to do so. If there's a need for this, I might pick it up later, provided I find the time and motivation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants