[5/n] Format trait v2 #508
Conversation
jonas-schievink
added
the
pr waits on: author
Dirbaio
force-pushed
the
format-v2
branch
2 times, most recently
from
2da3f6a to
7495528
Compare
Dirbaio
force-pushed
the
format-v2
branch
3 times, most recently
from
57021ad to
a7e06d2
Compare
|
This is now ready to go! @japaric @jonas-schievink |
There was a problem hiding this comment.
This is awesome! It's great to see the InternalFormatter go away. Also thanks for the macros to simplify the primitive implementations.
Something I noticed that doesn't need to be addressed in this PR:
I no longer see a point to the internp! vs intern! distinction because we no longer LEB encode the tag.
Originally, internp! was created to put "primitive" tags like =u8 at the beginning of the .defmt section so they get smaller values (<128). Because we LEB encoded those tags, primitive tags would be encoded as 1 byte on the wire. The rationale was that primitives are more likely to appear behind =? so they'll take less space on the wire but now we always encode the tag as 2 bytes.
So I think internp! and the .defmt.prim input section can probably go away.
| @@ -230,34 +14,3 @@ pub struct Str { | |||
| /// 14-bit address | |||
There was a problem hiding this comment.
nit: this is no longer true because this can be up to 16 bits in size
|
bors r+ |
|
Build succeeded: |
Should we track this in an issue? Or note it in the RFC? |
|
There is still some advantage of putting primitives first. Tags less than 256 have one zero byte. When rzCOBS-encoded, these will be 9 bits (8+1) instead of 16 bits. |
(I have not been following all PRs but) this is not being used in the code base at the moment, right? at least, that didn't seem to be the case from looking at the decoder tests. Would rzCOBS be applied not in the log macros but rather in the transport layer crate, e.g. defmt-rtt? |
|
Yeah, it's not done yet, but coming next. I have a PoC here. Yes, it's in the Logger impl. It has to be because impls that allow concurrent logging (eg one channel per irq prio level) need to have one rzcobs encoder state for each channel. If it was on the |
SInce knurling-rs#508 it is possible to call it multiple times, and the result is concatenated as you'd expect. The rest of the comment is still relevant. Fixes knurling-rs#583
519: target-side `env_logger`-like env filter r=Urhengulas a=japaric This PR implements an alternative approach to disabling/enabling logging in crates. Currently we use [Cargo features](https://defmt.ferrous-systems.com/setup-library.html). This PR removes the need for those Cargo features and replaces them with the env variable: `DEFMT_LOG`. The `DEFMT_LOG` env var specifies which crates should emit defmt logs and at which level. Its syntax is the same as the syntax used by the [`env_logger`](https://docs.rs/env_logger/0.8.4/env_logger/) crate. ~~The `DEFMT_LOG` env var parser currently rejects paths that include modules: so `DEFMT_LOG=krate=info` is OK, but `DEFMT_LOG=krate::module=info` is rejected. This is intentional and should let us accept module paths and filter based on modules in the future (as in add module support in a backwards compatible fashion).~~ No longer true, `DEFMT_LOG` now supports module paths. One of the main advantages of `DEFMT_LOG` is that allows filtering logs with module level granularity. The current Cargo features based approach is more coarse grained and only supports enabling / disabling entire crates. *NOTE* this is a *target* side *emit* filter and orthogonal to the *host* side *display* filter proposed in knurling-rs/probe-run#74. Disabling logs with the target filter makes the target program smaller (, maybe also slightly faster) and it reduces the amount of data sent from the target to the host. Disabling logs with the host filter does not change the target program at all not it changes the amount of data transmitted from the target to the host. Changing the value of the `DEFMT_LOG` env var causes all crates that transitively depend on the `defmt_macros` crate to be recompiled. Example usage: ``` console $ bat src/bin/hello.rs ``` ``` rust fn main() -> ! { defmt::info!("hello"); foo::foo(); bar::bar(); app::exit() } mod foo { pub fn foo() { defmt::info!("foo"); } } mod bar { pub fn bar() { defmt::info!("bar"); } } ``` ``` console $ DEFMT_LOG=hello::foo cargo r --bin hello 0 INFO foo └─ hello::foo::foo @ src/bin/hello.rs:16 $ DEFMT_LOG=hello::bar cargo r --bin hello 0 INFO bar └─ hello::bar::bar @ src/bin/hello.rs:22 $ DEFMT_LOG=hello::foo,hello::bar cargo r --bin hello 0 INFO foo └─ hello::foo::foo @ src/bin/hello.rs:16 1 INFO bar └─ hello::bar::bar @ src/bin/hello.rs:22 $ DEFMT_LOG=hello cargo r --bin hello 0 INFO hello └─ hello::__cortex_m_rt_main @ src/bin/hello.rs:8 1 INFO foo └─ hello::foo::foo @ src/bin/hello.rs:16 2 INFO bar └─ hello::bar::bar @ src/bin/hello.rs:22 $ rg '\[features' Cargo.toml || echo no Cargo features no Cargo features ``` --- ## how to test this as this is not even merged into the `main` branch and breaking changes (with respect to defmt v0.2.0) have landed into `defmt`, the whole process is quite involved 1. install a `probe-run` that uses the decoder in this branch ``` console $ git clone https://github.com/knurling-rs/probe-run $ cd probe-run $ # change the `#[dependencies]` section $ edit Cargo.toml ``` ``` diff colored = "2.0" -defmt-decoder = { version = "=0.2.2", features = ['unstable'] } +defmt-decoder = { git = "https://github.com/knurling-rs/defmt", branch = "env-filter", features = ['unstable'] } difference = "2.0" ``` ``` cansole $ cargo install --path . ``` 2. use `[patch.crates-io]` in `Cargo.toml` (the one in the root of the workspace) to use the git version of `defmt` instead of v0.2.x. The `app-template` already includes this section in [its `Cargo.toml`](https://github.com/knurling-rs/app-template/blob/52981902ebeb77d612635e7953882653e8019ebc/Cargo.toml#L73-L79) ``` toml [patch.crates-io] defmt = { git = "https://github.com/knurling-rs/defmt", branch = "env-filter" } defmt-rtt = { git = "https://github.com/knurling-rs/defmt", branch = "env-filter" } defmt-test = { git = "https://github.com/knurling-rs/defmt", branch = "env-filter" } panic-probe = { git = "https://github.com/knurling-rs/defmt", branch = "env-filter" } ``` *IMPORTANT* use `branch = "env-filter"` instead of `rev = "somehash"` 3. set the `PROBE_RUN_IGNORE_VERSION` env variable before you run your firmware e.g. ``` console $ PROBE_RUN_IGNORE_VERSION=1 cargo run --bin hello ``` 4. try setting / changing the value of the `DEFMT_LOG` env variable! ``` console $ export PROBE_RUN_IGNORE_VERSION=1 $ DEFMT_LOG=hello=trace cargo run --bin hello 0 INFO Hello, world! └─ hello::__cortex_m_rt_main @ src/bin/hello.rs:8 ``` --- As there's other breaking-change work currently ongoing (#508) I plan to leave this work on hold for now -- but will be answering to feedback! -- and resume it when that's concluded. ## Unresolved questions - rename `DEFMT_LOG` to `DEFMT_CAPTURE` or `DEFMT_TARGET_CAPTURE`? if we keep the `DEFMT_LOG` name, what should we name the env var to be used in knurling-rs/probe-run#74? ## TODO list - [x] RFC approval - [x] address inline TODO comments - [x] support log level without module path e.g. `DEFMT_LOG=info` - [x] support the `off` pseudo log-level (disables logging) - [x] fix module path handling so that `DEFMT_LOG=krate::module` doesn't match `krate::module1`, `krate::module2`, etc. - [x] fix CI - [x] documentation. important changes in behavior to note - default log level is error (when DEFTM_LOG is not set) - compilation profile (`dev` / `profile`) has no effect on the log level filter - ["this removes the ability to set a default log level"](#519 (comment)) closes #183 closes #255 closes #459 Co-authored-by: Jorge Aparicio <jorge.aparicio@ferrous-systems.com>
547: Migration guide `v0.2.x` to `v0.3.0` r=japaric a=Urhengulas Migration guide from `defmt v0.2.x` to version `v0.3.0`. https://deploy-preview-547--admiring-dubinsky-56dff5.netlify.app/migration-02-03.html Fixes #530. ### TODO - [x] #505: Logger trait v2 - [x] #521: [3/n] Remove u24 - [x] #522: Replace `µs` hint with `us` - [x] (no adaption needed) ~~#508: [5/n] Format trait v2~~ - [x] #519: `DEFMT_LOG` - [x] #550: `defmt::flush()` - [x] knurling-rs/probe-run#198, knurling-rs/probe-run#250, Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
Part of #492 . Depends on
#505 #507 #521#524