-
Notifications
You must be signed in to change notification settings - Fork 127
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
enforce no_std
for all? protocol/v2 crates
#1230
base: main
Are you sure you want to change the base?
Conversation
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
+ Coverage 19.36% 25.13% +5.76%
==========================================
Files 164 20 -144
Lines 10811 1134 -9677
==========================================
- Hits 2094 285 -1809
+ Misses 8717 849 -7868
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
03a2dae
to
fc640a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think the general idea is very welcome, I think addressing #1200 first would make more sense as it can potentially change how we approach this. I think in #1200 we should aim to provide a logger for std and no std env and then impl Debug
manually, which would allow us to remove the debug feature IIUC whats going here
Current
So it's pseudo
Maybe this can be discussed in #1132 ? |
If I do it before #1200, when #1200 will remove |
no_std
for all protocol/v2 cratesno_std
for all? protocol/v2 crates
Yea I think #1200 shouldn't necessarily be about removing |
dfb23fe
to
8ef9d5e
Compare
Actual This PR is not fixing this. CI does not see it because does not test it. |
Actual This PR is not fixing this. CI does not see it because does not test it. |
@Georges760 please note that we're aiming to deprecate I cannot give you a precise timeline but Q1 2025 would be my rough guess. I wonder if that would make this process simpler? |
It make sense, as it is already broken in binary_sv2 and framing_sv2 as stated above. So I understand the official/only codec way will be the "core" option ? Good |
So I stop here for this PR and open for reviews. |
@@ -19,6 +19,9 @@ | |||
//! Seq0255 <-> SEQ0_255[T] | |||
//! Seq064K <-> SEQ0_64K[T] | |||
//! ``` | |||
|
|||
#![cfg_attr(feature = "no_std", no_std)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now use #![no_std]
, and I'm confident that we are not using any std
-compliant methods in binary_codec_sv2
in any of the top-level crates that depend on it. Since almost all of our crates are no_std
-compliant, makes sense to have no-serde-sv2 to no-std only. cc: @rrybarczyk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked about this in this comment.
Current binary_codec_sv2
has a broken std
support, do we want to remove it or fix it ?
Look like you propose to remove it. It is also my preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think we should enforce no_std
on the protocols
crates. So let's remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me and is ready to go. We can enforce the no-std
restriction during our protocol refactoring effort, once we have more concrete reasoning for removing std
-related imports. However, I’m fine with either approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Let's confirm in the dev meet before we approve.
- only feature `debug` will add `std` dep which is not used by any other crates - just had to replace `std::hint` by its `core` equivalent - in `debug` removed a `mut` to clear a warning
- std::vec::Vec -> alloc::vec::Vec - std::mem -> core::mem - std::slice -> core::slice - std::convert::{TryFrom, TryInto} -> core::convert::{TryFrom, TryInto}
- std::format -> alloc::format - std::vec::Vec -> alloc::vec::Vec - std::string::{String, ToString} -> alloc::string::{String, ToString}
no need any feature for `std` stuff, there is none
- std::vec::Vec -> alloc::vec::Vec
completely removing |
if I understand correctly, we should expect this PR to leave all but I'm confused: why are we still leaving some crates with the
shouldn't |
following this criteria, we should bump the |
That is the end goal yes
I didn't consider subprotocols/* crates either in this PR
details here above
YES totally, but this PR being multi crate already, try to limit its action to not refactor any code, just remove |
exact, but usually the version bump is done in a separate PR/commit to merge multiple improvement in a single version increment. Should I do it in this PR anyway ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
you're right, we can leave it for a new PR |
@Georges760 could you please add a comment to the crates some crates are missing a for the crates that are missing a later, we're going to finish the |
Bencher Report
Click to view all benchmark results
|
Bencher Report
🚨 7 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Most of protocol/v2 crates are almost
no_std
, as listed here.Having a lib crate that can be
no_std
orstd
by the selection of a feature :#![cfg_attr(not(feature = "std"), no_std)]
or the current implementation :
#![cfg_attr(feature = "no_std", no_std)]
is only usefull if :
std
related dependency (for instancestd::vec::Vec
)std
so we disable the functionalities usingstd
In some case we can replace the
std
dep by itsalloc
equivalent if exists (alloc::vec::Vec
) and we can keep the functionality inno_std
environement.Finally, a
std
binary can use someno_std
dependencies, but anno_std
binary cannot use anystd
deps. So making the low level crates allno_std
will make them copamtible for every kind of binary (app).We should use the
std
feature only if some functionality requiresstd
and not have anyno_std
equivalent.As far as I tried all protocol/v2/ crates (libs) can be 100%
no_std
a,d does not need thestd
(orno_std
) feature to optionally addstd
dep.This PR is targeting to get rid of
std
dep without loosing any functionality or efficiency.Note: I will make 1 commit per crate to explain what and how I am doing it.
Note2: being
no_alloc
is another level of complexity for deep embedded system, which is not at all this PR concern.