-
-
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
Changes from all commits
d5ae3ca
5d0640e
d53161c
5ea9b43
b52ce58
7ad0cb7
2dbbfd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,7 +150,7 @@ | |
SERIAL_ECHO_SP(7); | ||
LOOP_L_N(i, GRID_MAX_POINTS_X) { | ||
if (i < 10) SERIAL_CHAR(' '); | ||
SERIAL_ECHO(i); | ||
SERIAL_ECHO((int)i); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is for now. I'm sorry, but those are hard to find in the code. The next step is to revert the print(uint8) to actually print the number (not the ASCII value). When this is done, I'll remove the cast as it'll not be required anymore, so I need some easy to find message to find them (I don't want to search for all For this to work, all the other place where SERIAL_ECHO(uint8) is used instead of SERIAL_CHAR will have to be converted to the later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've spread the exact same message in multiple places, so if you revert one, you'll have to revert all. I'd prefer you don't revert any. |
||
SERIAL_ECHO_SP(sp); | ||
} | ||
serial_delay(10); | ||
|
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.