Skip to content

Conversation

@bugadani
Copy link
Contributor

@bugadani bugadani commented Sep 1, 2023

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

Logging isn't particularly widely used in the HAL, but maybe defmt support may change that :)

PR is blocked on a new esp-println release

@bugadani bugadani force-pushed the defmt branch 3 times, most recently from c0b5cca to bafc9a9 Compare September 1, 2023 07:59
@bugadani bugadani changed the title Add defmt support Add defmt support, make log optional Sep 1, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

There might be an easier way to do this (completely stolen from the embassy crates :D)

Copy this file into the project: https://github.com/MabezDev/embedded-fatfs/blob/master/src/fmt.rs

and ensure that its the first module defined in the crate: https://github.com/MabezDev/embedded-fatfs/blob/master/src/lib.rs#L67-L68 by doing this you don't have to import all the macros.

@bugadani bugadani force-pushed the defmt branch 10 times, most recently from 76dfe9d to dbac001 Compare September 2, 2023 16:39
@bugadani bugadani force-pushed the defmt branch 11 times, most recently from 4a6f506 to aab19e3 Compare September 3, 2023 03:39
@bugadani

This comment was marked as outdated.

@bugadani bugadani marked this pull request as ready for review September 4, 2023 09:55
@bugadani bugadani requested a review from MabezDev September 4, 2023 09:55
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit pick :)

@MabezDev MabezDev merged commit 7866896 into esp-rs:main Sep 4, 2023
@bugadani bugadani deleted the defmt branch September 4, 2023 10:30
- This corresponds to the date that the `1.65.0` release was branched from `master`
- `1.65.0` for Xtensa devices (**ESP32**, **ESP32-S2**, **ESP32-S3**)
- `1.67.0` for all `async` examples (`embassy_hello_world`, `embassy_wait`, etc.)
- latest `stable` when using the `defmt` feature
Copy link
Member

Choose a reason for hiding this comment

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

Would have liked to discuss this a bit first :/ Guess I just need to release as-is.

Copy link
Contributor Author

@bugadani bugadani Sep 5, 2023

Choose a reason for hiding this comment

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

Well, apart from my comment being a slight lie (pinned defmt means MSRV is 1.70 or so I believe), the feature is opt-in and probably shouldn't affect existing users who don't enable it. In case you want to rush out the release, I'm not opposed to reverting.

SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Sep 22, 2023
* Executor related touchups

* Make log optional

* Add defmt feature and derive on Debug structs

* Test both log drivers

* Update esp-println

* Document defmt msrv
playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Sep 26, 2023
* Executor related touchups

* Make log optional

* Add defmt feature and derive on Debug structs

* Test both log drivers

* Update esp-println

* Document defmt msrv
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.

3 participants