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

Draft: Adding defmt-1.0 #909

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Draft: Adding defmt-1.0 #909

wants to merge 7 commits into from

Conversation

jonathanpallant
Copy link
Contributor

@jonathanpallant jonathanpallant commented Dec 3, 2024

Adds a defmt 0.3 proxy, and bumps most crates to 1.0.0-alpha.

Tested by patching the radio-app from the exercises to use the defmt-0.3 folder. That seems to work.

diff --git a/nrf52-code/radio-app/Cargo.toml b/nrf52-code/radio-app/Cargo.toml
index 78649ece..e959cfdd 100644
--- a/nrf52-code/radio-app/Cargo.toml
+++ b/nrf52-code/radio-app/Cargo.toml
@@ -33,3 +33,6 @@ incremental = false
 lto = "fat"
 opt-level = 3
 overflow-checks = false
+
+[patch.crates-io]
+defmt = { path = "/Users/jonathan/Documents/knurling-rs/defmt/defmt-03" }
diff --git a/nrf52-code/radio-app/src/bin/hello.rs b/nrf52-code/radio-app/src/bin/hello.rs
index 104bd832..60e6c3bd 100644
--- a/nrf52-code/radio-app/src/bin/hello.rs
+++ b/nrf52-code/radio-app/src/bin/hello.rs
@@ -8,6 +8,12 @@ use cortex_m_rt::entry;
 // this imports `src/lib.rs`to retrieve our global logger + panicking-behavior
 use radio_app as _;

+#[derive(defmt::Format)]
+struct Oblong {
+    width: u32,
+    height: u32
+}
+
 // the custom entry point
 // 👇🏾
 #[entry]
@@ -18,7 +24,8 @@ fn main() -> ! {
     // initializes the peripherals
     dk::init().unwrap();

-    defmt::println!("Hello, world!"); // 👋🏾
+    let ob = Oblong { width: 10, height: 20 };
+    defmt::println!("Hello, world! sq = {:?}", ob); // 👋🏾

     dk::exit();
 }
$ cargo run --bin hello -- --probe 1366:1051:001050288864
    Finished `dev` profile [optimized + debuginfo] target(s) in 0.14s
     Running `probe-rs run --chip nRF52840_xxAA --allow-erase-all target/thumbv7em-none-eabihf/debug/hello --probe '1366:1051:001050288864'`
      Erasing ✔ [00:00:00] [####################################################################################################################################################################################################] 8.00 KiB/8.00 KiB @ 29.09 KiB/s (eta 0s )
  Programming ✔ [00:00:00] [####################################################################################################################################################################################################] 8.00 KiB/8.00 KiB @ 24.74 KiB/s (eta 0s )    Finished in 0.628s
<lvl> Hello, world! sq = Oblong { width: 10, height: 20 }
└─ hello::__cortex_m_rt_main @ src/bin/hello.rs:28
<lvl> `dk::exit()` called; exiting ...
└─ dk::exit @ /Users/jonathan/Documents/ferrous-systems/rust-exercises/nrf52-code/boards/dk/src/lib.rs:415

@jonathanpallant
Copy link
Contributor Author

ahh, changelogs.

@jonathanpallant
Copy link
Contributor Author

error: package defmt is ambiguous: it is defined by in multiple manifests within the root path /home/runner/work/defmt/defmt

Not sure how to make cargo-semver-checks happy here.

@Urhengulas
Copy link
Member

error: package defmt is ambiguous: it is defined by in multiple manifests within the root path /home/runner/work/defmt/defmt

Not sure how to make cargo-semver-checks happy here.

Maybe you can exclude it?

@Urhengulas
Copy link
Member

Executing cargo semver-checks in the respective directory works.

$ cargo semver-checks --exclude defmt
<...>
$ cd defmt/
$ cargo semver-checks --default-features
     Parsing defmt v1.0.0-alpha (current)
      Parsed [   5.198s] (current)
     Parsing defmt v0.3.10 (baseline)
      Parsed [   5.530s] (baseline)
    Checking defmt v0.3.10 -> v1.0.0-alpha (major change)
     Checked [   0.000s] 0 checks: 0 pass, 94 skip
     Summary no semver update required
    Finished [  10.738s] defmt
$ cd ../defmt-03
$ cargo semver-checks --default-features
     Parsing defmt v0.3.100 (current)
      Parsed [   5.936s] (current)
     Parsing defmt v0.3.10 (baseline)
      Parsed [   5.592s] (baseline)
    Checking defmt v0.3.10 -> v0.3.100 (minor change)
     Checked [   0.006s] 87 checks: 84 pass, 3 fail, 0 warn, 7 skip
<checks all crates except the two defmt ones>

This should work (not tested):

diff --git a/.github/workflows/cargo-semver-check.yml b/.github/workflows/cargo-semver-check.yml
index b86b92c..0cbbf44 100644
--- a/.github/workflows/cargo-semver-check.yml
+++ b/.github/workflows/cargo-semver-check.yml
@@ -14,10 +14,20 @@ jobs:
 
     steps:
       - uses: actions/checkout@v4
-      - name: Semver check host crates
+      - name: Semver check host crates, except defmt
+        uses: obi1kenobi/cargo-semver-checks-action@v2
+        with:
+          exclude: defmt
+      - name: Semver check defmt v1
+        uses: obi1kenobi/cargo-semver-checks-action@v2
+        with:
+          feature-group: default-features
+          manifest-path: defmt/
+      - name: Semver check defmt v0.3
         uses: obi1kenobi/cargo-semver-checks-action@v2
         with:
           feature-group: default-features
+          manifest-path: defmt-03/
       - name: Semver check firmware crates
         uses: obi1kenobi/cargo-semver-checks-action@v2
         with:

Copy link

cloudflare-workers-and-pages bot commented Dec 9, 2024

Deploying knurling-defmt-book with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2bb6072
Status: ✅  Deploy successful!
Preview URL: https://d83f8d60.knurling-defmt-book.pages.dev
Branch Preview URL: https://defmt-1-0.knurling-defmt-book.pages.dev

View logs

@jonathanpallant
Copy link
Contributor Author

Rebased on main

Urhengulas
Urhengulas previously approved these changes Jan 7, 2025
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.

Looks good to me.

I am not sure if a minor version bump is necessary in defmt-semihosting, defmt-test, panic-probe, but it does not hurt either.

@jonathanpallant
Copy link
Contributor Author

jonathanpallant commented Jan 7, 2025

They are major version bumps, and I did that because there is no defmt compatibility at crate this time so you don't want to pick them up by accident.

@Urhengulas Urhengulas added pr waits on: author Pull Request requires changes from the author and removed pr waits on: author Pull Request requires changes from the author labels Jan 8, 2025
It doesn't like having two versions of defmt, and it ignores the 'exclude' field in the workspace.
…and pub use re-exports.

It's something Predrag is working on, but for now, we have to do this by hand.
@jonathanpallant
Copy link
Contributor Author

@Urhengulas Urhengulas dismissed their stale review January 9, 2025 13:06

Removing approval just to make sure we don't accidentally release.

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.

2 participants