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

Various bugfixes, timer rollover bugs, encoder, long press freeze bug, encoder issue at higher values #2967

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rondlh
Copy link

@rondlh rondlh commented Dec 11, 2024

Various bugfixes and small enhancements

Requirements

No specific requirements

Description

Bug fixes
Solves various timer rollover bugs as discussed in #2832
Solves the encoder not working well bug at higher values bug (speed and feedrate)
Solves the long press freeze bug when pressing the title bar
Corrects the UART6 pins on MKS TFT35 V1.0

Enhancements
improved handling of back button and long press button, 2 additional serial baud rates
Long press functionality can now easily be added to any icon.
Adds long press functions (jump to high temp without clicking 20 times, adjust movement speed, jump to temp from extruder menu)
Allow to jump from the first page to the last page in the console and visa versa.

Benefits

Squashing bugs, and small enhancements improving usability. For example, TFTs that don't have a encoder need to be pressed 20 times to go to 200 degrees C. The long press functionality makes that a 1 click action now. The same is true for the bed, which can jump to 75 degrees, or any degree you set.

Related Issues

#2832 (closed, but was not solved).

…der bug

Solves various timer rollover bugs
Solves the encoder not working well bug
Solves the long press freeze bug
Corrects the UART6 pins on MKS TFT35 V1.0

Enhancements, improved handling of back button and long press button
2 additional serial baud rates
Adds several long press functions (jump temp, adjust movement speed, jump to temp from extruder menu)
@rondlh
Copy link
Author

rondlh commented Dec 11, 2024

Anybody, please download and test, try long pressing the movement amount in the "move menu", or the "+" or "-" in the "Heat menu". The long press freeze bug should be solved now, but I still found some freezes in the console, when the motherboard is being restarted, I have not yet located that bug.
In the terminal you can now jump from the first page to the last and the other way around.

@bigtreetech Could you please add the current status to the releases? The last release if from 2021 and is quite outdated.

UPDATE: The back button has some issues, still needs fixing

"if (a == true)" is the same as "if (a)"
"if (a == false)" is the same as "if (!a)"
Some small comment formatting fixes
Small optimization
@rondlh rondlh marked this pull request as draft December 15, 2024 11:26
digant73

This comment was marked as duplicate.

@@ -709,7 +710,6 @@ LISTITEMS * getCurListItems(void)
GUI_POINT getIconStartPoint(int index)
{
GUI_POINT p = {curRect[index].x0, curRect[index].y0};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not remove the empty line before any return statement. It has been added to highlight the return point

@@ -765,7 +765,6 @@ void menuRefreshListPage(void)
for (uint8_t i = 0; i < ITEM_PER_PAGE; i++)
{
RAPID_PRINTING_COMM() // perform backend printing loop between drawing icons to avoid printer idling

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not remove the empty line before any GUI call. It has been added to separate GUI from non GUI calls

@@ -799,7 +798,6 @@ void setMenu(MENU_TYPE menu_type, LABEL * title, uint16_t rectCount, const GUI_R
void menuSetTitle(const LABEL * title)
{
curTitle = title;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as previous comment

@@ -820,7 +818,6 @@ void menuDrawTitle(void)
if (toastRunning())
{
drawToast(true);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as previous comment

@@ -883,7 +880,6 @@ static void itemDrawIconPress(uint8_t position, uint8_t is_press)
if (curListItems->items[position].icon == CHARICON_NULL)
{
GUI_ClearPrect(rect);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as previous comment

@@ -174,6 +175,7 @@ static inline uint8_t TS_CalibrationEnsure(uint16_t x, uint16_t y)
GUI_DispStringCenter(LCD_WIDTH / 2, LCD_HEIGHT - 40, (int32_t)LABEL_ADJUST_FAILED);
GUI_DispDec(0, 0, lcd_x, 3, 0);
GUI_DispDec(0, 20, lcd_y, 3, 0);
Buzzer_AddSound(100, 200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the buzzer call before the first GUI call and add an empty line before the first GUI call (separate GUI call from non GUI calls, when possible)

while (TS_IsPressed() == false);

while (!TS_IsPressed());
Buzzer_AddSound(BUZZER_FREQUENCY_HZ, BUZZER_FREQUENCY_DURATION_MS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line before this buzzer call. while, if, switch etc. must be always separated by empty line

@@ -4,6 +4,7 @@
#include "Notification.h"

#define STATUS_BAR_REFRESH_TIME 2000 // refresh time in ms
uint8_t longPressed = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line before this variable definition (separate by empty line #define etc. from variable definition etc.)

@@ -656,13 +658,12 @@ void drawBusySign(void)

busySign.status = SYS_STATUS_BUSY;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore the empty line after any if, switch, while etc.

@@ -198,13 +199,11 @@ void showLiveInfo(uint8_t index, const LIVE_INFO * liveicon, bool redrawIcon);
void displayExhibitHeader(const char * titleStr, const char * unitStr);
void displayExhibitValue(const char * valueStr);

KEY_VALUES menuKeyGetValue(void);
KEY_VALUES menuKeyGetValue(bool returnLongPressed);

// smart home
#ifdef SMART_HOME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the SMART_HOME is now always supported, remove this #ifdef from the entire project

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@digant73 Thanks a lot for the feedback! I just pushed an update reflecting your input.

About the SMART_HOME thing, it's still selectable, my changes don't affect this.
But... I see no real reason why one would want to disable it, so it could be removed if you ask me.
If you agree, then I can take out the Smart Home option everywhere needed.

BTW: The current status should be made into a release... it's a stable usable base. The last release is from 2021, which is very outdated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hhmm no, if still selectable, I would avoid to remove SMART_HOME (leave it as is)

…sistent

As commented by digant73 on December 17th.
Thanks!

Moved "return" command to separate line to be more consistent.
Add more feedback, I missed a few that were folded together in the comment section
@bigtreetech bigtreetech marked this pull request as ready for review December 25, 2024 01:30
@digant73
Copy link
Contributor

it seems ok

@rondlh
Copy link
Author

rondlh commented Dec 31, 2024

it seems ok

Great, thanks! Happy New year :D

@kisslorand
Copy link
Contributor

it seems ok

...but it isn't. Look again.

If you cannot spot the bugs this PR introduces, let me know, I can guide you.

@rondlh
Copy link
Author

rondlh commented Jan 22, 2025

@digant73 Don't waste your time listening to him, he is just trying to harm this repository...
If he really found a bug then he would have proudly rubbed it into my face already, but he looked and looked, but he found nothing :D
Anyway, he has already integrated the code into his own repository, which I consider to be a real endorsement.

@digant73
Copy link
Contributor

digant73 commented Jan 22, 2025

To speedup the merge I would consider to split this big PR in specific PRs:

  • Solves the encoder not working well bug at higher values bug (speed and feedrate).
    Solves the long press freeze bug when pressing the title bar.

  • Corrects the UART6 pins on MKS TFT35 V1.0.
    Added 2 additional serial baud rates.
    Allow to jump from the first page to the last page in the console and visa versa.

  • Improved handling of back button and long press button.
    Long press functionality can now easily be added to any icon.
    Adds long press functions (jump to high temp without clicking 20 times, adjust movement speed, jump to temp from extruder menu).

    NOTE: Avoid to add the bool argument in menuKeyGetValue() (restore the current method without any argument). Simply provide an additional function (e.g. menuKeyIsLongPress()) you can use in the 2 menus where you actually make use of long press.

  • Solves various timer rollover bugs as discussed in [BUG] Buggy time handling code (doesn't consider timer rollover) #2832.

    NOTE: This probably can be improved. There are some places where it is inefficient in your implementation

@digant73
Copy link
Contributor

@digant73 Don't waste your time listening to him, he is just trying to harm this repository... If he really found a bug then he would have proudly rubbed it into my face already, but he looked and looked, but he found nothing :D Anyway, he has already integrated the code into his own repository, which I consider to be a real endorsement.

there is a minor bug that could hang the TFT at startup I saw during the review but I forgot to report

static uint32_t posE_lastUpdateTime = FIL_ALARM_REMINDER_TIME
changed to
static uint32_t posE_lastUpdateTime = 0;
@rondlh
Copy link
Author

rondlh commented Jan 24, 2025

To speedup the merge I would consider to split this big PR in specific PRs:

* Solves the encoder not working well bug at higher values bug (speed and feedrate).
  Solves the long press freeze bug when pressing the title bar.

* Corrects the UART6 pins on MKS TFT35 V1.0.
  Added 2 additional serial baud rates.
  Allow to jump from the first page to the last page in the console and visa versa.

* Improved handling of back button and long press button.
  Long press functionality can now easily be added to any icon.
  Adds long press functions (jump to high temp without clicking 20 times, adjust movement speed, jump to temp from extruder menu).
  `NOTE:` Avoid to add the `bool` argument in `menuKeyGetValue()` (restore the current method without any argument). Simply provide an additional function (e.g. `menuKeyIsLongPress()`) you can use in the 2 menus where you actually make use of long press.

* Solves various timer rollover bugs as discussed in [[BUG] Buggy time handling code (doesn't consider timer rollover) #2832](https://github.com/bigtreetech/BIGTREETECH-TouchScreenFirmware/issues/2832).
  `NOTE:` This probably can be improved. There are some places where it is inefficient in your implementation

Thanks for the great feedback, I will look into it...

More elegant handling of long press function
@rondlh
Copy link
Author

rondlh commented Jan 24, 2025

I just updated the long press handling, your suggesting makes it much more elegant as it's only used in a few places, that was great feedback, thanks!

I just had a look into splitting the PR up, I found it means a lot of extra (and error prone) work and extra testing, while if I just use one version then I just upload it on my printers and test while using it.
I'm also not in a hurry, the current version of the software is already great, these changes are more in the nice to have category.
It would be great if a new release could be made of the current stable base, the previous release from 2021 is quite outdated.

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.

3 participants