-
-
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
"Fix all serial issues" pending PR #20932
Conversation
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... |
Ok, let's do that! |
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... |
@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 |
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 It also means that the bug was present previously (on AVR, the only platform that used |
Also samd is as avr,weird no one found that previously. I fixed that anyway |
Likely related with possible fix posted: |
That seems something different |
@GMagician It is different, but this PR is sneaking in a fix for that issue at the same time. |
rhapsodyv wrote:
I'm assuming that's what that's about. As a curious bystander, I'd like to propose de-obfuscating that overload of
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. |
Ok, this should be fixed. Thanks @logiclrd for spotting a bug. |
return; | ||
} | ||
if (base == DEC && c < 0) { | ||
write((uint8_t)'-'); c = -c; |
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
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.
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 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.
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 -- 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.
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.
@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. |
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) |
...well another solution might be that you incorporate my PR into this, even if they are really two different issues |
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 |
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. |
If you think it ready you might want to remove the draft tag |
@CRCinAU , @rhapsodyv, @GMagician Thank you for redirecting the issues here. It's really appreciated! |
@thinkyhead Can you please merge this ASAP to stop the flood of (related) issues this is solving ? |
@@ -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); |
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.
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.
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'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.
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:
|
Thanks! |
@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 |
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 |
… 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) ...
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.