Skip to content

Replace no_std feature with std feature #48

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

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

jhbruhn
Copy link

@jhbruhn jhbruhn commented Feb 3, 2022

This PR fixes #23 and fixes #47 by replacing the no_std flag with an additive std flag and disabling default features on serde.

libm is not an optional dependency anymore because it's functions are required in a no_std context. But as they are only used in said context, it should get optimized away if the std flag is enabled.
I'm happy to change that though if anyone has a better suggestion.

I have chosen not to explicitly enable serdes std flag as it is not required in this library and any consuming application will have that enable anyways if they are using serde in an std context, or am I missing a case there?

Copy link
Member

@eldruin eldruin 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 so far. Could you:

  • Adapt the CI .github/workflows/build.yml changing the no_std feature to `std?
  • Add an entry to the changelog
  • See if it is possible to use the std for the calculations where libm is currently explicitly used and only use the libm version in the no_std case? (e.g here)

@jhbruhn
Copy link
Author

jhbruhn commented Feb 3, 2022

Unfortunately I didn't understand your last point about libm, could you rephrase that? Are you suggesting to use libm-based calculations both for std and no_std targets?
Currently in this PR, no_std targets are using libm implementations while std targets are using the std-based implementations.
IMO, we could just use the libm implementations without loosing anything, but maybe I'm missing important details about libm here?

@eldruin
Copy link
Member

eldruin commented Feb 3, 2022

Sorry about that. I mean it would be good to avoid the libm dependency completely if including the std. At the moment libm seems to be used only for a few calculations in the humidity.rs file.
For example, one could use either libm::log for no-std or f64::ln with std enabled.

@jhbruhn
Copy link
Author

jhbruhn commented Feb 3, 2022

I agree, but I think this is not possible right now (rust-lang/cargo#1839) without introducing a new no-std feature, which would again mean that we could activate both std and no-std with is...semantically interesting.

@eldruin
Copy link
Member

eldruin commented Feb 4, 2022

Yeah that is a limitation. We will have to compile it no matter what. However, we could simply not use it and use the std function versions instead if enabled, as this may mean using more optimized math function implementations.

@jhbruhn
Copy link
Author

jhbruhn commented Feb 4, 2022

Agreed, that is already the case in this PR. The only place I could find that uses libm is humidity.rs, which has feature-gated implementations for some fns depending on whether std is enabled or not.

@eldruin
Copy link
Member

eldruin commented Feb 4, 2022

Interesting. Sorry about that. I saw now that the whole functions are simply not available when std is enabled (same situation as before). However, it does not make much sense to me how this has been the whole time. Maybe it was only an artifact of the no_std feature situation.
Could you try enabling them when std is enabled as well (but now using the std math functions)?

@jhbruhn
Copy link
Author

jhbruhn commented Feb 4, 2022

Which ones are you talking about? humidity.rs provides alternative implementations for some functions, if std is enabled, std is used, if std is not enabled, libm is used.
angle.rs seems to have some functions which are only enabled with the std features, I can add additional libm implementations for those.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, sorry for my confusion and thanks for bearing with me.
This PR is of course great as it is.
We can enable the trigonometric operations on Angle when on no-std with libm on a separate PR if you are interested.

@eldruin eldruin merged commit 31eca19 into rust-embedded-community:master Feb 4, 2022
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.

serde requires std Flip no_std flag
2 participants