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

Bugfix: Fix compile error for ESP32 FSM ULP GPIO Example (IDFGH-9687) #11028

Closed
wants to merge 4 commits into from
Closed

Bugfix: Fix compile error for ESP32 FSM ULP GPIO Example (IDFGH-9687) #11028

wants to merge 4 commits into from

Conversation

Kampi
Copy link
Contributor

@Kampi Kampi commented Mar 20, 2023

Fix different compile errors in ulp_example_main.c when compiling the code with C++ 20 and above.

@github-actions github-actions bot changed the title Bugfix: Fix compile error for ESP32 FSM ULP GPIO Example Bugfix: Fix compile error for ESP32 FSM ULP GPIO Example (IDFGH-9687) Mar 20, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 20, 2023
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Mar 22, 2023
Copy link
Contributor

@0xjakob 0xjakob left a comment

Choose a reason for hiding this comment

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

@Kampi Thanks a lot for this fix! It looks good except that I have one question regarding the changes. I think after it is resolved, we can merge it.

uint32_t pulse_count = 0;
esp_err_t err = nvs_get_u32(handle, count_key, &pulse_count);
assert(err == ESP_OK || err == ESP_ERR_NVS_NOT_FOUND);
printf("Read pulse count from NVS: %5"PRIu32"\n", pulse_count);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Kampi What is the reason for this change? The formatting macros have been introduced to fix warnings after switching between compilers for different platforms (e.g. Xtensa-gcc to RISCV-gcc). If it is because of the C++ compiler warning about invalid suffix on literal, please add the suggested change instead of changing the macro back to a hard-coded formatter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @0xjakob,

thanks for your suggestion. Your argument makes sense to me and I guess I had an error in my setup because I forgot (or accidentally removed) #include <inttypes.h>, which leads to a compilation error. I have modified the pull request and reverted this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kampi OK, thanks for the fix! Would you mind squashing the commits into one? Then we wouldn't have an intermediate commit that could potentially break builds on machines where the print formatters are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @0xjakob,
not sure if this was successful, because I´ve never done this before. Please check it. Otherwise I can create a new merge request.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kampi OK, let's leave it as it is then. We will amend the commits to squash them. The only difference is that the PR will be seen as closed instead of merged. The rest stays the same. In particular, your authorship of the commit in IDF will be retained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @0xjakob
It's fine. Thanks!

@0xjakob
Copy link
Contributor

0xjakob commented Mar 23, 2023

sha=c96429e395ac13064038eee6d9163170725dcf24

@0xjakob 0xjakob added the PR-Sync-Merge Pull request sync as merge commit label Mar 23, 2023
@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Selected for Development Issue is selected for development Resolution: NA Issue resolution is unavailable labels Mar 27, 2023
@Kampi Kampi deleted the v5.0_Bugfix_ULP_FSM_GPIO_Example branch March 31, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants