defmt impl for Vec #172

theunkn0wn1 · 2020-08-27T04:05:24Z

This PR adds support for encoding heapless::Vec for usage with defmt.
defmt can already handle slices, so this impl simply invokes that encoder method.

I chose to use the slice implementation is I did not perceive the difference between a Vec and a Slice to be of specific importance in a log file, but rather the contents of the vec / slice to be of interest. It also appeared to me as the path of least resistance.

I am marking this as a draft as I have yet to be able to validate this against my local hardware, probe-run is giving me some grief that I am reasonably sure is unrelated to this change set.

Relates to #171

src/lib.rs

korken89 · 2020-09-18T11:36:15Z

Hi, thanks for the PR!

Since you have started this, do you feel like adding support for String as well?

theunkn0wn1 · 2020-09-18T17:48:31Z

I think that can be arranged if that is desirable.

I have largely been blocked by the probe-rs wfi bug making doing anything with RTT painful, so by extension this includes defmt.

theunkn0wn1 · 2020-09-22T19:46:01Z

@korken89
Looked a little into implementing String with defmt, and don't see a clear path forward.

In the case of Vec, I leveraged the existing slice impl because the distinction wasn't important.
In the case of a string, having heapless emit output a slice of bytes isn't as helpful as getting a string out of it.

If i understand what defmt is doing correctly, for strings they are compile-time interned and arn't actually sent over-the-wire. I suspect defmt changes may need to be implemented for runtime strings such as those produced by heapless.

At this point I offer this PR as-is. Ive done some tests working around the WFI bug and this PR appears to work correctly. Further the unit test I added is modeled after existing tests in defmt, so I have some confidence my PR works as intended. If String support is desired, I wish for that to be in its own ticket and out of scope for this pull request.

birkenfeld · 2020-09-23T05:27:18Z

If i understand what defmt is doing correctly, for strings they are compile-time interned and arn't actually sent over-the-wire.

Well that's for format strings and other constant strings. But there are also primitive types "byte slice" and "string slice" which are sent as-is (see https://defmt.ferrous-systems.com/primitives.html). I think you just need to call fmt.str() on the Formatter and that should be all.

theunkn0wn1 · 2020-09-23T05:44:22Z

If i understand what defmt is doing correctly, for strings they are compile-time interned and arn't actually sent over-the-wire.

I think you just need to call fmt.str() on the Formatter and that should be all.

Wow I completely missed that! You may be right, il do some investigating.

theunkn0wn1 · 2020-09-23T20:11:47Z

Tested both String and Vec formatting on my stm32f4 while working around wfi.

     Running `probe-run --defmt --chip STM32F446RETx target/thumbv7em-none-eabihf/debug/rust_stm32_motor`
  (HOST) INFO  flashing program
  (HOST) INFO  success!
────────────────────────────────────────────────────────────────────────────────
0.000000 WARN  hello, world!
└─ rust_stm32_motor::init @ src/main.rs:99
0.000001 DEBUG msg: hello, heapless!
└─ rust_stm32_motor::init @ src/main.rs:102
...
0.000121 DEBUG emitting [1, 7, 130, 125, 18, 132, 1, 32, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0]
└─ rust_stm32_motor::uart4_on_rxne @ src/main.rs:321
0.000122 TRACE done processing packet. clearing buffer.

Relevant code snippets, for future reference

        let mut some_string:heapless::String<consts::U32> = heapless::String::new();
        some_string.push_str(&"hello, heapless!").unwrap();
        debug!("msg: {:str}", some_string);

                    debug!("emitting {:[u8]}", buf);

korken89 · 2020-09-24T08:38:38Z

Thanks for the update!

Now we just need to await defmt to be on crates.io and this is ready for inclusion.
Maybe @japaric knows when it will be released?

korken89 · 2020-09-24T09:19:15Z

Or, it is behind a feature-flag so I see no issue in including as is.
This feature will be considered unstable until a proper release of defmt is available.

korken89 · 2020-09-24T09:19:42Z

bors merge

172: defmt impl for Vec r=korken89 a=theunkn0wn1 This PR adds support for encoding `heapless::Vec` for usage with `defmt`. `defmt` can already handle slices, so this impl simply invokes that encoder method. I chose to use the slice implementation is I did not perceive the difference between a Vec and a Slice to be of specific importance in a log file, but rather the contents of the vec / slice to be of interest. It also appeared to me as the path of least resistance. I am marking this as a draft as I have yet to be able to validate this against my local hardware, `probe-run` is giving me some grief that I am reasonably sure is unrelated to this change set. Relates to #171 Co-authored-by: joshua salzedo <thHunkn0WNd@gmail.com> Co-authored-by: Joshua Salzedo <joshuasalzedo@gmail.com>

bors · 2020-09-24T09:25:26Z

Build failed:

continuous-integration/travis-ci/push

korken89 · 2020-09-24T09:28:36Z

bors retry

172: defmt impl for Vec r=korken89 a=theunkn0wn1 This PR adds support for encoding `heapless::Vec` for usage with `defmt`. `defmt` can already handle slices, so this impl simply invokes that encoder method. I chose to use the slice implementation is I did not perceive the difference between a Vec and a Slice to be of specific importance in a log file, but rather the contents of the vec / slice to be of interest. It also appeared to me as the path of least resistance. I am marking this as a draft as I have yet to be able to validate this against my local hardware, `probe-run` is giving me some grief that I am reasonably sure is unrelated to this change set. Relates to #171 Co-authored-by: joshua salzedo <thHunkn0WNd@gmail.com> Co-authored-by: Joshua Salzedo <joshuasalzedo@gmail.com>

bors · 2020-09-24T09:32:16Z

Build failed:

continuous-integration/travis-ci/push

korken89 · 2020-09-24T09:33:39Z

Hi @theunkn0wn1, the CI seems to be failing for 1.36.
Could you have a look into why?

theunkn0wn1 · 2020-09-24T15:53:19Z

Wilco. Yes it's behind a feature flag. As ci hasn't executed when I opened the pr I'm guessing I need to add the feature flag to the ci run.

theunkn0wn1 · 2020-09-24T16:53:59Z

@korken89
Looks like defmt depends on the profile-overrides feature, which isn't stable until 1.41.
1.41 > 1.36 so that may appears to be why. Can confirm build failure on rust:1.36 but note build success on 1.41

theunkn0wn1 · 2020-10-08T04:48:36Z

@thalesfragoso commented in matrix that depending on git dependencies would make this crate, to my understanding unreleasable on crates.io. Until the time defmt gets a crates.io release, this issue will persist.

Ultimately I do not see how the CI failure can be addressed without boosting this crates MSRV to at least rust stable 1.41, or having the MSRV of defmt lowered to 1.36.

korken89 · 2020-10-08T14:51:35Z

This crate has a quite low MSRV, and it seems like it starts to be time to look it over.
I am not sure if we will do it before const generics, as that will be a breaking change in any case.

theunkn0wn1 · 2020-12-09T03:59:16Z

Update to synchronize with current master.
As a note this PR appears to be broken by knurling-rs/defmt#267, which added heapless as a dependency.

This causes this PR to fail to resolve in cargo, since the dependency becomes circular.

korken89 · 2021-02-11T12:59:44Z

bors try

fkohlgrueber · 2021-05-11T18:28:41Z

It seems like defmt support for heapless should be possible now:

defmt is released on crates.io
defmt doesn't depend on heapless anymore (see https://github.com/knurling-rs/defmt/pull/340/files)

From what I see, this PR could be integrated when changing defmt in Cargo.toml from git to version "0.2".

@theunkn0wn1 do you want to update this PR or should I open another one?

theunkn0wn1 · 2021-05-11T18:43:56Z

@fkohlgrueber
Thanks for the the update!

defmt had fallen off my radar so i wasn't aware of the changes.

Looks like defmt's interface changed, changing from git to version 0.2.1 results in build failures..

Presently investigating.

theunkn0wn1 · 2021-05-11T21:11:50Z

Fixed this PR so it works with defmt 0.2 and I have tested it against my stm32f446 hardware.
Both a Vec and a Str appear to render correctly.

fkohlgrueber · 2021-05-16T12:35:56Z

@theunkn0wn1 seems like there are conflicts that prevent integration. Can you try to resolve them?

theunkn0wn1 · 2021-05-17T19:57:01Z

@theunkn0wn1 seems like there are conflicts that prevent integration. Can you try to resolve them?

The merge conflicts ended up running quite deep and i had to do some minor refactors to match the features, namely const-generics, presently on master.

I have verified this PR still appears to work on my f446.

fkohlgrueber

Looks good to me!

fkohlgrueber · 2021-05-17T20:08:13Z

Cargo.toml

@@ -24,6 +24,7 @@ x86-sync-pool = []
 __trybuild = []
 # Enable larger MPMC sizes.
 mpmc_large = []
+defmt-impl = ["defmt"]


You could remove this line and use defmt instead of defmt-impl as the feature flag. I think that's that standard way of doing it (e.g. see smoltcp-rs/smoltcp#455).

I followed the template provided by ufmt-impl on line 20, if we change this PR we should also change the other in a follow up.

fkohlgrueber · 2021-05-17T20:10:58Z

src/lib.rs

@@ -94,6 +94,8 @@ mod de;
 mod ser;

 pub mod binary_heap;
+#[cfg(feature = "defmt-impl")]


you could use defmt instead of defmt-impl here (see my other comment).

korken89

LGTM, thanks!

birkenfeld reviewed Aug 27, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

theunkn0wn1 marked this pull request as ready for review September 22, 2020 19:46

korken89 approved these changes Sep 24, 2020

View reviewed changes

theunkn0wn1 force-pushed the feature/defmt branch from 883cd43 to 0209b3f Compare December 9, 2020 03:50

theunkn0wn1 marked this pull request as draft December 9, 2020 04:02

Lotterleben mentioned this pull request Apr 14, 2021

111 use defmt logging ferrous-systems/embedded-trainings-2020#140

Merged

3 tasks

theunkn0wn1 marked this pull request as ready for review May 11, 2021 21:16

theunkn0wn1 requested a review from korken89 May 11, 2021 21:16

attempt at reimplementing on a clean history

f4617ea

theunkn0wn1 force-pushed the feature/defmt branch from 2ab171c to f4617ea Compare May 17, 2021 19:53

fkohlgrueber approved these changes May 17, 2021

View reviewed changes

MathiasKoch approved these changes May 18, 2021

View reviewed changes

cargo fmt

63d0ed8

korken89 approved these changes May 23, 2021

View reviewed changes

korken89 merged commit 6d1a2ed into rust-embedded:master May 23, 2021

theunkn0wn1 deleted the feature/defmt branch May 23, 2021 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defmt impl for Vec #172

defmt impl for Vec #172

theunkn0wn1 commented Aug 27, 2020

korken89 commented Sep 18, 2020

theunkn0wn1 commented Sep 18, 2020

theunkn0wn1 commented Sep 22, 2020

birkenfeld commented Sep 23, 2020

theunkn0wn1 commented Sep 23, 2020

theunkn0wn1 commented Sep 23, 2020 •

edited

Loading

korken89 commented Sep 24, 2020

korken89 commented Sep 24, 2020

korken89 commented Sep 24, 2020

bors bot commented Sep 24, 2020

korken89 commented Sep 24, 2020

bors bot commented Sep 24, 2020

korken89 commented Sep 24, 2020

theunkn0wn1 commented Sep 24, 2020

theunkn0wn1 commented Sep 24, 2020

theunkn0wn1 commented Oct 8, 2020

korken89 commented Oct 8, 2020

theunkn0wn1 commented Dec 9, 2020

korken89 commented Feb 11, 2021

fkohlgrueber commented May 11, 2021

theunkn0wn1 commented May 11, 2021 •

edited

Loading

theunkn0wn1 commented May 11, 2021

fkohlgrueber commented May 16, 2021

theunkn0wn1 commented May 17, 2021

fkohlgrueber left a comment

fkohlgrueber May 17, 2021

theunkn0wn1 May 17, 2021

fkohlgrueber May 17, 2021

korken89 left a comment

defmt impl for Vec #172

defmt impl for Vec #172

Conversation

theunkn0wn1 commented Aug 27, 2020

korken89 commented Sep 18, 2020

theunkn0wn1 commented Sep 18, 2020

theunkn0wn1 commented Sep 22, 2020

birkenfeld commented Sep 23, 2020

theunkn0wn1 commented Sep 23, 2020

theunkn0wn1 commented Sep 23, 2020 • edited Loading

korken89 commented Sep 24, 2020

korken89 commented Sep 24, 2020

korken89 commented Sep 24, 2020

bors bot commented Sep 24, 2020

korken89 commented Sep 24, 2020

bors bot commented Sep 24, 2020

korken89 commented Sep 24, 2020

theunkn0wn1 commented Sep 24, 2020

theunkn0wn1 commented Sep 24, 2020

theunkn0wn1 commented Oct 8, 2020

korken89 commented Oct 8, 2020

theunkn0wn1 commented Dec 9, 2020

korken89 commented Feb 11, 2021

fkohlgrueber commented May 11, 2021

theunkn0wn1 commented May 11, 2021 • edited Loading

theunkn0wn1 commented May 11, 2021

fkohlgrueber commented May 16, 2021

theunkn0wn1 commented May 17, 2021

fkohlgrueber left a comment

Choose a reason for hiding this comment

fkohlgrueber May 17, 2021

Choose a reason for hiding this comment

theunkn0wn1 May 17, 2021

Choose a reason for hiding this comment

fkohlgrueber May 17, 2021

Choose a reason for hiding this comment

korken89 left a comment

Choose a reason for hiding this comment

theunkn0wn1 commented Sep 23, 2020 •

edited

Loading

theunkn0wn1 commented May 11, 2021 •

edited

Loading