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 all 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
4 changes: 2 additions & 2 deletions Marlin/src/MarlinCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,8 +614,8 @@ inline void manage_inactivity(const bool ignore_stepper_queue=false) {
*/
void idle(TERN_(ADVANCED_PAUSE_FEATURE, bool no_stepper_sleep/*=false*/)) {
#if ENABLED(MARLIN_DEV_MODE)
static uint8_t idle_depth = 0;
if (++idle_depth > 5) SERIAL_ECHOLNPAIR("idle() call depth: ", int(idle_depth));
static uint16_t idle_depth = 0;
if (++idle_depth > 5) SERIAL_ECHOLNPAIR("idle() call depth: ", idle_depth);
#endif

// Core Marlin activities
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 (int) cast in the code.

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.
When this is done, I'll have a clearer usage of the right serial macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
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));
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
4 changes: 2 additions & 2 deletions Marlin/src/gcode/host/M115.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
static void cap_line(PGM_P const name, bool ena=false) {
SERIAL_ECHOPGM("Cap:");
serialprintPGM(name);
SERIAL_CHAR(':');
SERIAL_ECHOLN(int(ena ? 1 : 0));
SERIAL_CHAR(':', ena ? '1' : '0');
SERIAL_EOL();
}
#endif

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);
}
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);
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);
}

/**
Expand Down