Skip to content

Add defmt support to jiff#546

Closed
dickermoshe wants to merge 17 commits into
BurntSushi:masterfrom
dickermoshe:master
Closed

Add defmt support to jiff#546
dickermoshe wants to merge 17 commits into
BurntSushi:masterfrom
dickermoshe:master

Conversation

@dickermoshe
Copy link
Copy Markdown
Contributor

@dickermoshe dickermoshe commented May 1, 2026

Overview

This PR introduces optional defmt support for Jiff behind a defmt feature flag.

defmt is a deferred-formatting framework for resource-constrained environments (e.g. microcontrollers), where the host machine handles the actual formatting rather than the embedded target. This keeps binary size small and avoids pulling in core::fmt machinery at runtime.

This is fairly large PR which touches a bunch of files so I'll be patient. If you need any changes, I'll be here ready.

Approach

For structs and enums that previously only had a simple #[derive(Debug)], we now also derive defmt::Format. This covers the majority of public types.

For more complex types like date/time formatting, Span, and Zoned, we derive Format plainly. The drawback is that the output will be less human-readable, but this is a sacrifice worth making since defmt is mostly used for logging anyway. If someone needs human-readable output, they can still use core::fmt, which remains available even without std or alloc.

Omissions

In a few places, internal state had to be left out of the defmt output because Format is not implemented for certain std types (e.g. PathBuf). In most cases the omitted state is not relevant on embedded targets anyway. For instance, the file path to the time zone database is not meaningful on a microcontroller that doesn't have a standard operating system.

Testing

Added cargo check for various feature combinations.

@dickermoshe dickermoshe mentioned this pull request May 1, 2026
@dickermoshe
Copy link
Copy Markdown
Contributor Author

@BurntSushi I hate to bother you. Do you have any idea if/when you'll time to review this PR.

Don't you dare apologize. Thank you so much for all the hard work.

Let me know if there is any way I could help.

Copy link
Copy Markdown
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Thank you! I honestly can't tell if you did or not, but for the avoidance of doubt, could you avoid using AI for writing comments and communicating on the tracker? I'm not sure if the PR description here was generated by AI, but it feels like it was. Totally cool with it writing code, assisting you with writing comments and so on. But if you're contributing, I don't just want to be speaking to a proxy for a chat bot. It's a waste of time because I can just do that myself. (I've yet to update my contributing policy for my projects, but I plan to use uv's policy.)''

This otherwise looks like a good start. I left some feedback and a higher level comment here: #505 (comment)

defmt_features=(
"defmt"
"alloc defmt"
"std defmt"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you add two new combos here with alloc defmt serde and std defmt serde?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

There is a crate which does this very well, in case you are interested

https://crates.io/crates/cargo-hack

Comment thread src/civil/weekday.rs
impl defmt::Format for WeekdaysForward {
fn format(&self, fmt: defmt::Formatter) {
// Avoid mirroring `Cycle`'s internal state for defmt output. This public
// iterator is rarely logged directly, so the type name is sufficient.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

Comment thread src/error/mod.rs Outdated

#[cfg(feature = "defmt")]
impl defmt::Format for Error {
fn format(&self, fmt: defmt::Formatter) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you use f here to be consistent with the core::fmt::Write impls? (And similarly for any other by-hand defmt::Format impls.)

Comment thread src/error/mod.rs Outdated
impl defmt::Format for FilePathError {
fn format(&self, fmt: defmt::Formatter) {
// `std::path::PathBuf` does not implement `defmt::Format`. Since this error is std-only
// and defmt is mainly used in embedded contexts, omitting the path is fine.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you wrap this comment (and all other such comments) to 79 columns inclusive? Sorry to be a pain about this. I wish rustfmt would handle it for me but it doesn't.

Comment thread src/shared/util/array_str.rs Outdated
#[cfg(feature = "defmt")]
impl<const N: usize> defmt::Format for ArrayStr<N> {
fn format(&self, fmt: defmt::Formatter) {
defmt::write!(fmt, "{}", self.as_str())
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is using a macro here the best thing? It's a proc macro so maybe, but it's presumably going through some formatting machinery here. That's why the Display impls, for example, route through core::fmt::Display::fmt instead of write!(f, "{}", string).

With that said, these are Debug impls. So maybe it doesn't matter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand.
I believe the defmt::write! macro is the only way to do this.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe. I'm asking because I'm just not familiar with defmt.

If you look at std, and other Debug or Display impls, Jiff sometimes uses f.write_str. No macro even though it could use write!(f, "...").

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think defmt::write! macro is the right approach here. I would also make the format string "{=str}" to save a few bytes.

There's no function equivalent because the macro has to make an interned string in the .defmt section of the output file and then emit code that will send the ID of the interned string to the transport when this function is called.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I thought that might be the case. Thank you for confirming. :-)

Since this is for embedded, @dickermoshe let's go with saving as many bytes as we can here and use "{=str}". There are a lot of impls so this might add up. No need to do measurements or anything with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/zoned.rs
self.round
);
}
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why can't this impl be derived?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Also, do you know why defmt doesn't have things like Formatter::debug_struct like std has?

Copy link
Copy Markdown
Contributor Author

@dickermoshe dickermoshe May 25, 2026

Choose a reason for hiding this comment

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

Why can't this impl be derived?

The derive has some issues with lifetimes.

Details

C:\Users\Moshe\DickerSystems\jiff [master ≡]> cargo check
    Checking jiff v0.2.24 (C:\Users\Moshe\DickerSystems\jiff)
error[E0803]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
    --> src\zoned.rs:4156:38
     |
4156 | #[cfg_attr(feature = "defmt", derive(defmt::Format))]
     |                                      ^^^^^^^^^^^^^
     |
note: first, the lifetime cannot outlive the lifetime `'a` as defined here...
    --> src\zoned.rs:4157:28
     |
4157 | pub struct ZonedDifference<'a> {
     |                            ^^
note: ...so that the types are compatible
    --> src\zoned.rs:4156:38
     |
4156 | #[cfg_attr(feature = "defmt", derive(defmt::Format))]
     |                                      ^^^^^^^^^^^^^
     = note: expected `<&zoned::Zoned as Format>`
                found `<&'a zoned::Zoned as Format>`
     = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the types are compatible
    --> src\zoned.rs:4156:38
     |
4156 | #[cfg_attr(feature = "defmt", derive(defmt::Format))]
     |                                      ^^^^^^^^^^^^^
     = note: expected `<SpanRound<'static> as Format>`
                found `<SpanRound<'_> as Format>`
     = note: this error originates in the derive macro `defmt::Format` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0803`.
error: could not compile `jiff` (lib) due to 1 previous error

Also, do you know why defmt doesn't have things like Formatter::debug_struct like std has?

It does not

Comment thread Cargo.toml Outdated
[package]
name = "jiff"
version = "0.2.24" #:version
version = "0.2.24" #:version
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
version = "0.2.24" #:version
version = "0.2.24" #:version

Comment thread Cargo.toml Outdated
# feature just to forward it to Jiff. Instead, application authors can depend
# on Jiff directly and enable the `js` feature themselves.
#
#
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
#
#

Comment thread Cargo.toml

# Enables support for formatting Jiff types with `defmt`, a compact logging
# framework commonly used in embedded Rust.
defmt = ["dep:defmt"]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this feature should be added and documented in src/lib.rs as well. The docs in Cargo.toml should stay. They are meant to be a little shorter and perhaps more technical and focused on how they are defined as opposed to end user impact. The comment you have here is good for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll make an attempt at this, but I'm sure you're going to want to make changes. I'll try to do my best.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a small snippet to the docs, let me know that you think

Comment thread Cargo.toml
defmt = ["dep:defmt"]

[dependencies]
defmt = { version = "1.0.0", optional = true }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this version constraint accurate? I'd rather start with what we know works. Or else I can actually test the minimal version here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are using the so called "semver trick" to allow people still on the latest 0.x versions to use the Format type from defmt 1.0. They also claim in this article that there will never be a 2.0 of defmt and that all subsequent version of defmt will be backwards compatible.

I think the correct approach here is to use defmt = { version = "1", optional = true }

I've gathered the above from the following release blog post:

https://ferrous-systems.com/blog/defmt-1-0/#the-defmt-10-release

It would be really helpful if someone from the defmt could weigh in on this.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

1 is short hand for 1.0.0 (roughly?). In any case, I'm asking if Jiff actually works with defmt 1.0.0. or if it only works with a newer version. This is the opposite of the question you answered I think?

Copy link
Copy Markdown
Contributor Author

@dickermoshe dickermoshe May 25, 2026

Choose a reason for hiding this comment

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

1 is short hand for 1.0.0 (roughly?)

I did not know that. I always expected 1.0.0 to mean >1.0.0 <1.1.0.
We could leave it as is.

In any case, I'm asking if Jiff actually works with defmt 1.0.0. or if it only works with a newer version.

It will work with every version 1.x.x of defmt and the latest version of defmt 0.3.x

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

defmt 1.1.0 has no important changes from an "implementing defmt::Format" point of view, so ^1.0.0 (or equivalently ^1) is fine. defmt is a library where you can only have one version in the tree, so the less specific you can be, the better.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Beautiful. Love to hear that.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's just keep it at version = "1.0.0" here.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 25, 2026

Thank you! I honestly can't tell if you did or not, but for the avoidance of doubt, could you avoid using AI for writing comments and communicating on the tracker? I'm not sure if the PR description here was generated by AI, but it feels like it was. Totally cool with it writing code, assisting you with writing comments and so on. But if you're contributing, I don't just want to be speaking to a proxy for a chat bot. It's a waste of time because I can just do that myself. (I've yet to update my contributing policy for my projects, but I plan to use uv's policy.)''

I'm sorry if it came off that way.
Admittedly, I did use a drop of AI to help me start writing the PR description, but I never got rid of those stupid headers, so it reads very slop-like. However, none of the code was done with AI. I wanted to make sure that only what needed to be changed was changed.

Thank you so much for the review.

@BurntSushi
Copy link
Copy Markdown
Owner

Please don't mark things as resolved. Can you unresolve them? Reviewers should mark things as resolved.

@BurntSushi
Copy link
Copy Markdown
Owner

I'm totally fine with coding via AI. I just don't want to be talking with one when I expect to be speaking with a human. I've added a policy here: #565

@dickermoshe
Copy link
Copy Markdown
Contributor Author

Please don't mark things as resolved. Can you unresolve them?

I apologize.

@BurntSushi
Copy link
Copy Markdown
Owner

Thank you! No worries at all and no apologies needed. It's just nice to have them unresolved so that I can mark them resolved. :-)

@BurntSushi
Copy link
Copy Markdown
Owner

Is there any advice on what trait impls should be added more generally? Like, is it to add a defmt::Format impl for anything that impls Debug and is also in the public API? (And I'd expect this to apply recursively, i.e., add a defmt::Format impl for anything internal that is needed to provide a defmt::Format impl on something exported. Or is it recommended to start more conservative and just hit the "main types" of the crate? I could see the latter being defensible in embedded environments where maybe binary size matters. Jiff has a sprawling public API, so adding lots of defmt::Format impls I imagine will bloat things?

It might be wise to start here with impls on just the core types. Like Zoned, Timestamp and the datetime types in jiff::civil. And probably tz::TimeZone. Maybe a select few others. But just kind of throwing everything against the wall here is not the conservative way to go. We should add what is actually needed for folks to get useful defmt support out of Jiff. Then we can expand as we go.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 27, 2026

For any type which simply #derive(Debug), I think we can stick with #derive(defmt::Format).
The logging behavior is identical. I do not see any utility in trying to limit the amount of information we are logging just because we are on an embedded target. I think the defmt already does a very good job of reducing the runtime cost and binary size, I'm not sure we need to optimize for it. (I would love to have their input on this though @jonathanpallant )

For types which use a custom impl Debug, I'll have AI crank out some examples of what the output would be. It would be very helpful to judge this with some real-world examples.

It might be wise to start here with impls on just the core types

If these types aren't ever logged by defmt then it won't be compiled into the final binary. I'm only nervous about cluttering the shell with useless internal state. That will make it harder for a developer to reason about what in the world is going on.

@BurntSushi
Copy link
Copy Markdown
Owner

Examples would help yeah. Especially on the core types. Thank you.

I'm not yet convinced that unused format impls won't impact compile times though. It still has to go through proc macro expansion right?

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 27, 2026

Here are all the types which jiff uses a custom Debug impl and where are just sharting out everything:

[INFO ] Date: Date { year: 2026, month: 5, day: 27 } (test_defmt src/bin/main.rs:65)
[INFO ] Time: Time { hour: 9, minute: 41, second: 15, subsec_nanosecond: 123456789 } (test_defmt src/bin/main.rs:66)
[INFO ] DateTime: DateTime { date: Date { year: 2026, month: 5, day: 27 }, time: Time { hour: 9, minute: 41, second: 15, subsec_nanosecond: 123456789 } } (test_defmt src/bin/main.rs:67)
[INFO ] Timestamp: Timestamp { dur: 1779888075s 123456789ns } (test_defmt src/bin/main.rs:68)
[INFO ] SignedDuration: -86399s 987654321ns (test_defmt src/bin/main.rs:69)
[INFO ] whole-second SignedDuration: 12s (test_defmt src/bin/main.rs:70)
[INFO ] Span clock-only: Span { sign: Positive, units: {"h", "m", "s"}, years: 0, months: 0, weeks: 0, days: 0, hours: 5, minutes: 6, seconds: 7, milliseconds: 0, microseconds: 0, nanoseconds: 0 } (test_defmt src/bin/main.rs:71)
[INFO ] Span calendar-and-clock: Span { sign: Positive, units: {"y", "mo", "w", "d", "h", "m", "s"}, years: 2, months: 3, weeks: 1, days: 4, hours: 5, minutes: 6, seconds: 7, milliseconds: 0, microseconds: 0, nanoseconds: 0 } (test_defmt src/bin/main.rs:72)
[INFO ] negative Span: Span { sign: Negative, units: {"h", "m", "s"}, years: 0, months: 0, weeks: 0, days: 0, hours: 5, minutes: 6, seconds: 7, milliseconds: 0, microseconds: 0, nanoseconds: 0 } (test_defmt src/bin/main.rs:73)
[INFO ] Offset: -04:00:00 (test_defmt src/bin/main.rs:74)
[INFO ] precise Offset: 05:45:00 (test_defmt src/bin/main.rs:75)
[INFO ] TimeZone fixed: TimeZone { repr: Fixed("-"04:00:00) } (test_defmt src/bin/main.rs:76)
[INFO ] TimeZone unknown: TimeZone { repr: Etc/Unknown } (test_defmt src/bin/main.rs:77)
[INFO ] TimeZone POSIX: TimeZone { repr: Posix(PosixTimeZone { inner: PosixTimeZone { std_abbrev: "EST", std_offset: PosixOffset { second: -18000 }, dst: Some(PosixDst { abbrev: "EDT", offset: PosixOffset { second: -14400 }, rule: PosixRule { start: PosixDayTime { date: WeekdayOfMonth { month: 3, week: 2, weekday: 0 }, time: PosixTime { second: 7200 } }, end: PosixDayTime { date: WeekdayOfMonth { month: 11, week: 1, weekday: 0 }, time: PosixTime { second: 7200 } } } }) } }) } (test_defmt src/bin/main.rs:78)
[INFO ] TimeZoneDatabase: TimeZoneDatabase (test_defmt src/bin/main.rs:79)
[INFO ] Zoned: Zoned { inner: ZonedInner { timestamp: Timestamp { dur: 1779888075s 123456789ns }, datetime: DateTime { date: Date { year: 2026, month: 5, day: 27 }, time: Time { hour: 9, minute: 21, second: 15, subsec_nanosecond: 123456789 } }, offset: "-"04:00:00, time_zone: TimeZone { repr: Fixed("-"04:00:00) } } } (test_defmt src/bin/main.rs:80)
[INFO ] Error: Error { kind: Bounds(Month(RawBoundsError { what: month, min: 1, max: 12 })), cause: None } (test_defmt src/bin/main.rs:83)

The remaining types have defmt::Format match core::fmt::Debug exactly.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 27, 2026

I think we need to make two changes:

  1. units should not be in Debug/Format of Span. UnitSet should not be included in Debug/Format. This also fixes Stack Overflow when debug formating UnitSet #569
    In fact, a comment says as much:

    jiff/src/span.rs

    Lines 5616 to 5622 in a6870c4

    // N.B. This `Debug` impl isn't typically used.
    //
    // This is because the `Debug` impl for `Span` just emits itself in the
    // friendly duration format, which doesn't include internal representation
    // details like this set. It is included in `Span::debug`, but this isn't
    // part of the public crate API.
    impl core::fmt::Debug for UnitSet {
  2. Logging a Zoned with a Posix time zone will log an absolute mess.
Zoned with Julian POSIX zone after transition: Zoned { inner: ZonedInner { timestamp: Timestamp { dur: 1710000000s }, datetime: DateTime { date: Date { year: 2024, month: 3, day: 9 }, time: Time { hour: 13, minute: 45, second: 0, subsec_nanosecond: 0 } }, offset: "-"02:15:00, time_zone: TimeZone { repr: Posix(PosixTimeZone { inner: PosixTimeZone { std_abbrev: "XST", std_offset: PosixOffset { second: -12600 }, dst: Some(PosixDst { abbrev: "XDT", offset: PosixOffset { second: -8100 }, rule: PosixRule { start: PosixDayTime { date: JulianOne(60), time: PosixTime { second: 5025 } }, end: PosixDayTime { date: JulianZero(304), time: PosixTime { second: 86399 } } } }) } }) } } }

We can shorten this by logging timezone here as a string ( "Etc/Unknown", etc. ).
The logic for this is already present in the printer. It would have to be extracted.

@BurntSushi
Copy link
Copy Markdown
Owner

Thank you for those examples. That shows a number of problems actually. The Debug impls for, e.g., Zoned are not derived. So we really shouldn't be deriving defmt::Format either. For example:

jiff/src/zoned.rs

Lines 3397 to 3401 in 943a73f

impl core::fmt::Debug for Zoned {
fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
core::fmt::Display::fmt(self, f)
}
}

Now you won't be able to use <Zoned as Display>::fmt directly, so you'll need to copy the implementation and adapt it for defmt::Format:

jiff/src/zoned.rs

Lines 3455 to 3462 in 943a73f

use crate::fmt::StdFmtWrite;
let precision =
f.precision().map(|p| u8::try_from(p).unwrap_or(u8::MAX));
temporal::DateTimePrinter::new()
.precision(precision)
.print_zoned(self, StdFmtWrite(f))
.map_err(|_| core::fmt::Error)

Copying is probably fine. This is simple enough that DRYing it up isn't critical unless it's super easy to do.

This should be applied basically anywhere there is a custom Debug impl. Those exist for basically all of the core Jiff primitive types. That should in turn let you delete some defmt::Format impls on internal types, as they will no longer be used. e.g., for ZonedInner among what is likely many others.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 31, 2026

Got busy on other things for a couple days, sorry about that.

The code for the Display for Zoned is quite large and spread across a whole bunch of types that use fmt which is something that we are trying to avoid for embedded targets. Copying out all that code would leave to an explosion of code.

I think deriving Format on Zoned is ok, as long as we keep the time zone short.

What do you think about logging it like this?

Zoned { 
 timestamp: Timestamp { dur: 1710000000s }, 
 datetime: DateTime { date: Date { year: 2024, month: 3, day: 9 }, time: Time { hour: 13, minute: 45, second: 0, subsec_nanosecond: 0 } }, 
 offset: "-"02:15:00, 
 time_zone: "America/New_York"
 }

Making Format match Debug would be a much larger change.

This should be applied basically anywhere there is a custom Debug impl. Those exist for basically all of the core Jiff primitive types. That should in turn let you delete some defmt::Format impls on internal types, as they will no longer be used. e.g., for ZonedInner among what is likely many others.

Good point; There should be some internal Timezone related types that we can prob remove the Format derives from.

@BurntSushi
Copy link
Copy Markdown
Owner

I think that output is pretty horrible IMO. Not a good user experience.

I hear the concern about code bloat though. I agree. It pulls in a bunch of jiff::fmt shenanigans. One thing I've been toying with is just inlining exactly the code we need to print the format we want. It will be a little code duplication, but it should be a lot smaller than what's in jiff::fmt. What do you think about giving that a whirl for defmt::Format impls here? (I might then end up re-using them for Display and Debug too, but that we can punt on for now.)

@dickermoshe
Copy link
Copy Markdown
Contributor Author

dickermoshe commented May 31, 2026

I'll take a look at this.
Much of this gets into the complexities of how datetimes and time zones are represented which I am much less familiar with.

Do you only want this done for Zoned?
Zoned is particularly ugly, but the output for Date is also pretty ugly. Should we do that there too?

If so, I fear we are going down a pretty deep rabbit hole, one that duplicates two identical code paths for the same logic, all with one ending in format! and another in defmt:write!

@BurntSushi
Copy link
Copy Markdown
Owner

@dickermoshe I have to ask, are you using AI to write comments? I agree this is a rabbit hole, but it's an important one. And I already explicitly addressed the DRY violation. It just kind of feels like we are repeating points here.

I do appreciate your work here. It's okay to drop this. Like I said, I consider this critical functionality but I have high standards for landing changes. If no one else does it, I will eventually get to it.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

I have to ask, are you using AI to write comments

No, I swear I'm not. I wish there was some way to prove this.

It just kind of feels like we are repeating points here.

I thought that you only wanted this done for Zoned and was asking if you want this done for all the custom types.
I'm re-reading your previous massages and I'm realizing now that you mentioned that already.

This should be applied basically anywhere there is a custom Debug impl

I started with the assumption that Format would not have to match Debug exactly because it would involve to much work. However, at this point, I'm already invested in this codebase and would love to get this finished.

I'm gonna see if there is a way to accomplish this without doing a massive amount of changes. I do not feel comfortable contributing code I do not understand.

Again, I'm really sorry that this started off with AI communication, because now everything seems like it. I promise I'm not using it for using it. I cannot stress that enough.

@BurntSushi
Copy link
Copy Markdown
Owner

Thanks I appreciate it. I believe you. :-)

I think the key here is just implementing this for the main datetime types. Zoned. Timestamp. Civil types. Time zone. Hand write the impls, DRY be damned. That way you avoid adding a flood of impls on internal types.

If you aren't sure the right way to hand write the impls, start with jiff::fmt. We can land that for now. Otherwise, take a crack and I can help guide you on how to fix them if there is a problem.

@dickermoshe
Copy link
Copy Markdown
Contributor Author

DRY be damned

I would normally agree, but testing this defmt code either requires plugging in a microcontroller or setting up some QEMU contraption. I'm not sure how you plan on testing in CI, but I think it would simplify things greatly if most of the code that was defmt was in the normal path as well.


There are 7 types which require this kind of attention, I'll first try to see if we can shoehorn defmt into the current crate::fmt module. I'll get back to you

@BurntSushi
Copy link
Copy Markdown
Owner

Yeah there might be an internal abstraction we can use to mitigate DRY. Probably a new trait that abstracts over std and defmt formatting? Or hell, even a macro. I would need to look more closely.

If we ultimately just need to rely on user testing then that is fine. I would take that over having vomitted debug output. :-)

@dickermoshe
Copy link
Copy Markdown
Contributor Author

Right now, the formatting code uses a different buffer type depending on whether alloc is enabled. On embedded targets, alloc often is not available. And even when it is available, it is usually better to avoid heap allocation when we can.

So for these defmt trait shenanigans I’m about to try, would it be okay if we always use fixed-size buffer arrays instead of Vec, even when Vec is available?

@BurntSushi
Copy link
Copy Markdown
Owner

Yes. Absolutely. There are fixed bounds on the length of the strings produced by these impls. All of them are reasonably small.

Definitely do no want any allocs here. No bueno!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants