Add defmt support to jiff#546
Conversation
|
@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. |
BurntSushi
left a comment
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Could you add two new combos here with alloc defmt serde and std defmt serde?
There was a problem hiding this comment.
Done.
There is a crate which does this very well, in case you are interested
| 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. |
|
|
||
| #[cfg(feature = "defmt")] | ||
| impl defmt::Format for Error { | ||
| fn format(&self, fmt: defmt::Formatter) { |
There was a problem hiding this comment.
Can you use f here to be consistent with the core::fmt::Write impls? (And similarly for any other by-hand defmt::Format impls.)
| 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. |
There was a problem hiding this comment.
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.
| #[cfg(feature = "defmt")] | ||
| impl<const N: usize> defmt::Format for ArrayStr<N> { | ||
| fn format(&self, fmt: defmt::Formatter) { | ||
| defmt::write!(fmt, "{}", self.as_str()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure I understand.
I believe the defmt::write! macro is the only way to do this.
There was a problem hiding this comment.
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, "...").
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| self.round | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why can't this impl be derived?
There was a problem hiding this comment.
Also, do you know why defmt doesn't have things like Formatter::debug_struct like std has?
There was a problem hiding this comment.
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
| [package] | ||
| name = "jiff" | ||
| version = "0.2.24" #:version | ||
| version = "0.2.24" #:version |
There was a problem hiding this comment.
| version = "0.2.24" #:version | |
| version = "0.2.24" #:version |
| # feature just to forward it to Jiff. Instead, application authors can depend | ||
| # on Jiff directly and enable the `js` feature themselves. | ||
| # | ||
| # |
|
|
||
| # Enables support for formatting Jiff types with `defmt`, a compact logging | ||
| # framework commonly used in embedded Rust. | ||
| defmt = ["dep:defmt"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a small snippet to the docs, let me know that you think
| defmt = ["dep:defmt"] | ||
|
|
||
| [dependencies] | ||
| defmt = { version = "1.0.0", optional = true } |
There was a problem hiding this comment.
Is this version constraint accurate? I'd rather start with what we know works. Or else I can actually test the minimal version here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Beautiful. Love to hear that.
There was a problem hiding this comment.
Let's just keep it at version = "1.0.0" here.
Co-authored-by: Andrew Gallant <jamslam@gmail.com>
Co-authored-by: Andrew Gallant <jamslam@gmail.com>
I'm sorry if it came off that way. Thank you so much for the review. |
|
Please don't mark things as resolved. Can you unresolve them? Reviewers should mark things as resolved. |
|
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 |
I apologize. |
|
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. :-) |
|
Is there any advice on what trait impls should be added more generally? Like, is it to add a It might be wise to start here with impls on just the core types. Like |
|
For any type which simply For types which use a custom
If these types aren't ever logged by |
|
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? |
|
Here are all the types which The remaining types have |
|
I think we need to make two changes:
We can shorten this by logging timezone here as a string ( "Etc/Unknown", etc. ). |
|
Thank you for those examples. That shows a number of problems actually. The Lines 3397 to 3401 in 943a73f Now you won't be able to use Lines 3455 to 3462 in 943a73f 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 |
|
Got busy on other things for a couple days, sorry about that. The code for the I think deriving What do you think about logging it like this? Making
Good point; There should be some internal Timezone related types that we can prob remove the |
|
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 |
|
I'll take a look at this. Do you only want this done for 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 |
|
@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. |
No, I swear I'm not. I wish there was some way to prove this.
I thought that you only wanted this done for
I started with the assumption that 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. |
|
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. |
I would normally agree, but testing this There are 7 types which require this kind of attention, I'll first try to see if we can shoehorn |
|
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. :-) |
|
Right now, the formatting code uses a different buffer type depending on whether So for these |
|
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! |
Overview
This PR introduces optional
defmtsupport for Jiff behind adefmtfeature flag.defmtis 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 incore::fmtmachinery 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 derivedefmt::Format. This covers the majority of public types.For more complex types like date/time formatting,
Span, andZoned, we deriveFormatplainly. The drawback is that the output will be less human-readable, but this is a sacrifice worth making sincedefmtis mostly used for logging anyway. If someone needs human-readable output, they can still usecore::fmt, which remains available even withoutstdoralloc.Omissions
In a few places, internal state had to be left out of the
defmtoutput becauseFormatis not implemented for certainstdtypes (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 checkfor various feature combinations.