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

Enable vsnprintf on compilers newer than 1.73.0.1 #293

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 18, 2023

A not terribly complex patch to resolve #16. It might be easier to just bump MSRV, but 🤷‍♂️.

Also closes esp-rs/esp-mbedtls#13

@AnthonyGrondin
Copy link
Contributor

This seems to fix the Xtensa part of esp-rs/esp-mbedtls#13

I haven't narrowed down the problem caused by printing \n, for RISCV targets (now Xtensa) after this patch.
I can provide more testing later (24h-48h) with esp-mbedtls if needed.

I'd like to get it fixed eventually so that I don't have to patch every printf in mbedtls C libs because they're not printing 😆

@bugadani
Copy link
Contributor Author

Hmm, do you have access to something I don't? This PR should have no effect right now without the yet-to-be-built compiler 😅 But I'll take a win if you offer one.

As to \n, if the problem is now consistent between xtensa and risc-v, maybe we can go through the vsnprintf algo to see what's what.

@AnthonyGrondin
Copy link
Contributor

Oh, no I don't.

I didn't realize it wasn't out yet. I just did a static analysis.

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 19, 2023

You can already download builds for Windows MSVC and Linux 64:
https://github.com/esp-rs/rust-build/releases/tag/untagged-3ecdd54e672f465cad2e

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 19, 2023

Should also conditionally enable calling syslog in log_write and log_writev

@bugadani
Copy link
Contributor Author

bugadani commented Oct 19, 2023

You can already download builds for Windows MSVC and Linux 64: https://github.com/esp-rs/rust-build/releases/tag/untagged-3ecdd54e672f465cad2e

I guess that might be a private pre-release because I'm getting a page not found error.

Should also conditionally enable calling syslog in log_write and log_writev

I think I can just call syslog unconditionally, it should handle the conditional aspects of this internally. Let me know if I'm missing something.

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 19, 2023

URL changed: https://github.com/esp-rs/rust-build/releases/tag/v1.73.0.1
Shouldn't be private

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 19, 2023

I think there must be a reason why there are cfg guards in those other functions - maybe just to log anything if we are not able to use c-variadics or it was crashing when trying to pass the variadics 🤔 t.b.h I don't really remember exactly

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 19, 2023

Fixes #16

@bugadani
Copy link
Contributor Author

bugadani commented Oct 19, 2023

it was crashing when trying to pass the variadics

I think I can test this, this is a fair point.

  • 1.73.0.1 - several empty log strings, same as @AnthonyGrondin reports. turns out this is just some internal logging code being implemented weirdly. The code prints a \n at the end, in a separate printf call, as printf("%s", "\n") 😅
  • 1.73.0.0
  • 1.72.1.0
  • 1.72.0.0

Update: no crash with current PR using supported rustc on esp32s3

@bugadani bugadani marked this pull request as ready for review October 19, 2023 10:13
@bugadani
Copy link
Contributor Author

I've raised the level of puts from trace to info so it's the same as syslog (and thus, printf) now. I'm tempted to implement the level-based filtering for syslog but that would just be even more binary bloat.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@bjoernQ bjoernQ merged commit 992832c into esp-rs:main Oct 20, 2023
7 checks passed
@bugadani bugadani deleted the vaarg branch October 20, 2023 08:13
@AnthonyGrondin
Copy link
Contributor

Thank you for taking care of it.

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.

printf binding doesn't work for strings without arguments. ESP32: Wifi logs cannot be configured
3 participants