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

improved robustness of Printing menu during a dialog with Marlin #1898

Closed
wants to merge 14 commits into from

Conversation

digant73
Copy link
Contributor

@digant73 digant73 commented May 3, 2021

IMPROVEMENTS:

  • improved robustness of Printing menu during a dialog with Marlin (e.g. printing from onboard SD). Now when a dialog with Marlin is ongoing (e.g. M600) pressing on the "Resume" button will be simply ignored and an error message "busy processing, please wait..." is displayed on the notification bar just to inform the user that a dialog with Marlin is ongoing and it must be first completed.
  • config.ini has been updated reporting Marlin 2.0.8.1 as the suggested minimum version fully compliant with TFT fw

PR STATUS: ready to merge

NOTE:

  1. bug [BUG] Print pause not working with REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER disabled MarlinFirmware/Marlin#21813 needs to be fixed in order to cover all the possible Marlin settings
  2. PR fixes #21813 MarlinFirmware/Marlin#21828 fixing the above bug on Marlin must be merged on Marlin

EDIT 2021-05-16:

  • Marlin PR #21828 is present in the new official Marlin 2.0.8.1 so this PR is ready to be merged now
  • config.ini has been updated reporting Marlin 2.0.8.1 as the suggested minimum version fully compliant with TFT fw

@digant73 digant73 marked this pull request as draft May 3, 2021 14:29
@kisslorand
Copy link
Contributor

NOTE: fix MarlinFirmware/Marlin#21813 is required to fix one scenario and covering all the possible Marlin settings

That code area you identified in Marlin has been previously making problems. It is the same code that made Pause during print not to work OK if ADVANCED_PAUSE was enabled in Marlin.
I remember at the time being I also brought up the issue with that infamous if conditional but they (Marlin guys) told me that if the conditionals are altered something else (I do not recall what it was) will be broken.
I really hope some day someone will rewrite the logic behind it.

@guruathwal
Copy link
Contributor

The ADVANCED_PAUSE_FEATURE in marlin requires any type of LCD display to be enabled. This is because it requires user interaction on the LCD display. Without a display defined in marlin, the pause will be stuck forever waiting for user input.

@digant73
Copy link
Contributor Author

digant73 commented May 6, 2021

@guruathwal that was before the PR submitted by Alan (MarlinFirmware/Marlin#21671) and recently merged on Marlin. That PR made M600 available even in case NO LCD is present but HOST_ACTION_COMMANDS and HOST_PROMPT_SUPPORT are enabled. That was the only scenario not working. The fix (as reported at the end of my bug report on Marlin) is easy (simly add a condition used by Alan in M600.cpp).

@digant73
Copy link
Contributor Author

digant73 commented May 6, 2021

@kisslorand I tested all the scenarios, and with the fix all of them perfectly works. I would say that the best logic should be to check if the interaction can be provided (with LCD, with prompt, with both LCD and prompt) before M125 is engaged.
Yes, I totally agree that M125.cpp should be reviewed and improved a lot. There were always something wrong

@kisslorand
Copy link
Contributor

The ADVANCED_PAUSE_FEATURE in marlin requires any type of LCD display to be enabled. This is because it requires user interaction on the LCD display. Without a display defined in marlin, the pause will be stuck forever waiting for user input.

Yes and No. For example Octoprint provides popups for user feedback

@guruathwal
Copy link
Contributor

Yes and No. For example Octoprint provides popups for user feedback

Does it provides feedback when printing from OnboardSD and when host prompt support is disabled in marlin?

@kisslorand
Copy link
Contributor

I do not know that.

@oldman4U
Copy link
Contributor

oldman4U commented May 8, 2021

Your fix works well for CZ-Rebel and hopefully I can find some time to test it s as again soon.

Thank you

👍🏻

@digant73 digant73 marked this pull request as ready for review May 16, 2021 11:25
@oldman4U
Copy link
Contributor

Will change the readme asap.

Comment on lines +7 to +42
/**
* @brief Compare file/folder details according to sort settings
*
* @param name1 name of first file/folder
* @param date1 date/time for first file/folder
* @param name2 name of second file/folder
* @param date2 date/time for second file/folder
*/
bool compareFile(char * name1, int32_t date1, char * name2, int32_t date2)
{
// sort by date
if (infoSettings.files_sort_by <= SORT_DATE_OLD_FIRST)
{
// file with most recent date displays first in newest first and last in oldest first
return ((date1 > date2) == infoSettings.files_sort_by % 2);
}
// sort by name
else
{
uint16_t maxlen = (strlen(name1) < strlen(name2)) ? strlen(name1) : strlen(name2);

// compare each character
for (uint16_t i = 0; i < maxlen; i++)
{
// convert all upper case characters to lower case
char a = (name1[i] > 64 && name1[i] < 91) ? (name1[i] + 32) : name1[i];
char b = (name2[i] > 64 && name2[i] < 91) ? (name2[i] + 32) : name2[i];

if (a != b)
return ((a < b) == infoSettings.files_sort_by % 2);
}
// file with longer name displays last in ascending order and first in descending order
return ((strlen(name1) < strlen(name2)) == infoSettings.files_sort_by % 2);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@digant73 Please merge commits to your branch by rebase. It looks like you are manually adding changes instead of rebasing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that rebase doesn't work on my totoise git

Copy link
Contributor

@guruathwal guruathwal May 17, 2021

Choose a reason for hiding this comment

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

There is a rebase option in tortoise git. https://tortoisegit.org/docs/tortoisegit/tgit-dug-rebase.html
And there is a rebase option in github desktop app also under Branch menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but it doesn't work properly. It mixed very old chenges instead of the last ones.

@digant73
Copy link
Contributor Author

due to some problems with the merge, I will open a new PR with th eproper changes

@digant73 digant73 closed this May 17, 2021
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.

4 participants