-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
"Fix all serial issues" pending PR #20932
Merged
thinkyhead
merged 7 commits into
MarlinFirmware:bugfix-2.0.x
from
X-Ryl669:fixSTM32BuildIssue
Jan 31, 2021
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d5ae3ca
Fix a build issue reported by DrumClock
X-Ryl669 5d0640e
Merge branch 'bugfix-2.0.x' into fixSTM32BuildIssue
X-Ryl669 d53161c
Fix signedness char issue for printing
X-Ryl669 5ea9b43
Fix print method
X-Ryl669 b52ce58
Fix serial output error
X-Ryl669 7ad0cb7
casting to int is the correct fix here
thinkyhead 2dbbfd2
Tweak some serial echoes
thinkyhead File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write('-') ;is more than enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or at worst (char)'-'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep probably but on x86, it'll trigger a "conversion from signed to unsigned" warning if not cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, there is a latent bug in
c = -c
forLONG_MIN
which does not exist (since-LONG_MIN > LONG_MAX
) and can not be represented in a signed long... This was present in Marlin previously, so I don't know if I should fix it or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only happens for platform without 2-complement representation for negative number. I think all platform are 2-complement so it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd like to remove all special case print with base == 0, since when you do call print with base = 0, you should have called write instead (it's only for char and unsigned char). This means replacing in the code base all call to
SERIAL_ECHO(uint8)
when what was really meant isSERIAL_CHAR()
. That way, once you read in the code a SERIAL_ECHO, it always mean printing a number as decimal (or at worst, in another base), but not outputing an ASCII char.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this is done, we'll be able to simplify the print functions not to have so many overload/case. In your code, you're computing a modulo and that's expensive (although, the only base we are interested in is 10 and 16 in reality). If you compute modulo 16 and you have 2 complement number, then the modulo is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that that level of optimization is really necessary. What proportion of the available CPU cycles is actually used by formatting integers? We could make it blindingly fast with a separate path per base and careful bit arithmetic, but is that solving any problem?
I'm only a bystander, a user of the software and not a developer of it, but I am a developer professionally and can say wholeheartedly that I am 100% on board with refactoring and reworking as entirely independent steps. :-)
I might say that if different parts of the code have different interpretations of
SERIAL_ECHO
then perhaps they should all have to change -- makeSERIAL_WRITE_NUMBER
andSERIAL_WRITE_CHAR
and get rid ofSERIAL_ECHO
entirely, and every call site changes to a macro that is truly explicit about what it is doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I'm inline with your opinion. Yet, it's a huge work due to the number of SERIAL_ECHO in the code. Will do it step by step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our assumption has always been that an 8-bit number (
byte
,char
,uint8_t
, etc.) may be printed as a single character by the serial methods, so you will see many places where an 8-bit value is cast to an integer in order to get the desired numerical output.