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

Refactor fmt modules into a single shared embassy_fmt crate #190

Closed
wants to merge 3 commits into from

Conversation

rukai
Copy link
Contributor

@rukai rukai commented May 18, 2021

advantages:

  • reduces duplication
  • the #[macro_use] extern crate embassy_fmt ensure that we always use the defmt macros over the space expensive core macros.
    • previously the best we could do was use crate::fmt::{unreachable, *}; which will import unreachable and give a compile error if any formatting macros are used but not manually imported. But I was accidentally deleting these because of the unused warnings and I didnt realize they create a helpful compiler error instead of using a core format macro.
  • the current fmt module implementation has really strange repercussions for importing. the pub used macros are imported into the fmt module while the new macro_rules macros have a global scope. This means the macros are accessible at different paths depending on if the defmt feature is enabled or not. There are two ways to resolve this issue:

disadvantages:

  • introduces a new nightly feature requirement: namespaced-features

We could potentially provide embassy_fmt as a wrapper over defmt/log for end users but for now its just considered an implementation detail.
I havent changed stm32 to use it yet, I plan to do that in a follow up PR which will also add a .cargo/config and fix the warnings for stm32.

@rukai
Copy link
Contributor Author

rukai commented May 20, 2021

btw this PR reduces the size of most of the nrf examples by 20K.
image
image

We could certainly achieve this by careful manual inspection but it was made trivial by #[macro_use] extern crate embassy_fmt

@rukai rukai force-pushed the embassy_fmt_crate branch from 386406c to 5170697 Compare May 21, 2021 12:28
@rukai
Copy link
Contributor Author

rukai commented May 21, 2021

I realized how to do this without namespaced-features, so i've removed it. (in hindsight it was pretty obvious)
It makes the features we expose a little more confusing but I think its worth it to avoid reintroducing namespaced-features.
I've also changed stm32 to use the new embassy_fmt crate.

@rukai rukai force-pushed the embassy_fmt_crate branch from 5170697 to a51d9b5 Compare May 22, 2021 00:25
@rukai rukai force-pushed the embassy_fmt_crate branch 4 times, most recently from dbd7641 to 0b8e456 Compare May 29, 2021 09:58
@Dirbaio
Copy link
Member

Dirbaio commented Jun 6, 2021

Sorry for taking so long to get back to you on this 😓

This fmt stuff is a very very hairy topic. I do agree the current solution is very suboptimal, however it's been changed many times already in the past and it seems all solutions that we find have some downside :(

The embassy-fmt crate approach also has some:

  • If we rename the defmt feature to defmt-enable, the user can accidentally enable defmt instead, which will enable the optional dependency but not actually use it for logging.
  • #[macro_use] extern crate foo is the Rust 2015 way of importing macros from another crate. It's accidentally convenient, as it puts the macros in-scope in the entire local crate, but I think its use is somewhat discouraged now.
  • proc macros are not friendly to reexporting. defmt macros emit code that does defmt::blahblah. This code gets parsed in the calling crate's context, so defmt needs to be in scope there. This causes problems if only some crates have defmt enabled. Try this: edit embassy-nrf-examples/Cargo.toml then remove defmt feature from embassy-nrf, but leave it in for embassy. Build then fails, because embassy enables embassy-fmt/defmt. embassy-nrf then calls info! or whatever, which emits code that does defmt::blahblah, but defmt is not resolved in embassy-nrf because the optional dep is not enabled there.

I think due to the last point we're stuck with copypasting a fmt.rs to all the crates, so all the macroing is done in-crate to not reexport proc macros. The how the fmt.rs works is definitely improvable though.

For example, to avoid "unused import" warnings depending on log/defmt features maybe all macros should be macro_rules! that call defmt, log, core, or nothing? Then no use is needed at all in all cases I think.

@rukai
Copy link
Contributor Author

rukai commented Jun 6, 2021

thanks for the feedback!
I'll do some more investigation on the problem and get back to this in a week or two.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 7, 2021

With #227 the unfixable warnings should be gone.

Thank you @rukai for the push and trying out different solutions!

@Dirbaio Dirbaio closed this Jun 7, 2021
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