Typos and some other things... #476

finalyards · 2025-10-07T21:44:26Z

This started when I was reading the sources and spotted some typos. Turns out, the IDE can find them all - so I did!

In the mean time, there are also other areas of improvement; this is a mixed bag kind of on purpose.

Some, few but some, places may indicate bugs, so please someone check out my comments before pushing this further.

.github/workflows/ci.yaml

finalyards · 2025-10-07T21:48:29Z

ci.sh

+cargo +nightly fmt --check --manifest-path ./host/Cargo.toml
+cargo +nightly fmt --check --manifest-path ./host-macros/Cargo.toml
+
+cargo clippy --manifest-path ./host/Cargo.toml --features central,gatt,peripheral,scan,security


Suggesting to raise these above the cargo batch stuff. Reasoning: when people make changes, we often (?) don't run the cargo fmt. Running these commands doesn't require any downloads, unlike the rest of the CI steps.

Added +nightly. It doesn't harm on the CI (where nightly already is), but it makes the command more copy-pasteable for us users. Serves as a documentation that yes, cargo fmt needs nightly, in this repo.

Added features to clippy. The intention is that it's run with all features, isn't it?

Yeah, that didn't work out:

error: 'cargo-fmt' is not installed for the toolchain 'nightly-x86_64-unknown-linux-gnu'.

https://github.com/embassy-rs/trouble/actions/runs/18328070279/job/52197152861

finalyards · 2025-10-07T22:00:14Z

ci.sh

+
+# Running tests requires some hardware, so we just check they compile.
+cargo test --manifest-path ./host/Cargo.toml --features central,gatt,peripheral,scan,security --no-run
+cargo test --manifest-path ./examples/tests/Cargo.toml --no-run


Some things going on here.. and the diff isn't too clear because lines moved to the top.

This helps:

-cargo test --manifest-path ./host/Cargo.toml --lib -- --nocapture -cargo test --manifest-path ./host/Cargo.toml --no-run -- --nocapture -cargo test --manifest-path ./examples/tests/Cargo.toml --no-run -- --nocapture +cargo test --manifest-path ./host/Cargo.toml --features central,gatt,peripheral,scan,security --lib -- --nocapture + +# Running tests requires some hardware, so we just check they compile. +cargo test --manifest-path ./host/Cargo.toml --features central,gatt,peripheral,scan,security --no-run +cargo test --manifest-path ./examples/tests/Cargo.toml --no-run

Added features to the test lib run. This exposes tests that were dormant before.

Removed -- --nocapture from the --no-run's. It's not needed there, since those lines only compile, don't run, and -- --nocapture only affects execution of crates, I believe.

Added features to the --no-run's, as well. This means all tests get compiled, by the CI.

finalyards · 2025-10-07T22:01:41Z

examples/apps/Cargo.toml

 embassy-futures = "0.1.1"
 embassy-sync = { version = "0.7" }
 embassy-time = "0.5"
-#embedded-hal = "1.0"


Such comments were intended to be removed, before merging, in an earlier PR. Now's a good time.

finalyards · 2025-10-07T22:04:12Z

examples/nrf-sdc/Cargo.toml

 #embassy-time = {path = "../../../embassy/embassy-time"}
-#embassy-time-driver = {path = "../../../embassy/embassy-time-driver"}
-#embassy-embedded-hal = {path = "../../../embassy/embassy-embedded-hal"}
-#embassy-hal-internal = {path = "../../../embassy/embassy-hal-internal"}


Here, and below, I decided to prune the Cargo.toml's a bit, without fully understanding what's going on.

If there's a comment, refering to a crate that's not in the [dependencies], such a comment may be stale, and can be let go. Note: there may be some reason that, say, embassy-time-driver should remain here. I don't know. Only familiar with the ESP32 targets.

finalyards · 2025-10-07T22:05:29Z

examples/rp-pico-2-w/Cargo.toml

-#embassy-hal-internal = {path = "../../../embassy/embassy-hal-internal"}
-#nrf-sdc = { path = "../../../nrf-sdc/nrf-sdc" }
-#nrf-mpsl = { path = "../../../nrf-sdc/nrf-mpsl" }
-#bt-hci = { path = "../../../bt-hci" }


e.g. bt-hci was not mentioned, otherwise, and nrf-* within a rp-pico-... file feel like a copy/paste.

finalyards · 2025-10-07T22:06:14Z

host-macros/src/uuid.rs

-use syn::{parse::Result, spanned::Spanned, Expr};
+use syn::parse::Result;
+use syn::spanned::Spanned;
+use syn::Expr;


Before, cargo fmt in CI was not applied to host-macros. If it is, this needs to be reformatted.

finalyards · 2025-10-07T22:08:56Z

host/Cargo.toml

+heapless = "0.9"  # "gatt", "security" need it
 trouble-host-macros = { path = "../host-macros", version = "0.3.0", optional = true }
 p256 = { version = "0.13.2", default-features = false, features = ["ecdh","arithmetic"], optional = true }
+rand = { version="0.8", default-features = false, optional = true }


hmm. here I seem to have just moved the line above the rand_... ones.

Most dependencies seem to be in alphabetical order, which I find very welcome. That's why. Can move back.

finalyards · 2025-10-07T22:11:00Z

host/src/attribute_server.rs

    att_table: AttributeTable<'values, M, ATT_MAX>,
    cccd_tables: CccdTables<M, CCCD_MAX, CONN_MAX>,
-    _p: PhantomData<P>,
+    _p: PhantomData<P>, // Q: may we have a comment on the use of this?  Ties 'PacketPool', somehow.


This is related to a journey I had, trying to understand the DefaultPacketPool system.

I then decided it's not worth it.

finalyards · 2025-10-07T22:13:23Z

host/src/attribute_server.rs

+                    //"last_handle_in_group for 0x{:0>4x?}, 0x{:0>2x?}  0x{:0>2x?}", // "Unknown display hint: '0>4x?' by IDE; does this not get tested? #TEMP
                    trace!(
-                        "last_handle_in_group for 0x{:0>4x?}, 0x{:0>2x?}  0x{:0>2x?}",
+                        "last_handle_in_group for 0x{:04x}, 0x{:02x}  0x{:02x}", // not sure which is the intended format, here!


My IDE (Rust Rover) showed:

Unknown display hint: '0>4x?'

It seems that's indeed an invalid combination (asked Copilot). What is the intended output - what should be here?

Also, shouldn't CI or something pick this up? I'm confused, please help. O:)

finalyards · 2025-10-07T22:13:47Z

host/src/attribute_server.rs

                    let response = &buffer[0..length];
+                    #[cfg(false)] // bad format
                    trace!("  0x{:0>2x?}", response);
+                    trace!("  0x{:02x}", response);


..here, too

finalyards · 2025-10-07T22:16:31Z

host/src/channel_manager.rs

                debug!("[l2cap][conn = {:?}] connection param update request: {:?}", conn, req);
                let interval_min: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_min);
-                let interva_max: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_max);
+                let interval_max: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_max);


This one's interesting. It's a typo in a variable name. Now..

the variable is not used, so it never mattered

should the variable be used, see next diff

Kindly, it would also be great if unused variables were reported. Have I missed something - didn't get anything on the IDE or the CI output. What's really going on?

finalyards · 2025-10-07T22:17:26Z

host/src/channel_manager.rs

                    conn,
                    ConnectionEvent::RequestConnectionParams {
                        min_connection_interval: Duration::from_micros(interval_min.as_micros()),
-                        max_connection_interval: Duration::from_micros(interval_min.as_micros()),


interval_min used for both min and max - is that the intention, too?

finalyards · 2025-10-07T22:18:36Z

host/src/host.rs

            + ControllerCmdSync<LeSetScanEnable>
            + ControllerCmdSync<LeSetExtScanEnable>
-            + for<'t> ControllerCmdSync<LeSetAdvEnable>
+            + ControllerCmdSync<LeSetAdvEnable>


I belive, but am not 100% sure, that the for<'t> is a no-op when 't is not used on the same element.

finalyards · 2025-10-07T22:20:01Z

host/src/scan.rs

                None
            } else {
-                Some(Instant::now() + config.timeout.into())
+                Some(Instant::now() + config.timeout)


flagged as unnecessary by - I think - clippy, due to the added features in CI checks

finalyards · 2025-10-07T22:22:15Z

host/src/security_manager/constants.rs


-/// Long duration, to disable the timer
-pub(crate) const TIMEOUT_DISABLE: Duration = Duration::from_secs(3600 * 24 * 365 * 10); // ~10 years
-                                                                                        // Workaround for Duration multiplication not being const


This is just cleanup. The const was used only as Instant::now() + ... to get a time stamp far, far in the future. For this, we can use Instant::MAX just as well.

finalyards · 2025-10-07T22:23:10Z

host/src/security_manager/mod.rs

            .as_ref()
            .map(|x| x.timeout_at())
-            .unwrap_or(Instant::now() + constants::TIMEOUT_DISABLE);
+            .unwrap_or(Instant::MAX /*no timeout*/);


..here is one of the Instant::MAX positions.

Should I remove the /*no timeout* - it's kind of obvious.

finalyards · 2025-10-07T22:28:05Z

host/src/types/mod.rs

 //! Common types.

 /// Traits for conversion between types and their GATT representations
+#[cfg(feature = "gatt")]


Seemed like we can make that conditional. I'm not trying to be meticulous about feature use, but it's generally pleasant not to need to see things (types, in this case) that's don't have to do with my app's setup.

It was a pleasure to make this PR. Hope you appreciate it as a mixed-bag approach... My main focus is in trying to learn the code, and make apps using BLE.

finalyards commented Oct 7, 2025

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

finalyards commented Oct 7, 2025

View reviewed changes

finalyards mentioned this pull request Oct 8, 2025

Adapted for latest rand APIs #474

Open

finalyards marked this pull request as draft October 8, 2025 11:31

typos, some (2-3) possible bugs

e2627fd

finalyards force-pushed the pr-typos-and-what branch from 7b2fe87 to e2627fd Compare October 15, 2025 19:42

Typos and some other things... #476

Are you sure you want to change the base?

Typos and some other things... #476

Uh oh!

Conversation

finalyards commented Oct 7, 2025

Uh oh!

Uh oh!

finalyards Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finalyards Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finalyards Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finalyards Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

finalyards Oct 7, 2025 •

edited

Loading

finalyards Oct 7, 2025 •

edited

Loading

finalyards Oct 7, 2025 •

edited

Loading

finalyards Oct 7, 2025 •

edited

Loading