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

Fix initialization macros for twai_timing_config_t not setting .clk_src and .brp (IDFGH-10134) #11405

Merged
merged 1 commit into from
May 24, 2023

Conversation

JustinOng
Copy link
Contributor

.clk_src and .brp of twai_timing_config_t are not set by the TWAI_TIMING_CONFIG_* macros.

The examples declare twai_timing_config_t globally and are thus zero-initialized (eg twai_self_test).

However, the C++ compiler warns about this:

/opt/esp/idf/components/hal/include/hal/twai_types.h:78:135: warning: missing initializer for member 'twai_timing_config_t::clk_src' [-Wmissing-field-initializers]
   78 | #define TWAI_TIMING_CONFIG_25KBITS()    {.quanta_resolution_hz = 625000, .tseg_1 = 16, .tseg_2 = 8, .sjw = 3, .triple_sampling = false}
      |                                                                                                                                       ^
/opt/esp/idf/examples/peripherals/twai/twai_self_test/main/twai_self_test_example_main.cpp:41:46: note: in expansion of macro 'TWAI_TIMING_CONFIG_25KBITS'
   41 | static const twai_timing_config_t t_config = TWAI_TIMING_CONFIG_25KBITS();
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/esp/idf/components/hal/include/hal/twai_types.h:78:135: warning: missing initializer for member 'twai_timing_config_t::brp' [-Wmissing-field-initializers]
   78 | #define TWAI_TIMING_CONFIG_25KBITS()    {.quanta_resolution_hz = 625000, .tseg_1 = 16, .tseg_2 = 8, .sjw = 3, .triple_sampling = false}
      |                                                                                                                                       ^
/opt/esp/idf/examples/peripherals/twai/twai_self_test/main/twai_self_test_example_main.cpp:41:46: note: in expansion of macro 'TWAI_TIMING_CONFIG_25KBITS'
   41 | static const twai_timing_config_t t_config = TWAI_TIMING_CONFIG_25KBITS();
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~

, and there are potential issues if twai_timing_config_t is declared on the stack without being initialized fully.

@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@espressif-bot espressif-bot added the Status: Opened Issue is new label May 16, 2023
@github-actions github-actions bot changed the title Fix initialization macros for twai_timing_config_t not setting .clk_src and .brp Fix initialization macros for twai_timing_config_t not setting .clk_src and .brp (IDFGH-10134) May 16, 2023
@suda-morris
Copy link
Collaborator

sha=a18d7e24d6efa86ced17e74867e6537e5cbe617e

@suda-morris suda-morris added the PR-Sync-Merge Pull request sync as merge commit label May 16, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels May 22, 2023
@espressif-bot espressif-bot merged commit b4e6019 into espressif:master May 24, 2023
@nebkat
Copy link
Contributor

nebkat commented Jun 30, 2023

Needs backport to release/v5.1.

@wanckl
Copy link
Contributor

wanckl commented Jul 3, 2023

@nebkat Hello, This fix is plain to including in v5.1.1 next month. if you are emergency to this fix, please tell me, I can give you a patch, but there will no full release test for this patch, So if you have mass produced, need consider the risk from this patch.
Although this little fix seems no any risks.

@nebkat
Copy link
Contributor

nebkat commented Jul 3, 2023

@wanckl This is just a compiler warning so not critical.

However #11331 is a regression in v5.1 - might be worth noting it somewhere until it is fixed. I have cherry-picked it already so no need for a patch for me (though perhaps others will need one).

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: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants