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

[BUG] Reboot on USB/Serial Connect and EEPROM Errors #26030

Closed
1 task done
Artemonim opened this issue Jun 28, 2023 · 17 comments · Fixed by #26037
Closed
1 task done

[BUG] Reboot on USB/Serial Connect and EEPROM Errors #26030

Artemonim opened this issue Jun 28, 2023 · 17 comments · Fixed by #26037

Comments

@Artemonim
Copy link

Artemonim commented Jun 28, 2023

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

After merge commit 46b5753 my Ender 3 started rebooting when trying to connect to a computer via USB. I couldn't find in which pull request this commit was accepted, but it is visible in Git

Bug Timeline

new

Expected behavior

No response

Actual behavior

No response

Steps to Reproduce

  1. Merge 🎨 Indent temp structs with SHA 46b5753
  2. Build with Auto Build Marlin
  3. Firmware the printer
  4. Connect the USB cable
  5. Connect to a printer via Repetier-Host V2.3.1
  6. After less than 10 seconds the printer goes to reboot

Version of Marlin Firmware

2.1.x

Printer model

Ender-3

Electronics

BTT SKR MINI E3 V3.0

Add-ons

No response

Bed Leveling

No Bed Leveling

Your Slicer

None

Host Software

Repetier Host

Don't forget to include

  • A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

Configuration.zip

@Artemonim
Copy link
Author

2023-06-28_16-03-20

@ellensp
Copy link
Contributor

ellensp commented Jun 28, 2023

there are only white space change here... this doesn't make any sense. C doesn't care about white spaces

@ellensp
Copy link
Contributor

ellensp commented Jun 28, 2023

@Artemonim provided configuration files are the stock BOARD_RAMPS_14_EFB configuration files
Please try again

@Artemonim
Copy link
Author

Artemonim commented Jun 28, 2023

@Artemonim provided configuration files are the stock BOARD_RAMPS_14_EFB configuration files Please try again

0_o
New: Configuration.zip

UPD: Or I don't know what you're talking about

@ellensp
Copy link
Contributor

ellensp commented Jun 28, 2023

this one has MOTHERBOARD BOARD_BTT_SKR_MINI_E3_V3_0 which matches the board you listed in the issue.
So is much more likley to be configs used

@Artemonim
Copy link
Author

Yeah, maybe I accidently switch to the main branch when I zip the files :)

@thisiskeithb
Copy link
Member

All of my printers/boards are now bootlooping once any kind of serial connection is made (TFT or USB), so I'll mark this as confirmed.

We're also discussing bootloops in #25852 since this issue has come and gone a couple times this month through various code cleanups. This issue may get closed in favor of that one (or both will get solved today and we can close them all together!)

@thisiskeithb
Copy link
Member

thisiskeithb commented Jun 28, 2023

♻️ String helper class (#24390) is the offending commit causing boards to reboot on serial connect.

@ThomasToka
Copy link
Contributor

i saw this commit and thought: wt*. if i understand all this correctly every lcd display class using serial connections will have to be adjusted. am i right?

@RV-from-be
Copy link

RV-from-be commented Jun 28, 2023

I confirm that the printer reboot on USB. And not only, reboot also when trying to print via the SD card. This bug appeared since this commit 574dd34
I came back before this commit and everything is back to normal.
Tested on an Ender3V2, Ender3 v1 + v427 with the jyersui as the UI.
I also tested keeping all files from this commit 574dd34 except the dwin.cpp. Ditto reboot printers. In my opinion, there is an overflow. But I can't trace its origin.

@0xC4DE
Copy link

0xC4DE commented Jun 29, 2023

I am getting similar symptoms compiling for SKR V3 EZ. Let me know if there's anything I can help with. Would temp fix be to revert to before #24390 ?

I can extra confirm that this is an issue, reverting the commit and resolving conflicts favoring older seems to completely fix the USB pairing issue.

here's my configs for reference:
configs.zip

@ellensp
Copy link
Contributor

ellensp commented Jun 29, 2023

574dd34 is also cause of EEPROM datasize error

In settings.cpp

TERN(FLASH_EEPROM_EMULATION, EEPROM_SKIP, EEPROM_WRITE)(&ver); 

writes 8 bytes when only 4 are expected

According to mstring.h

  // Use &mystring as shorthand for mystring.str
  char* operator&() { return str; }

&ver should return the str, but it looks like it returns some structure information before it also. (tested on the simulator)

if you modify mstring.h so that str in public vs protected and use

MString<3> ver(F("ERR"));
TERN(FLASH_EEPROM_EMULATION, EEPROM_SKIP, EEPROM_WRITE)(ver.str);

It works as expected...

but as for why? I cannot see

@RV-from-be
Copy link

@thisiskeithb @thinkyhead Shouldn't we go rollback with this commit, put it back in PR and do tests on various configurations. Adding more commits to an unstable base will make it more and more complex. It also seems to me that PRs "at risk" should be tested longer. These are only suggestions.

@thisiskeithb thisiskeithb changed the title [BUG] Reboot immediately after connecting via USB [BUG] Reboot on USB/Serial Connect and EEPROM Errors Jul 1, 2023
@thisiskeithb
Copy link
Member

Bootlooping was fixed in #26037, but now temperatures are no longer reported over serial & serial devices like BTT's TFT can no longer connect.

@thisiskeithb thisiskeithb linked a pull request Jul 1, 2023 that will close this issue
@thinkyhead
Copy link
Member

It also seems to me that PRs "at risk" should be tested longer. These are only suggestions.

Much appreciated. The string refactor has been sitting in the PR queue for many months, but unfortunately we don't have a group of testers standing by other than myself who will pull down PRs and test them. I can put out the word to the Testers role on Discord that something needs testing, but that has not really proven to be effective. TBH I often don't do the most thorough job of testing due to my general workload. So it is often necessary to merge a PR before it will actually get tested enough to catch all of its issues. The bugfix-* branch will be getting renamed to dev pretty soon, so hopefully that will help to clarify that the branch is not expected to be bug-free, but is expected to have egregious bugs sometimes.

@thisiskeithb
Copy link
Member

Fixed in #26037 & 1a241e6

@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@ellensp @thinkyhead @thisiskeithb @0xC4DE @Artemonim @RV-from-be @ThomasToka and others