"Fix all serial issues" pending PR #20932

X-Ryl669 · 2021-01-30T17:38:32Z

Description

Fix a building issue with STM32 platform and emergency parser as reported by @DrumClock

Requirements

STM32 board with emergency parser enabled

Benefits

Fix build, so it does not break anymore

Configurations

I don't have those. Please have a look here

Related Issues

None created.

rhapsodyv · 2021-01-30T18:16:32Z

Maybe you should add all fixes for serial here, as a follow-up for the recent changes. What you think?

Seems that multi serial, emergency parser and auto report are having issues with the last changes...

X-Ryl669 · 2021-01-30T18:28:35Z

Ok, let's do that!

CRCinAU · 2021-01-31T02:49:58Z

Agreed - it seems there's a lot broken right now, and the problem is there's various fixes scattered everywhere making it really difficult to track what's going on...

GMagician · 2021-01-31T11:32:37Z

@X-Ryl669 you may go on with this fix. I posted #20945 to fix invalid uint8_t printed as char since I noted that mostly Marlin cast them to int when it wants a number. Unfortunatelly G29 T was not fixed and double unlucky SAMD51 uses signed chars.

In code there are some parts that need uint8_t sent as real ascii value (not number) then I think we solved all issues

X-Ryl669 · 2021-01-31T12:56:33Z

I tend to follow your opinion here. The majority of platforms previously had "print(unsigned char, base = 0)" and not "print(unsigned char, base = DEC)". So if some code use print((uint8_t)i) to actually mean formatting the number, then this code should be fixed.

It also means that the bug was present previously (on AVR, the only platform that used print(uint8_t,DEC)) but it was not reported.

GMagician · 2021-01-31T13:11:20Z

I tend to follow your opinion here. The majority of platforms previously had "print(unsigned char, base = 0)" and not "print(unsigned afixed.
It also means that the bug was present previously (on AVR, the only platform that used print(uint8_t,DEC)) but it was not reported.

Also samd is as avr,weird no one found that previously. I fixed that anyway

CRCinAU · 2021-01-31T13:12:38Z

Likely related with possible fix posted:
#20946

GMagician · 2021-01-31T13:15:34Z

Likely related with possible fix posted:
#20946

That seems something different

logiclrd · 2021-01-31T13:17:04Z

@GMagician It is different, but this PR is sneaking in a fix for that issue at the same time.

logiclrd · 2021-01-31T13:22:25Z

rhapsodyv wrote:

Maybe you should add all fixes for serial here, as a follow-up for the recent changes. What you think?

I'm assuming that's what that's about.

As a curious bystander, I'd like to propose de-obfuscating that overload of print. From where I'm standing, it doesn't seem to actually improve the performance or behaviour of the code at all, and it significantly decreases readability and maintainability. All the other overloads are one-liners, sure, but that overload is actually doing something. I think that one in particular ought to be returned to the pre-refactor form:

  void print(long n, int base = DEC)
  {
    if (base == 0) write(n);
    else if (base == DEC) {
      if (n < 0) { print('-'); n = -n; }
      printNumber(n, DEC);
    }
    else
      printNumber(n, base);
  }

Note that there is also another behavioural difference, in that the refactored form will now emit negative signs for bases other than 10 as well.

X-Ryl669 · 2021-01-31T16:57:23Z

Ok, this should be fixed. Thanks @logiclrd for spotting a bug.

GMagician · 2021-01-31T16:58:50Z

Marlin/src/core/serial_base.h

+      return;
+    }
+    if (base == DEC && c < 0) {
+      write((uint8_t)'-'); c = -c;


write('-') ;is more than enough

or at worst (char)'-'

Yep probably but on x86, it'll trigger a "conversion from signed to unsigned" warning if not cast.

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.

This only happens for platform without 2-complement representation for negative number. I think all platform are 2-complement so it's ok.

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.

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.

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.

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.

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.

X-Ryl669 · 2021-01-31T17:03:25Z

@logiclrd I've removed all the (different) print implementations for all platforms to have a single one. As strange as it seems, the different implementation where not behaving the same (as @GMagician spotted for printing unsigned char, that's not the same for AVR and ARM). So I had to made choices and honestly I've missed some code or over-optimized some other code as your spotted, introducing my own bugs here. The patch removed close to a thousand line of code and added additional features in exchange.

In the end, I think it's better that there is a single print implementation for all Marlin, since, once the "bug tsunami" is gone, there will be no more strange bug that happens on some platform but not on other due to these difference of platform.
I'm sorry for inconvenience meanwhile.

GMagician · 2021-01-31T17:09:56Z

@logiclrd I've removed all the (different) print implementations for all platforms to have a single one. As strange as it seems, the different implementation where not behaving the same (as @GMagician spotted for printing unsigned char, that's not the same for AVR and ARM). So I had to made choices and honestly I've missed some code or over-optimized some other code as your spotted, introducing my own bugs here.

Current implementation works correctly (when merged with my PR #20945). All the misunderstanding born because of some bug in Marlin (fixed by my pr) where some uint8_t where expected to print as numbers. This was unlucky correct on AVR and SAMD51 and for some STM32 (the ones with compiler option to turn chars to signed), but as I checked into code I saw that usually uint8_t are casted to int16_t when a number is needed (exists some part where uint8_t and chars are expected to be printed as a byte, so you have to keep them with base=0).

I think this PR and mine can and should be merged as soon as possible (yours because of divide by 0 and reboot and mine to complete the fix)

GMagician · 2021-01-31T17:11:45Z

...well another solution might be that you incorporate my PR into this, even if they are really two different issues

X-Ryl669 · 2021-01-31T17:12:35Z

I'll do that. This PR is a collect all serial issue.

GMagician · 2021-01-31T17:13:53Z

I'll do that. This PR is a collect all serial issue.

Give me a shot when you do it so I properly close mine

X-Ryl669 · 2021-01-31T22:16:30Z

Due to the high quantity of issue reported in the last hours, I'm not sure it's a good idea to delay merging this code. I'll probably ask to merge this ASAP since it fixes top priority issues first, and I'll reopen another PR for the less priority version.

ellensp · 2021-01-31T22:30:15Z

If you think it ready you might want to remove the draft tag

X-Ryl669 · 2021-01-31T22:35:44Z

@CRCinAU , @rhapsodyv, @GMagician Thank you for redirecting the issues here. It's really appreciated!

X-Ryl669 · 2021-01-31T22:38:21Z

@thinkyhead Can you please merge this ASAP to stop the flood of (related) issues this is solving ?

X-Ryl669 · 2021-01-31T23:02:48Z

Marlin/src/feature/bedlevel/ubl/ubl.cpp

@@ -150,7 +150,7 @@
    SERIAL_ECHO_SP(7);
    LOOP_L_N(i, GRID_MAX_POINTS_X) {
      if (i < 10) SERIAL_CHAR(' ');
-      SERIAL_ECHO((int)i); //PLEASE LEAVE THIS COMMENT, THIS CODE IS HARD TO FIND, I'LL REVERT IT LATER ON
+      SERIAL_ECHO((int)i);


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.

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.

thinkyhead · 2021-01-31T23:19:30Z

If later you want to deal with the issue of integer casting of 8-bit values to make serial printing happy, you can use this regular expression to search the code and find all the instances:

(SERIAL|DEBUG)_[A-Z_]+\([^)\n]+int

X-Ryl669 · 2021-01-31T23:22:52Z

Thanks!

X-Ryl669 · 2021-02-01T14:43:55Z

@logiclrd I've tested your version of printNumber. Unfortunately, it does not work with unsigned value higher than 2³¹ (it prints a negative value in that case). I'll probably replace with this that seems to work with both sign and it's easier to understand IMHO.

This works because when computing -n in a 2 complement system, the CPU inverts the bits and add 1. So -0x8000 gives 0x7FFF + 1 => 0x8000 which is exactly the good value when understood as unsigned. I don't know if it was thought this way when invented, but in our case, it really help simplifying the code.

logiclrd · 2021-02-01T15:31:53Z

Huh, okay, yeah, I thought basically that decimal numbers just always needed to be treated as signed, but if the function needs to handle both signed and unsigned numbers, then it isn't possible to do that with a single overload. Having both signed and unsigned overloads makes the most sense, and it is definitely convenient that the specific case where n = -n doesn't actually work still does the right thing with this design. I might suggest adding a comment calling out that behaviour for future maintainers. Thanks for looking into this more deeply :-)

… into bugfix-2.0.x * 'bugfix-2.0.x' of https://github.com/MarlinFirmware/Marlin: (121 commits) [cron] Bump distribution date (2021-02-04) [cron] Bump distribution date (2021-02-03) Add "more" menu in LVGL interface (MarlinFirmware#20940) Evaluate ANY_SERIAL_IS in place Note (MarlinUI) limit on PREHEAT settings (MarlinFirmware#20966) Update a UBL comment (MarlinFirmware#20931) STM32 Shared Media - USB Mass Storage Device (MarlinFirmware#20956) Multi-language pertains to Color UI (MarlinFirmware#20972) Touch Calibration Screen auto-save option (MarlinFirmware#20971) Include ui_common for MARLIN_LOGO_FULL_SIZE (MarlinFirmware#20963) Fix host_response_handler compile (MarlinFirmware#20962) [cron] Bump distribution date (2021-02-02) LVGL UI G-code console (MarlinFirmware#20755) [cron] Bump distribution date (2021-02-01) Refresh screen on M22 (detach) (MarlinFirmware#20958) Fix AutoReporter implementation (MarlinFirmware#20959) Serial refactor followup (MarlinFirmware#20932) Init serial ports first (MarlinFirmware#20944) Remove extra G29 V newlines (MarlinFirmware#20955) [cron] Bump distribution date (2021-01-31) ...

Fix a build issue reported by DrumClock

d5ae3ca

X-Ryl669 mentioned this pull request Jan 31, 2021

refactor serial follow up (some reboot fix) #20938

Closed

X-Ryl669 added 2 commits January 31, 2021 10:41

Merge branch 'bugfix-2.0.x' into fixSTM32BuildIssue

5d0640e

Fix signedness char issue for printing

d53161c

GMagician mentioned this pull request Jan 31, 2021

declare emergency_state variable to fix compile issues on SKR Pro #20951

Closed

X-Ryl669 mentioned this pull request Jan 31, 2021

[BUG] ADVANCED_OK returns additional info #20946

Closed

Fix print method

5ea9b43

GMagician reviewed Jan 31, 2021

View reviewed changes

X-Ryl669 changed the title ~~Fix a build issue reported by DrumClock~~ Collective "fix all serial issue" PR Jan 31, 2021

X-Ryl669 marked this pull request as draft January 31, 2021 17:46

X-Ryl669 changed the title ~~Collective "fix all serial issue" PR~~ "Fix all serial issues" pending PR Jan 31, 2021

GMagician mentioned this pull request Jan 31, 2021

Fix some number printed as chars #20945

Closed

This was linked to issues Jan 31, 2021

[BUG] ADVANCED_OK returns additional info #20946

Closed

[BUG] Latest bugfix commit breaks emergency parser #20906

Closed

rhapsodyv linked an issue Jan 31, 2021 that may be closed by this pull request

[BUG] Temperature no longer reported to OctoPrint #20921

Closed

CRCinAU mentioned this pull request Jan 31, 2021

OctoPrint unable to connect to printer when plug-in is enabled scottmudge/OctoPrint-MeatPack#12

Open

Fix serial output error

b52ce58

X-Ryl669 marked this pull request as ready for review January 31, 2021 22:37

ellensp added the S: Please Merge label Jan 31, 2021

rhapsodyv linked an issue Jan 31, 2021 that may be closed by this pull request

[Possible BUG] Serial comunication closes/drops #20957

Closed

casting to int is the correct fix here

7ad0cb7

X-Ryl669 commented Jan 31, 2021

View reviewed changes

Tweak some serial echoes

2dbbfd2

thinkyhead merged commit 2736619 into MarlinFirmware:bugfix-2.0.x Jan 31, 2021

Jyers pushed a commit to Jyers/Marlin that referenced this pull request Feb 3, 2021

Serial refactor followup (MarlinFirmware#20932)

5ee675b

rhapsodyv mentioned this pull request Feb 7, 2021

[BUG] Random freezes during printing over serial connection (skr 1.4 turbo) #21010

Open

kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021

Serial refactor followup (MarlinFirmware#20932)

4854da4

zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021

Serial refactor followup (MarlinFirmware#20932)

6ef27ea

W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021

Serial refactor followup (MarlinFirmware#20932)

610b197

thinkyhead pushed a commit that referenced this pull request Apr 30, 2021

Serial refactor followup (#20932)

9ae204d

"Fix all serial issues" pending PR #20932

"Fix all serial issues" pending PR #20932

Conversation

X-Ryl669 commented Jan 30, 2021

Description

Requirements

Benefits

Configurations

Related Issues

rhapsodyv commented Jan 30, 2021 • edited Loading

X-Ryl669 commented Jan 30, 2021

CRCinAU commented Jan 31, 2021

GMagician commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

GMagician commented Jan 31, 2021 • edited Loading

CRCinAU commented Jan 31, 2021

GMagician commented Jan 31, 2021

logiclrd commented Jan 31, 2021

logiclrd commented Jan 31, 2021 • edited Loading

X-Ryl669 commented Jan 31, 2021

Choose a reason for hiding this comment

GMagician Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X-Ryl669 Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X-Ryl669 commented Jan 31, 2021 • edited Loading

GMagician commented Jan 31, 2021

GMagician commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

GMagician commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

ellensp commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkyhead commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

X-Ryl669 commented Feb 1, 2021 • edited Loading

logiclrd commented Feb 1, 2021

rhapsodyv commented Jan 30, 2021 •

edited

Loading

GMagician commented Jan 31, 2021 •

edited

Loading

logiclrd commented Jan 31, 2021 •

edited

Loading

GMagician Jan 31, 2021 •

edited

Loading

X-Ryl669 Jan 31, 2021 •

edited

Loading

X-Ryl669 commented Jan 31, 2021 •

edited

Loading

X-Ryl669 commented Feb 1, 2021 •

edited

Loading