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

A fix for a new regression in stable v5.0 #16

Merged
merged 4 commits into from
Feb 6, 2023

Conversation

tajnymag
Copy link
Contributor

@tajnymag tajnymag commented Dec 16, 2022

Related issue: espressif/esp-idf#10379

I just changed %d to %ld and the compiler seems to be happy now.

EDIT: I've also changed components version number in idf_component.yml back to 0.9.8 because idf component manager complained about the version not being semantic.

@turgu1
Copy link
Owner

turgu1 commented Dec 16, 2022

Hello tajnymag,
It is a bit weird as a change. Normally, %ld, %lu are for 64 bits values (%lld for 128 bits), but the parameters are 32 bits. Have you found the rationale for this change somewhere on the net?

@tajnymag
Copy link
Contributor Author

Hi,
yeah, %ld might not be the completely correct way of fixing this. However, I know what caused the regression. In the new version of Xtensa compiler, int32_t and uint32_t types are no longer defined as int but as long, so they no longer fit in %d.

Official migration doc: https://docs.espressif.com/projects/esp-idf/en/release-v5.0/esp32/migration-guides/release-5.x/gcc.html#int32-t-and-uint32-t-for-xtensa-compiler
GitHub issues explanation: espressif/esp-idf#9511 (comment)

@turgu1
Copy link
Owner

turgu1 commented Dec 16, 2022

Ok. I've just read the issue here. Your request would resolve the issue for v5.0 but will cause a bug with older versions.

To get this to work with all versions, there is a need to use "inttypes.h" content and use PRIu32 and PRIi32 in the formatting strings instead of %ld or %ud. This would be a change required to all pieces of code that must go to version 5.0

Do you want to make the change and reconduct a fix? I can do it if you can't.

@tajnymag
Copy link
Contributor Author

Makes sense. I can do it tomorrow.

@turgu1
Copy link
Owner

turgu1 commented Dec 16, 2022

As an example, the following:

ESP_LOGI(TAG, "STA Event, Base: %08x, Event: %ld.", (unsigned int) event_base, event_id);

would become:

ESP_LOGI(TAG, "STA Event, Base: %08" PRx32 ", Event: %" PRIi32 ".", (unsigned int) event_base, event_id);

Not sure if (unsigned int) would require some change...

@turgu1
Copy link
Owner

turgu1 commented Dec 16, 2022

Another one: lld would be PRIi64 instead

@turgu1 turgu1 merged commit 4a78e5b into turgu1:idf-v5.0-support Feb 6, 2023
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