Rename MIN and MAX to Z_MIN and Z_MAX for Zephyr compatibility

[marc-hb]

Fixes zephyrproject-rtos/zephyr#33567

No functional change, compiler output is identical.

Note a number of some source files modified are not compiled in the
default configurations and were not even compile-tested locally:

src/ipc/handler.c
src/schedule/timer_domain.c
zephyr/wrapper.c
src/audio/codec_adapter/codec_adapter.c
src/audio/drc/drc.c
src/audio/drc/drc_generic.c
src/audio/crossover/crossover.c
src/audio/smart_amp/smart_amp.c
src/audio/smart_amp/smart_amp_maxim_dsm.c

Signed-off-by: Marc Herbert marc.herbert@intel.com

[marc-hb]

The 3 checkpatch warnings long predate this PR.

[dbaluta]

What does it mean 'Zephyr compatibility' ? Is Zephyr using Z_MAX naming instead of MAX?

[marc-hb]

What does it mean 'Zephyr compatibility' ? Is Zephyr using Z_MAX naming instead of MAX?

Zephyr has both MAX() and Z_MAX(). It uses Z_MAX at run-time with gcc and MAX with other toolchains and at compile-time. See zephyrproject-rtos/zephyr#17941

The longer story is in SOF issue zephyrproject-rtos/zephyr#33567 fixed by this.

[marc-hb]

https://sof-ci.01.org/sof-pr-viewer/#/build/PR4002/build6260929 built and ran unit tests fine but did not run any "real" tests without any error message available anywhere.

I don't think this is caused by this PR because the exact same happened an hour later for totally unrelated PR
https://sof-ci.01.org/sof-pr-viewer/#/build/PR4000/build6261036

@zrombel any idea?

PS: I filed internal issue PTS-31638

[marc-hb]

https://sof-ci.01.org/sof-pr-viewer/#/build/PR4002/build6260929 built and ran unit tests fine but did not run any "real" tests without any error message available anywhere.

I'm downgrading this to a draft until we have at least one positive Quickbuild run considering the number of files changed by this PR and how insanely complicated C pre-processing can sometimes get (e.g. #3804). While Quickbuild has been mostly dysfunctional recently, when we're lucky the tests it runs can find bugs nothing else does. For instance there seems to be no default configuration using src/audio/codec_adapter/codec_adapter.c (and other files, see commit message) whereas Quickbuild just found an issue in this same file in (totally unrelated) #3999 (comment)

[lgirdwood]

Can we have a sof-rtos.h that defines Z_MIN, Z_MAX etc and also does a

#ifdef ZEPHYR
#error This file should not be in Zephyr build
#endif 

This way there is no confusion over which macro we are using.

[marc-hb]

Yes but later please, immediately after merging this. I want to keep this gigantic commit free of functional changes. At this moment the binary output is exactly the same before/after this commit which makes super quick and convenient to validate; no need to run any test (except for making sure nothing is missing from it, hence the wait for QB).

It also enough to fix zephyrproject-rtos/zephyr#33567 which has been blocked on it for some time (sorry), don't want to delay that more with more complex projects.

This will also avoid drowning the future sof-rtos.h in this one. Continuous Integration.

Finally, everything except Quickbuild has been passing, even https://sof-ci.01.org/sofpr/PR4002/build8591/devicetest has. Don't want to risk losing such an exceptional planet alignment.

[lgirdwood]

Now is the best time to make functional changes - as we have just tagged v1.7. We have far bigger problems if we get confused between sof RTS and Zephyr about which macros and APIs we are using.

[lgirdwood]

The rest of the PR is good except for this part.

Today we wrap Zephyr RTOS.

1. Call most SOF RTOS APIs natively.
2. Have a zephyr/wrapper.c that wraps the zephyr calls to look like SOF calls.

The target is to wrap SOF/xtos RTOS

1. Call all RTOS APIs as native zephyr APIs.
2. Have a file and header that wraps the SOF RTOS calls/macros so they look like Zephyr.

This PR needs to create a sof/xtos wrapper.c and header.

[marc-hb]

There was a hardware issue, Quickbuild is running again and it's green: https://sof-ci.01.org/sof-pr-viewer/#/build/PR4002/build6263141
Besides checkpatch everything is green, even https://sof-ci.01.org/sofpr/PR4002/build8591/devicetest. However there is now a conflict with src/audio/codec_adapter/codec_adapter.c, this needs rebase.

[lyakh]

I'm not sure this is the best solution. First of all the "z_" namespace is used by Zephyr, so, claiming it now for us doesn't seem logical to me. Secondly, I thought our goal was to use Zephyr APIs natively, and Zephyr does already define MIN() and MAX(), right? So, for Zephyr builds we should just remove our own definitions of those and automatically begin using those from Zephyr. Whereas only for native non-Zephyr builds we should use our own MIN(), MAX(), etc. Isn't this what we wanted to do?

[marc-hb]

Secondly, I thought our goal was to use Zephyr APIs natively, and Zephyr does already define MIN() and MAX(), right?

As I already wrote above, Zephyr APIs use both Z_MIN() at (gcc) run-time and MIN() at compile-time. See references above.

Before this PR, SOF uses MIN() at run-time. This is inconsistent with Zephyr and fails to compile.

This PR is a (literally) big step towards the goal of using Zephyr APIs by merely switching to the name Z_MIN() for run-time.

[marc-hb]

Pure rebase to fix small conflict with 2888212, here's the manual git range-diff (Github does not care about force-pushes and git range-diff)

 1:  7a5db242d47e ! 10:  b47edcd8b041 Rename MIN and MAX to Z_MIN and Z_MAX for Zephyr compatibility
      ## src/audio/codec_adapter/codec_adapter.c ##
        /* Allocate local buffer */
    --  buff_size = MAX(cd->period_bytes, codec->cpd.in_buff_size) * buff_periods;
    -+  buff_size = Z_MAX(cd->period_bytes, codec->cpd.in_buff_size) * buff_periods;
    +-  buff_size = MAX(cd->period_bytes, codec->cpd.out_buff_size) * buff_periods;
    ++  buff_size = Z_MAX(cd->period_bytes, codec->cpd.out_buff_size) * buff_periods;
        if (cd->local_buff) {
                ret = buffer_set_size(cd->local_buff, buff_size);
                if (ret < 0) {

[nashif]

a note that Z_/z_ zephyr APIs are internal to zephyr and shall not be used outside of Zephyr.

[marc-hb]

Thanks @nashif, can you confirm the boundary of what "internal" means? More specifically, can you confirm modules/audio/sof is considered external? So "internal" mean C code in the https://github.com/zephyrproject-rtos/zephyr git repo only? I couldn't find any information about the z_ prefix.

[lgirdwood]

@nashif I'm assuming we just use Zephyr MIN() and MAX here ? I'm also wondering if it's worthwhile now changing the drivers over to the Z_/z_ internal APIs since they are destined to be part of Zephyr later this year (i.e. driver code would use Z_MIN()). This would give them more validation prior to merging.

[marc-hb]

As stated earlier, the most urgent problem is to resolve the clash between Zephyr's compile-time MIN() versus SOF's run-time MIN(). This is holding back Zephyr for everyone right now, including the vast majority of Zephyr users who don't care about SOF at all.

Assuming SOF should never use Z_MIN() or anything starting with Z_ (I'm still not sure about that sorry), would the following mass search/replace be acceptable instead: MIN() -> SOF_RT_MIN()? Better name suggestions welcome.

So, for Zephyr builds we should just remove our own definitions of those and automatically begin using those from Zephyr. Whereas only for native non-Zephyr builds we should use our own MIN(), MAX()

I'm all for code re-use and I hate duplication but for such simple and short macros I don't think it matters. In this case I think it's better to keep it simple and have the same macros use the same definition always. We've seen the massive confusion that can happen when the pre-processor gets abused (#3804, admittedly an extreme example). The focus should IMHO be to get these different configurations to just compile with limited complexity which as we're seeing is already a challenge because outdated C doesn't support namespaces and its pre-processor even less; hence these Z_ and SOF_ prefix kludges.

PS: yet another git conflict, this time with src/ipc/handler.c, yay! This is bound to happen with such a large search and replace and yet another reason why I want follow-ups to be in different PRs immediately after this one but not in this one.

[marc-hb]

As stated earlier, the most urgent problem is to resolve the clash between Zephyr's compile-time MIN() versus SOF's run-time MIN()[*]. This is holding back Zephyr for everyone right now, including the vast majority of Zephyr users who don't care about SOF at all.

Speaking of holding back Zephyr, there will be an extra step after this: cherry-picking it to zephyrproject-rtos/sof/somebranch. To make that next step faster please help with generic SOF upstreaming "optimization" zephyrproject-rtos#7 (comment)

[andyross]

This seems to be getting bikesheddy. Really the stuff in Zephyr util.h is something of a special case. We provide a bunch of unprefixed macros there that more or less conform to broad industry[1] convention, like IS_ENABLED, ARRAY_SIZE, CONTAINER_OF, etc... MIN and MAX are two of those, though they have an extra wart where we split the API into single- and multiple- evaluating versions (I genuinely did not notice this had happened).

No one really seems to think Zephyr shouldn't be able to define MIN and MAX. No one really seems to think SOF shouldn't be able to use the same macros. The fact that Z_MAX has a Z on the front obscures the argument, I think. It's really not an "internal" API in any meaningful sense and Zephyr has no particular interests in preventing its use outside the tree.

Some options:

1. If SOF can tolerate the fact that Zephyr MIN/MAX() evaluate their arguments twice, then SOF can just use the Zephyr macros as is. Simply suppress the existing SOF macros when ZEPHYR and include sys/util.h in the file where they are defined.

2. If SOF needs single-evaluation MIN/MAX, then we could consider fixing this in the Zephyr API instead by making our MIN/MAX into the macros that are currently called Z_MIN/Z_MAX, and renaming the multiple-definition variant used in BUILD_ASSERT() (which is a very special case usage!) into Z_STATIC_MIN/MAX() or something. Then the solution in DMA trace output can contain repeated trace data blocks #1 becomes trivial. (This does introduce a "GCC syntax" dependency, though IIRC all our existing toolchains support GCC compound expressions, right?)

3. If neither is possible, then I guess SOF needs to rename its own MIN/MAX symbols to something unique, and define them to Z_MIN/Z_MAX when ZEPHYR instead of using its own definitions. But this seems like by far the most work, and mostly pointless.

[1] Properly pronounced "linux" in this context.

[marc-hb]

Thanks @andyross for the additional details and suggestions, much appreciated.

1. If SOF can tolerate the fact that Zephyr MIN/MAX() evaluate their arguments twice,...

I don't think anyone has the time to actually review every single use of theses macros in SOF and whether it's safe to "downgrade" them to multiple evaluation versions in every single place. Moreover there's a high risk of long time SOF contributors wrongly assuming these macros are safe wrt. multiple evaluations because it's actually the case with plain SOF and the tests are actually passing. Really not a fan of using the same name for different, incompatible implementations depending on the context.

2. we could consider fixing this in the Zephyr API instead by making our MIN/MAX into the macros that are currently called Z_MIN/Z_MAX, and renaming the multiple-definition variant used in BUILD_ASSERT() (which is a very special case usage!) into Z_STATIC_MIN/MAX() or something

I ran some quick and dirty west forall -c "git grep -n '[^_[:alnum:]]Z_MAX[:blank:]*('" and the compile-time friendly MAX and MIN macros seem massively more popular than the gcc run-time only Z_MAX and Z_MIN macros.

3. If neither is possible, then I guess SOF needs to rename its own MIN/MAX symbols to something unique, [...] But this seems like by far the most work, and mostly pointless.

That search and replace was some amount of work but it's already been done in this PR and it passed all the tests. Changing the name again is a small effort now.

3... and define them to Z_MIN/Z_MAX when ZEPHYR instead of using its own definitions. ...

SOF could keep its own SOF_MIN definitions even when ZEPHYR, no need for any smart pre-processor logic here, it would not add any value in this particular case. The fewer the layers of pre-processing indirections and logic always the better.

[lgirdwood]

All, direction of travel is SOF becomes a Zephyr module and hence we wrap then deprecate the xtos/HAL/SOF RTOS macros and APIs.
@marc-hb we need to use the Zephyr APIs and macros everywhere. This means we directly call them when we build with Zephyr RTOS and we use the xtos wrappers when we don't build with Zephyr (today we it's the other way around and we wrap Zephyr to look like xtos/SOF RTOS). This is @andyross option 1.
We will pick up any changes to performance or functionality as part of CI and/or weekly testing and can correct these before next SOF and Zephyr releases.

[marc-hb]

Decision is not to use the Z_MIN/Z_MAX macros because no one should (in fact very few do)

If SOF can tolerate the fact that Zephyr MIN/MAX() evaluate their arguments twice,...

I don't think anyone has the time to actually review every single use of theses macros in SOF and whether it's safe to "downgrade" them to multiple evaluation versions in every single place.

I took the time, submitted at #4119 that supersedes this. Closing.