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

111 use defmt logging #140

Merged
merged 72 commits into from
Jan 12, 2022
Merged

111 use defmt logging #140

merged 72 commits into from
Jan 12, 2022

Conversation

Lotterleben
Copy link
Contributor

@Lotterleben Lotterleben commented Apr 8, 2021

NOTE: I have currently abandoned this for time reasons. If you're reading this and would like to finish it, feel free to take it over.

opening as draft PR for visibility (fixes #111 when finished)

TODO

Notes:

stack_overflow example yields

└─ stack_overflow::fib @ /Users/lottesteenbrink/ferrous/embedded-trainings-2020/beginner/apps/src/bin/stack_overflow.rs:27
────────────────────────────────────────────────────────────────────────────────
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: {"package":"defmt","tag":"defmt_prim","data":"{=__internal_Display}","disambiguator":"1356451526447435407"}
error: the stack appears to be corrupted beyond this point
  (HOST) ERROR the program has overflowed its stack

according to my debugger step-through this is printed as a result of probe-run's symtab fallback: https://github.com/knurling-rs/probe-run/blob/main/src/main.rs#L769

dk::exit()
}
}

log::info!("{:?}", dict);
defmt::info!("{:?}", defmt::Debug2Format(&dict));
// ^^^^^^^^^^^^^^^^^^^ this adapter iscurrently needed to log `heapless`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't implement this for heapless due to circular dependency conundrum, see rust-embedded/heapless#172 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

heapless 0.7.1 has defmt support (behind a Cargo feature); also const generics 🎉 . we should move to that version ASAP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! From what I see, it doesn't impl Format for LinearMaps though, right? PR incoming...

Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

Found some nits :D

advanced/firmware/src/bin/vec.rs Outdated Show resolved Hide resolved
beginner/apps/Cargo.toml Outdated Show resolved Hide resolved
beginner/apps/src/bin/blinky.rs Outdated Show resolved Hide resolved
boards/dk/Cargo.toml Outdated Show resolved Hide resolved
boards/dk/Cargo.toml Show resolved Hide resolved
@Lotterleben Lotterleben requested a review from japaric June 3, 2021 14:43
@Lotterleben Lotterleben marked this pull request as ready for review June 3, 2021 14:44
Copy link
Contributor

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Looks great! I left some inline comments.

as a follow-up improvement we could implement defmt::timestamp! in board to use dk::uptime. That way the uptime (in seconds) should show up in the log messages

advanced/firmware/src/bin/stack_overflow.rs Outdated Show resolved Hide resolved
@@ -87,7 +88,9 @@ fn ep0setup(usbd: &USBD, ep0in: &mut Ep0In, state: &mut State) -> Result<(), ()>

let request = Request::parse(bmrequesttype, brequest, wvalue, windex, wlength)
.expect("Error parsing request");
log::info!("EP0: {:?}", request);
defmt::info!("EP0: {:?}", defmt::Debug2Format(&request));
Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to send a PR to usb2 to add derive(Format) to these structs. I can quickly merge that and put out a new crates.io release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note for anyone picking this up: I've done most of the work for this here, but at the time it was blocked on knurling-rs/defmt#501
because I couldn't simply derive or impl Format for it from there because it's not part of my crate. This has since been resolved with the above issue being closed

advanced/firmware/src/lib.rs Outdated Show resolved Hide resolved
beginner/apps/src/bin/radio-recv.rs Outdated Show resolved Hide resolved
beginner/apps/src/lib.rs Outdated Show resolved Hide resolved
let mut dict = LinearMap::<_, _, consts::U2>::new();
// ^^ capacity; this is a type not a value
let mut dict = LinearMap::<_, _, 2>::new();
// content types ^^ ^^ ^ capacity
Copy link
Contributor

Choose a reason for hiding this comment

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

these markers (in the comment) no longer match the preceding line of code -- this happens in other files as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah dangit. they only match if you're using Rust-Analyzer. I wonder how to best resolve this

Copy link
Contributor

Choose a reason for hiding this comment

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

the original assumes you are not viewing the code with Rust-Analyzer's annotations so perhaps we can leave it like that? The other option I can think of is discarding the marker and instead describing the type parameters with text in a preceding comment (first param is X, second is Y, etc.)

@Mirabellensaft Mirabellensaft merged commit d63f8df into main Jan 12, 2022
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.

use defmt logging
4 participants