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.

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

Added support for HEX leading zero in print #6750

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Tijgerd
Copy link

@Tijgerd Tijgerd commented Sep 21, 2017

for issue #1097

Added the leading zero for the print funcionality

if the argument PRINT_LEADINGZERO is added after the base definition, a leading zero is added to the hex

if PRINT_LEADINGZERO is in the arg, add a leading zero to a HEX if the value is smaler than 0x10
@facchinm facchinm added the Print and Stream class The Arduino core library's Print and Stream classes label Sep 22, 2017
@Tijgerd
Copy link
Author

Tijgerd commented Sep 26, 2017

@clivepengelly, @fake-name, @technikadonis, @cmaglie is this a solution for the problem?
Anyone willing to test this?

@fake-name
Copy link

fake-name commented Sep 26, 2017

I still stand that the current implementation is broken, and the correct option is to have print HEX always emit a leading zero, but that's probably not a winning opinion at this point.

Really, this seems overcomplicated. Why not just have an alternative to the HEX (maybe ACTUAL_HEX? or the less-accurate HEX_LEADINGZERO suggested in the bug report) parameter, rather then redefining a bunch of methods and changing their signatures?

@Tijgerd
Copy link
Author

Tijgerd commented Sep 26, 2017

@fake-name ,
I do support your opinion, I'm using HEX because I want it to be with leading zero..

BUT

HEX, DEC, etc. are a bunch of predefined values indicating the base of the value (e.g. HEX = 16)
This base is used as a divider for the value, so for each "letter" printed, the value is divided by the base.

adding a strange value for the base, like ACTUAL_HEX, will be a bit odd because it will be something like: 42 which is not a base value.

I added a OPTIONAL input value which defaults to PRINT_NOARG when not assigned.
if it IS assigned it checks if the given HEX value is smaller than 16, if so, it will add a leading zero.

With this solution, old scripts can still be used :)
I think that's the whole point.. they want it to be backward compatible.

@fake-name
Copy link

fake-name commented Sep 26, 2017

HEX, DEC, etc. are a bunch of predefined values indicating the base of the value (e.g. HEX = 16)
This base is used as a divider for the value, so for each "letter" printed, the value is divided by the base.

So change that to an enum?

Also, gah, what terrible API design.

@Tijgerd
Copy link
Author

Tijgerd commented Sep 26, 2017

That's what my initial plan was BUT there may be some users that did actual write the base number and so.. It won't be backward compatible.

I know, it doesnt feel right for the HEX not printing the leading zero but this was once implemented and changing it would make a small group of arduino users unhappy ;)

@fake-name
Copy link

fake-name commented Sep 26, 2017

That's what my initial plan was BUT there may be some users that did actual write the base number and so.. It won't be backward compatible.

That's not documented, and the documentation makes no statements about what HEX, DEC, etc... actually contain, so they're relying on undefined behaviour now. Their code is already broken, it just coincidentally works.

In fact the documentation explicitly states:

An optional second parameter specifies the base (format) to use; permitted values are BIN (binary, or base 2), OCT (octal, or base 8), DEC (decimal, or base 10), HEX (hexadecimal, or base 16).

So passing anything but BIN, OCT, DEC or HEX is incorrect.

@Tijgerd
Copy link
Author

Tijgerd commented Sep 26, 2017

@fake-name
Copy link

fake-name commented Sep 26, 2017

Ah, it's kind of crappily documented on https://www.arduino.cc/en/Serial/Println, the contents of which actually contradict https://www.arduino.cc/en/Serial/Print (which I'm quoting literally above).
Well, except that println() then says "has the same interface as print(). so WTF?

Siiiiiiiigh, Arduino code in a nutshell.

@Tijgerd
Copy link
Author

Tijgerd commented Sep 26, 2017

True 👍

@fake-name
Copy link

fake-name commented Sep 26, 2017

Anyways, print()'s documentation explicitly enumerates the valid values for format when val is integral, so I'm still sticking with just change the function of format. If you really want to keep existing behaviour, just place the new enum values in the top bits of format. If anyone is printing in > base 64, they're sufficiently nuts that I think we can ignore them.

It's horrible, yeah, but not any more so then the implementation it's papering over top of.

@Tijgerd
Copy link
Author

Tijgerd commented Dec 22, 2017

@facchinm any news on this?

@cousteaulecommandant
Copy link
Contributor

cousteaulecommandant commented Dec 25, 2017

Some ideas/opinions/thoughts:

  1. Arduino's print() is meant as a simple printing function rather than a fully featured formatting one, so I'm not sure adding more arguments is a good idea.
  2. For more advanced formatting specifications, I think using printf() is a better idea. See Add function "printf" to AVR core class "print" #5938 which adds support for Serial.printf().
  3. Back in the day I thought of including more fields in the format specifier, like
    #define WIDTH(x) ((x)<<8) // up to 127
    #define SIGN (1<<7) // include +/-
    #define ZERO_PAD (1<<6) // pad with zeros instead of spaces
    #define BASE(x) (x) // up to 36
    Serial.print(x, WIDTH(2) | ZERO_PAD | BASE(16));
    
    which would allow cramming multiple format specifiers on a single argument, thus being compatible with the proposal in Variadic print #5829.
  4. Maybe it is a good idea to just print UNSIGNED integers (byte, word, unsigned long) as fixed width (zero-padded), and SIGNED integers (signed char, int, long) as regular numbers with a sign (e.g. Serial.print(-15,HEX) should print -F, not FFFFFFF1; see Print negative numbers as negative numbers regardless of base #4535). This way, a simple conversion would be enough in most cases.


do {
//the argument is only valid for HEX values smaler than 0x10
if(base != 8 || n>=16) arg = 0;
Copy link

Choose a reason for hiding this comment

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

if(base != HEX || n>=16) arg = 0;

@Tijgerd
Copy link
Author

Tijgerd commented May 2, 2018

@retfie , if I remember correctly, HEX is defines as 16 (By default in the original arduino code)

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Print and Stream class The Arduino core library's Print and Stream classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants