search

espressif / esp-adf

Espressif Audio Development Framework
Other
1.49k stars 667 forks source link

Changed order of definitions in i2s_stream to be compatible with cpp. (AUD-5329) #1192

Open tank104 opened 2 months ago

tank104 commented 2 months ago

Changed order of definitions in i2s_stream to be compatible with c++. C++ requires define of struct to match the order.

We are getting errors like:

components/audio_stream/include/i2s_stream.h:232:1: error: designator order for field 'i2s_stream_cfg_t::transmit_mode' does not match declaration order in 'i2s_stream_cfg_t'

See issue: https://github.com/espressif/esp-adf/issues/1190

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

X-Ryl669 commented 2 months ago

I think those macros should die (see comment in the linked issue). You're correcting one issue here (and thank you), but many other are pending underneath for the other structures you haven't used yet. This technique of construction by macro is dumb and should be avoided. The compiler is telling you so too, just listen to it.

tank104 commented 2 months ago

Sure I agree - but up to you if you want to put it in - it does solve my issues though, and the samples.

awong-dev commented 2 months ago

Would love to see this merged. It's bugging me too. Either way, this fix should go in.

As for macro based construction... is there an alternate pattern for C?

From a pure compat perspective, maybe the ADF team should adopt a pattern that any macro initializer list has to have a unittest with a .cc extension so it is run through the C++ compiler. That would likely lock in the ordering during CI in a "good enough" way.