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

Broken configuration of atomics with tokio/bytes #51

Closed
jklmnn opened this issue Feb 2, 2025 · 8 comments
Closed

Broken configuration of atomics with tokio/bytes #51

jklmnn opened this issue Feb 2, 2025 · 8 comments

Comments

@jklmnn
Copy link
Contributor

jklmnn commented Feb 2, 2025

I'm currently using this crate on an rp2040 and noticed that with the latest master release here I get the following errors:

error[E0599]: no method named `fetch_add` found for reference `&AtomicUsize` in the current scope
    --> /.../.cargo/registry/src/index.crates.io-6f17d22bba15001f/bytes-1.9.0/src/bytes.rs:1131:27
     |
1131 |     let old_cnt = ref_cnt.fetch_add(1, Ordering::Relaxed);
     |                           ^^^^^^^^^ method not found in `&AtomicUsize`

I'm using tokio-rs/bytes@16fd473 (see tokio-rs/bytes#467) and it works fine with mqttrs 0.4.1. It breaks with master and the problematic change seems to be 9f70f89 by @mchodzikiewicz. I see that there is 6e06377 but it doesn't seem to address the problem fully. Unfortunately I don't understand enough of the inner workings of Cargo to provide a fix.

My Cargo.toml is

[dependencies]
cortex-m-rt = "0.7.5"
defmt = "0.3.10"
defmt-rtt = "0.4.1"
embassy-time = "0.4.0"
static_cell = "2.1.0"
no-std-net = "0.6.0"

[dependencies.bytes]
#version = "1.9"
git = "https://github.com/tokio-rs/bytes.git"
rev = "16fd473d5c6ca20787e157ec19afc469438fc66b"
default-features = false
features = ["extra-platforms"]

[dependencies.cyw43]
version = "0.3.0"
features = ["defmt", "firmware-logs"]

[dependencies.cyw43-pio]
version = "0.3.0"
features = ["defmt"]

[dependencies.embassy-executor]
version = "0.7.0"
features = ["arch-cortex-m", "executor-thread", "defmt", "task-arena-size-16384"]

[dependencies.embassy-net]
version = "0.6.0"
features = ["defmt", "tcp", "udp", "raw", "dhcpv4", "medium-ethernet", "dns", "proto-ipv4", "proto-ipv6", "multicast"]

[dependencies.embassy-rp]
version = "0.3.0"
features = ["defmt", "unstable-pac", "time-driver", "critical-section-impl", "rp2040"]

[dependencies.mqttrs]
#version = "0.4"
git = "https://github.com/00imvj00/mqttrs.git"
rev = "6296bab1673b9d7d316165d46e2836d03a6e5965"
default-features = false

[dependencies.panic-probe]
version = "0.3.2"
features = ["print-defmt"]

[dependencies.portable-atomic]
version = "1.10"
default-features = false
features = ["critical-section"]

[dependencies.rp2040-hal]
version = "0.11"
features = ["critical-section-impl"]
@00imvj00
Copy link
Owner

00imvj00 commented Feb 3, 2025

@mchodzikiewicz, please check this out and see if we can resolve it. I would appreciate the help.

@mchodzikiewicz
Copy link
Contributor

mchodzikiewicz commented Feb 3, 2025

I can reproduce it in my project for esp32c3 (that also doesn't have hardware atomics) - I'll dig into it later today

@mchodzikiewicz
Copy link
Contributor

@jklmnn there was bytes release yesterday including commit you use, I have a slight suspicion that it was an odd version mismatch (cargo gets a bit weird with semver when you source a git commit instead of released crate), can you try to use published version of bytes 1.10.0?

It resolved my reproducer but I am not sure if it is universal

@mchodzikiewicz
Copy link
Contributor

Also, do you have a specific reason to use bytes over heapless?

That's not a solution for this specific issue but rather a hint I want to say out loud. I find avoiding usage of alloc more robust on small targets

@jklmnn
Copy link
Contributor Author

jklmnn commented Feb 5, 2025

@mchodzikiewicz The atomic error is gone with bytes 1.10 however now I get

error: no global memory allocator found but one is required; link to std or add `#[global_allocator]` to a static item that implements the GlobalAlloc trait

This problem starts to appear with 9f70f89. The commits before it work just fine. If I remove the explicit dependency to bytes I get the original error again. It's still pulling bytes.

Also, do you have a specific reason to use bytes over heapless?

Mostly because the README said so. But it doesn't seem to make a difference if I remove bytes from my dependencies, it's still pulling it, just without the correct features.

@jklmnn
Copy link
Contributor Author

jklmnn commented Feb 5, 2025

The problem seems to be that 9f70f89#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542L25 makes bytes non-optional. If I change it to

diff --git a/Cargo.toml b/Cargo.toml
index 0638cf0..13e9cd7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,7 +23,7 @@ std = ["bytes/std", "serde/std"]
 defmt = ["dep:defmt", "heapless/defmt-03"]
 
 [dependencies]
-bytes = { version = "1.0", default-features = false}
+bytes = { version = "1.0", default-features = false, optional = true}
 serde = { version = "1.0", features = ["derive"], optional = true }
 heapless = { version = "0.8" }
 defmt = { version = "0.3.10", optional = true }

It builds and runs just fine with the latest commit.

Tbh I'm not sure why you changed it to be non-optional, so I can't say whether this breaks something else.

@mchodzikiewicz
Copy link
Contributor

Yup, that's it - it's a rebase artifact I didn't catch, sorry, issue a PR that brings back bytes being optional or I'll do it later today

jklmnn added a commit to jklmnn/mqttrs that referenced this issue Feb 6, 2025
@jklmnn
Copy link
Contributor Author

jklmnn commented Feb 6, 2025

I opened #52.

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

No branches or pull requests

3 participants