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

zero padding display hint diverges from core::fmt #491

Closed
Urhengulas opened this issue May 23, 2021 · 11 comments · Fixed by #503
Closed

zero padding display hint diverges from core::fmt #491

Urhengulas opened this issue May 23, 2021 · 11 comments · Fixed by #503
Labels
difficulty: medium Somewhat difficult to solve priority: medium Medium priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working

Comments

@Urhengulas
Copy link
Member

Urhengulas commented May 23, 2021

This code:

defmt::info!("{:00x}", 42);
defmt::info!("{:01x}", 42);
defmt::info!("{:02x}", 42);
defmt::info!("{:03x}", 42);
defmt::info!("{:04x}", 42);
defmt::info!("{:05x}", 42);
defmt::info!("{:06x}", 42);

results in these logs:

0 INFO  0x2a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:8
1 INFO  0x2a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:9
2 INFO  0x2a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:10
3 INFO  0x2a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:11
4 INFO  0x2a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:12
5 INFO  0x02a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:13
6 INFO  0x002a
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:14

I'd expect that a display hint :04x results in 0x002a for 42, but it currently seems to include the 0x in the length and only results in 0x2a. 06x will currently result in the desired 0x002a, but I would expect 0x000002a.

@Urhengulas
Copy link
Member Author

@derekdreery Would you like to look into this?

@Urhengulas Urhengulas added difficulty: medium Somewhat difficult to solve priority: medium Medium priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working labels May 23, 2021
@jonas-schievink
Copy link
Contributor

This matches the behavior of core::fmt, so it might be better to keep it

@Urhengulas
Copy link
Member Author

Urhengulas commented May 23, 2021

Really? That is weird, but okay with me if it is consistent 🤔

@jonas-schievink
Copy link
Contributor

Hmm, actually our output for {:06x} matches {:#06x} with core::fmt - it doesn't include the 0x unless you use #, so maybe it's fine if we change how the padding works too?

@Urhengulas Urhengulas reopened this May 23, 2021
@Urhengulas
Copy link
Member Author

Urhengulas commented May 23, 2021

Hmm, actually our output for {:06x} matches {:#06x} with core::fmt - it doesn't include the 0x unless you use #, so maybe it's fine if we change how the padding works too?

That is also what I just tested and was about to write 😅

This code

println!("{:02X}", 42);
println!("{:#02X}", 42);
println!("{:03X}", 42);
println!("{:#03X}", 42);
println!("{:04X}", 42);
println!("{:#04X}", 42);

results in

2A
0x2A
02A
0x2A
002A
0x2A

I think consistency is helpful to help people not getting confused, about display hints. I'd rather vote for matching the behavior, and explude the 0x.

@Urhengulas Urhengulas changed the title bug in zero padding display hint zero padding display hint diverges from core::fmt May 23, 2021
@richard-uk1
Copy link
Contributor

richard-uk1 commented May 24, 2021 via email

@Urhengulas
Copy link
Member Author

@derekdreery: I would be happy if you go forward to adapt the output to match core::fmt! 😄

@richard-uk1
Copy link
Contributor

Hi, so there are a few differences between defmt and core::fmt:

  1. In core::fmt you have :x and :X which will use lower and upper case a-f respectively.
  2. In core::fmt, the alternative :#x rather than :x includes the 0x at the beginning, in defmt 0x is always included.

The might be others. Would you like me to change behaviour to match for 1 and 2, just 1, or something else?

@Urhengulas
Copy link
Member Author

Number one is already the case for defmt, isn't it? Thinking about it again I think including the 0x isn't that bad of an idea, therefore I think no action is required.

@richard-uk1
Copy link
Contributor

richard-uk1 commented Jun 6, 2021

@Urhengulas are you sure that having :x and :X differ to core::fmt is good? I've currently got a MAC address I'd like to format as AA:BB:11:22:33:44 but currently can't do this because I get 0xAA:0xBB:0x11:0x22:0x33:0x44. In core::fmt you can still get the 0x prefix by using the alternative formatter (:#x and :#X).

People will be familiar with how core::fmt works and making defmt work the same will reduce cognitive overhead.

@Urhengulas
Copy link
Member Author

I see. I mainly thought about logging single hex numbers, but for something like a mac-address it makes sense to be able to exclude the 0x. I then would agrre, that completely matching the core::fmt behavior makes sense 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium Somewhat difficult to solve priority: medium Medium priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants