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.

Sign up for GitHub

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

Jump to bottom

"Fix all serial issues" pending PR #20932

Merged
merged 7 commits into from
Jan 31, 2021
Merged

"Fix all serial issues" pending PR #20932

Show file tree
Hide file tree
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 Jan 30, 2021
5d0640e
Merge branch 'bugfix-2.0.x' into fixSTM32BuildIssue
X-Ryl669 Jan 31, 2021
d53161c
Fix signedness char issue for printing
X-Ryl669 Jan 31, 2021
5ea9b43
Fix print method
X-Ryl669 Jan 31, 2021
b52ce58
Fix serial output error
X-Ryl669 Jan 31, 2021
7ad0cb7
casting to int is the correct fix here
thinkyhead Jan 31, 2021
2dbbfd2
Tweak some serial echoes
thinkyhead Jan 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Marlin/src/HAL/STM32/MarlinSerial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ void MarlinSerial::_rx_complete_irq(serial_t *obj) {
}

#if ENABLED(EMERGENCY_PARSER)
emergency_parser.update(emergency_state, c);
emergency_parser.update(static_cast<MSerialT*>(this)->emergency_state, c);
#endif
}
}
Expand Down
27 changes: 20 additions & 7 deletions Marlin/src/core/serial_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,22 @@ struct SerialBase {
FORCE_INLINE void write(const char* str) { while (*str) write(*str++); }
FORCE_INLINE void write(const uint8_t* buffer, size_t size) { while (size--) write(*buffer++); }
FORCE_INLINE void print(const char* str) { write(str); }
NO_INLINE void print(char c, int base = 0) { print((long)c, base); }
NO_INLINE void print(unsigned char c, int base = 0) { print((unsigned long)c, base); }
NO_INLINE void print(int c, int base = DEC) { print((long)c, base); }
NO_INLINE void print(unsigned int c, int base = DEC) { print((unsigned long)c, base); }
void print(long c, int base = DEC) { if (!base) write(c); write((const uint8_t*)"-", c < 0); printNumber(c < 0 ? -c : c, base); }
void print(unsigned long c, int base = DEC) { printNumber(c, base); }
void print(double c, int digits = 2) { printFloat(c, digits); }
NO_INLINE void print(char c, int base = 0) { print((long)c, base); }
NO_INLINE void print(unsigned char c, int base = 0) { print((unsigned long)c, base); }
NO_INLINE void print(int c, int base = DEC) { print((long)c, base); }
NO_INLINE void print(unsigned int c, int base = DEC) { print((unsigned long)c, base); }
void print(unsigned long c, int base = DEC) { printNumber(c, base); }
void print(double c, int digits = 2) { printFloat(c, digits); }
void print(long c, int base = DEC) {
if (!base) {
write(c);
return;
}
if (base == DEC && c < 0) {
write((uint8_t)'-'); c = -c;
Copy link
Contributor

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

Copy link
Contributor

@GMagician GMagician Jan 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

or at worst (char)'-'

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 for LONG_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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 is SERIAL_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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 -- make SERIAL_WRITE_NUMBER and SERIAL_WRITE_CHAR and get rid of SERIAL_ECHO entirely, and every call site changes to a macro that is truly explicit about what it is doing.

Copy link
Contributor Author

@X-Ryl669 X-Ryl669 Jan 31, 2021 •
edited
Loading

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.

Copy link
Member

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.

}
printNumber(c, base);
}

NO_INLINE void println(const char s[]) { print(s); println(); }
NO_INLINE void println(char c, int base = 0) { print(c, base); println(); }
Expand All @@ -98,6 +107,10 @@ struct SerialBase {

// Print a number with the given base
void printNumber(unsigned long n, const uint8_t base) {
if (!base) {
write((uint8_t)n);
return;
}
if (n) {
unsigned char buf[8 * sizeof(long)]; // Enough space for base 2
int8_t i = 0;
Expand Down
2 changes: 1 addition & 1 deletion Marlin/src/feature/bedlevel/ubl/ubl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
SERIAL_ECHO_SP(sp);
}
serial_delay(10);
Expand Down
4 changes: 2 additions & 2 deletions Marlin/src/gcode/calibrate/M48.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ void GcodeSuite::M48() {
sigma = SQRT(dev_sum / (n + 1));

if (verbose_level > 1) {
SERIAL_ECHO(n + 1);
SERIAL_ECHOPAIR(" of ", int(n_samples));
SERIAL_ECHO((int)(n + 1)); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
SERIAL_ECHOPAIR(" of ", (int)n_samples);
SERIAL_ECHOPAIR_F(": z: ", pz, 3);
SERIAL_CHAR(' ');
dev_report(verbose_level > 2, mean, sigma, min, max);
Expand Down
2 changes: 1 addition & 1 deletion Marlin/src/module/temperature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2062,7 +2062,7 @@ void Temperature::init() {
switch (heater_id) {
case H_BED: SERIAL_ECHOPGM("bed"); break;
case H_CHAMBER: SERIAL_ECHOPGM("chamber"); break;
default: SERIAL_ECHO(heater_id);
default: SERIAL_ECHO((int)heater_id); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
}
SERIAL_ECHOLNPAIR(
" ; sizeof(running_temp):", sizeof(running_temp),
Expand Down
2 changes: 1 addition & 1 deletion Marlin/src/module/tool_change.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ inline void fast_line_to_current(const AxisEnum fr_axis) { _line_to_current(fr_a
#if EXTRUDERS
inline void invalid_extruder_error(const uint8_t e) {
SERIAL_ECHO_START();
SERIAL_CHAR('T'); SERIAL_ECHO(int(e));
SERIAL_CHAR('T'); SERIAL_ECHO((int)e); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
SERIAL_CHAR(' '); SERIAL_ECHOLNPGM(STR_INVALID_EXTRUDER);
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion Marlin/src/sd/SdBaseFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ int SdBaseFile::peek() {
// print uint8_t with width 2
static void print2u(const uint8_t v) {
if (v < 10) SERIAL_CHAR('0');
SERIAL_ECHO(int(v));
SERIAL_ECHO((int)v); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
}

/**
Expand Down