Skip to content

refactor serial follow up (some reboot fix)#20938

Closed
GMagician wants to merge 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
GMagician:prevent-divide-by-0
Closed

refactor serial follow up (some reboot fix)#20938
GMagician wants to merge 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
GMagician:prevent-divide-by-0

Conversation

@GMagician
Copy link
Contributor

Fix a divide by 0 issue that may cause boards reboot

@GMagician GMagician changed the title refactor serial follow up refactor serial follow up (some reboot fix) Jan 30, 2021
@rhapsodyv
Copy link
Member

It would be good to take a look in the serial methods code before the recent changes, as before we had no issues.

@X-Ryl669
Copy link
Contributor

The problem described above was present previously but it never triggered.

In the new serial code, I've merged all Serial::print to a single place, and I've used AVR's print method as a base for this code. It seems that GMagician's compiler consider uint8_t as char so the overload of print based on char is called to print uint8_t numbers. Marlin's code is using uint8_t in some loop iterator, so, instead of printing the iterator value, it's printing the character with the iterator's value.

I'm not fond of the fix here, since it changes the behavior of the print code to pass the issue, but it'll break elsewhere where print(char) is actually expected. I have to think about a correct fix.

@X-Ryl669
Copy link
Contributor

If you don't mind, I'll use your fix in my kill all serial bugs PR here: #20932, so you can close this one.

@GMagician
Copy link
Contributor Author

If you don't mind, I'll use your fix in my kill all serial bugs PR here: #20932, so you can close this one.

It's ok to mee

@GMagician GMagician closed this Jan 31, 2021
@X-Ryl669
Copy link
Contributor

Can you try the pushed fix in #20932 to see if it works ?

@GMagician GMagician deleted the prevent-divide-by-0 branch January 31, 2021 10:16
@GMagician
Copy link
Contributor Author

GMagician commented Jan 31, 2021

Can you try the pushed fix in #20932 to see if it works ?

No it doesn't, it prints G29 T with chars and not with numbers
Edit: but solve reboot

@GMagician
Copy link
Contributor Author

GMagician commented Jan 31, 2021

@X-Ryl669 I think that Marlin needs chars as signed otherwise when doing: SERIAL_ECHO(' ') and SERIAL_ECHO(i) (with i as uint8_t) should always print a letter and that's not what should do.
Some STM32 platforms force the signed chars and I think this should be default to all, I proposed a PR for this, let's see what @thinkyhead says on that

@X-Ryl669
Copy link
Contributor

My error here. -fsigned-char is used with gcc for ARM, so yes, all platform use signed char on Marlin. Sorry for the noise.

@GMagician
Copy link
Contributor Author

Wait...there are some parts in marlin that send uint8_t and needs a single byte... Things turn out to be harder

@X-Ryl669
Copy link
Contributor

We'll continue discussion in the PR's thread, if you don't mind.

@GMagician
Copy link
Contributor Author

We'll continue discussion in the PR's thread, if you don't mind.

Yep, Better

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