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

Provide macros for switching off Zephyr kernel version #37850

Closed
yperess opened this issue Aug 20, 2021 · 14 comments
Closed

Provide macros for switching off Zephyr kernel version #37850

yperess opened this issue Aug 20, 2021 · 14 comments
Labels
Enhancement Changes/Updates/Additions to existing features

Comments

@yperess
Copy link
Collaborator

yperess commented Aug 20, 2021

Is your enhancement proposal related to a problem? Please describe.
When writing common code for multiple apps it is possible that different apps will be compiled against different Zephyr versions. Switching off KERNEL_VERSION_MAJOR and KERNEL_VERSION_MINOR is needed.

Describe the solution you'd like
I would like there to be a standardized way of doing this. It would look something like:

#define IS_ZEPHYR_VERSION_EQ(major, minor)                                                                   \
    (KERNEL_VERSION_MAJOR == (major) && KERNEL_VERSION_MINOR == (minor))

#define IS_ZEPHYR_VERSION_GT(major, minor)                                                                   \
    (KERNEL_VERSION_MAJOR > (major) ||                                                                       \
    (KERNEL_VERSION_MAJOR == (major) && KERNEL_VERSION_MINOR > (minor))

#define IS_ZEPHYR_VERSION_GTE(major, minor)                                                                  \
    (IS_ZEPHYR_VERSION_GT(major, minor) || IS_ZEPHYR_VERSION_EQ(major, minor)

#define IS_ZEPHYR_VERSION_LT(major, minor)                                                                   \
    (KERNEL_VERSION_MAJOR < (major) ||                                                                       \
    (KERNEL_VERSION_MAJOR == (major) && KERNEL_VERSION_MINOR < (minor))

#define IS_ZEPHYR_VERSION_LTE(major, minor)                                                                  \
    (IS_ZEPHYR_VERSION_LT(major, minor) || IS_ZEPHYR_VERSION_EQ(major, minor))

Describe alternatives you've considered
Two alternatives were considered:

  1. Do nothing and let each application just use the KERNEL_VERSION_* values.
  2. Provide only a GTE option and rely on else statements to account for the rest of the logic.
@yperess yperess added the Enhancement Changes/Updates/Additions to existing features label Aug 20, 2021
@stephanosio
Copy link
Member

stephanosio commented Aug 20, 2021

When writing common code for multiple apps it is possible that different apps will be compiled against different Zephyr versions.

I am not sure if that is something supported and/or we should support. Zephyr "apps" are too closely coupled to the kernel (or core OS) for that to be even remotely feasible.

@yperess
Copy link
Collaborator Author

yperess commented Aug 20, 2021

An example from Chromium's source while we were migrating from 2.5 to 2.6...

#if IS_ZEPHYR_VERSION(2, 6)
	struct espi_evt_data_acpi *acpi = (struct espi_evt_data_acpi *)&data;

	return acpi->type;
#elif IS_ZEPHYR_VERSION(2, 5)
	return (data >> NPCX_ACPI_TYPE_POS) & 0x01;
#endif

We could obviously introduce these macros in our own tree. I just wanted to see if that's something anyone else might care to have available first.

@brockus-zephyr
Copy link
Member

I like the idea of being able to generically code for API changes while continuing to support multiple version. If we had a product that would only be on a single version of zephyr then I would not see the need but that is not our case. We could have a branch that ties a product to v2.5 and another with v2.6 with both using common code, where the common code is where the zephyr API change affected, that may not be compatible.

@stoberblog
Copy link

I asked for this back in January (#31169) and people seem to not see that we too are developers and can actually work our way around the different API changes which is the exact reason why we want this change in the first place.

@yperess
Copy link
Collaborator Author

yperess commented Aug 22, 2021

Seems like as a macro it doesn't hurt anyone, some people will use it, others won't. Any suggestions on the proposed macros? Should I go ahead with a PR?

@stoberblog This gets harder in Kconfig. I'm not sure it's possible programmatically. Normally you can add more Kconfig files to the CONF_FILE list before loading the Zephyr module. Since this is done before loading the module, you wouldn't know which version of Zephyr you're building against yet so you can't split up your version specific Kconfig files there. I think that the best you'd be able to do is:

  1. Create a new Kconfig.version which will have a choice and manually add each version you support in your common code.
  2. Add a dependency for the Kconfigs you want to switch off of on these.

I'm not a fan of that approach, but I don't know that there's a better alternative for Kconfigs.

@yperess
Copy link
Collaborator Author

yperess commented Aug 26, 2021

@stephanosio any thoughts? Do you think this is something we'd want in Zephyr?

@stephanosio
Copy link
Member

@stephanosio any thoughts? Do you think this is something we'd want in Zephyr?

@yperess For some reason, I was under the impression that you were going to link compiled apps/libraries targeting different Zephyr versions together (e.g. an app targeting Zephyr 2.7 linked against a library targeting Zephyr 2.6) -- clearly I did not read this carefully enough.

I can see the value of having these macros in certain use cases, so no objections.

@gregshue
Copy link

Do you think this is something we'd want in Zephyr?

We already need this in Zephyr. I am facing a similar type of problem, but it is due to building against different Zephyr forks (upstream vs NCS) and it impacts source, Kconfig, CMakeLists.txt, tests, documentation, and even manifests.

When writing common code for multiple apps it is possible that different apps will be compiled against different Zephyr versions.

I am not sure if that is something supported and/or we should support. Zephyr "apps" are too closely coupled to the kernel (or core OS) for that to be even remotely feasible.

It is essential for any project with a support lifetime that spans LTS releases. It is also quite feasible with a bit of engineering. Consider:

  • "projects" are only functional configurations and settings.
    Expecting that every piece of logic will eventually get reused in some other project, refactoring will push that logic out of the project src/ directory into a subsystem, driver, or library. This leaves each project with a trivial main.c that only contains an empty void main(void){} and SYSINIT hooks to register settings which aren't configured through Kconfig.

  • All global namespaces (Kconfig, include pathnames, C preprocessor symbols, testcase identifiers, board names, subsystems, global variables, function names, linker symbols, DTS aliases, etc) already need to be managed across the ecosystem to support REUSING released code/tests/documentation from multiple places, along with the safety test artifacts available for them. This naming pattern should also consider if/how to support changes introduced in downstream forks. In general this seems to mean adding a prefix(?) to each of these symbols indicating the organization + repository/module/fork that introduced the symbol and manages its definition/usage.

  • Logic is designed around RTOS concepts (e.g., preemptively scheduled threads, event driven designs), and implemented to call APIs to those services. As long as the RTOS concepts are pretty stable it is straight forward to adjust which API version is called at build time. (NOTE: This should be API-version-specific, not Kernel version specific.)

  • Downstream forks will naturally want to extend/patch the upstream content rather than change it ... since it will need to be able to pull in security fixes at any time. This means it may be easier to extend functionality using a new module rather than touching the fork of the upstream repo.

  • Documentation, tests, and samples need to be in the same module as the interfaces/drivers/subsystems they describe. Documentation structure and directory trees need to be in a consistent pattern so that the doc generation scripts can pull them all together into one doctree from across all the modules. (I assume documentation needs to be built for the entire workspace. It may be important for some to have documentation build for just an individual project.)

  • Twister needs to be able to skip tests/samples not supported by this fork/version, so we will also need to extend the testcase schema and yml files.

  • Separate top-level manifests are needed due to the different forks and/or versions. By using west's --mf <pathname> option I already use different manifests for different forks. This same mechanism could let me use different manifests for different kernel versions, or even different manifests for different projects (to ensure specific modules CANNOT be in a project build).

As you can see, this is a pervasive issue that is really unavoidable.

Should the conditionals/prefixes/etc be based off of the Zephyr kernel version value? Probably not. Some forks have not kept aligned with the Zephyr release numbers. There will also be logic that needs is conditional on a module version rather than the kernel version. Also note that module versions will be controlled by the manifest, which may be outside of the user's Zephyr repository.

@yperess
Copy link
Collaborator Author

yperess commented Aug 30, 2021

I need to think about that last one a bit more. My gut reaction is to say that if you're building against a fork then it's on the application/library developer to manage that. Seems to me that you should add a KERNEL_FLAVOR or something like that into your build system.

The concern regarding Kconfig/CMake is a very sound one and I'm actively thinking about it. CMake should be easy, these values are defined in cmake/version.cmake. So we can just add functions to cmake/extensions.cmake. I'll send out PRs for both C and CMake tonight and will think about Kconfig some more.

@yperess
Copy link
Collaborator Author

yperess commented Aug 31, 2021

So I just came across https://cmake.org/cmake/help/latest/command/if.html#version-comparisons when looking for a CMake solution. This exists in KERNEL_VERSION_STRING so you could do something like:

if ("${KERNEL_VERSION_STRING}" VERSION_GREATER_EQUAL "2.6")
  # Do stuff...
endif()

I believe that this solves the CMake issue.

@yperess
Copy link
Collaborator Author

yperess commented Aug 31, 2021

(@gregshue, @stoberblog) For Kconfig, I have an idea...

The issue is that we run find_package(Zephyr ... to load the Zephyr CMake package and at that point all the Kconfigs need to be set so it's too late to switch off of the Zephyr kernel version. So two options exist AFAICT:

  1. We would want to split the Zephyr package into 2: Zephyr and ZephyrVersion. We can then find_package(ZephyrVersion ... and set the CONF_FILE programmatically in CMake before loading the full Zephyr package.
  2. We use https://cmake.org/cmake/help/latest/command/find_package.html#version-selection. Basically if you want a specific Kconfig for v2.6 and a different one for v2.5 you make the latest ToT named prj.conf and you run find_package to find v2.6 or greater. If that fails, you replace CONF_FILE and run find_package again for v2.5. Rinse and repeat.

Option 1 is a bit easier for developers IMO, but option 2 has built-in support (we should probably document it to make sure devs follow that pattern). Thoughts? I'm leaning towards option 2 and just documenting it better.

@gregshue
Copy link

My gut reaction is to say that if you're building against a fork then it's on the application/library developer to manage that. Seems to me that you should add a KERNEL_FLAVOR or something like that into your build system.

Since I'm REUSING zephyrproject-rtos/zephyr and Nordic's zephyr fork, the goal is to not add anything to the build system(s) that are provided. Living under a reuse paradigm is very different than a leverage one!

IIRC, the next thing to be aware of with the Nordic fork is Zephyr release tags and associated SHAs are not always in the downstream fork. I think there was also something unexpected with kernel and project numbering - like ending in a .99 or using a possibly contradictory version numbering assignment. This variance may quickly lead to cluttered source.

I have learned that some users have put their application module in a directory shared across multiple VMs (one per release/fork). I am attempting to build in one VM two versions per fork. Please keep in mind that we can't guarantee the forks use non-conflicting versions.

yperess pushed a commit to yperess/zephyr that referenced this issue Aug 31, 2021
Issue zephyrproject-rtos#37850
Add ==, >, >=, <, <= macro comparators for the Zephyr version to enable
conditional code based on the kernel version.

Signed-off-by: Yuval Peress <peress@chromium.org>
@yperess
Copy link
Collaborator Author

yperess commented Aug 31, 2021

@nashif pointed out that this is already possible via:

#if KERNELVERSION >= ZEPHYR_VERSION(2,6,0)
....
....
#endif

I would say that the only thing left to do is to add these 3 approaches to the documentation?

  1. In code #if...
  2. In CMakeLists.txt via trying to load specific versions of the Zephyr package or checking using "${KERNEL_VERSION_STRING}" VERSION_GREATER_EQUAL "2.6.0"
  3. In Kconfig by using This is a test commit #2 above and trying to load specific versions of the Zephyr plugin and updating CONF_FILE accordingly.

@yperess
Copy link
Collaborator Author

yperess commented Sep 18, 2021

I believe since this is all possible now, we can close this issue.

@yperess yperess closed this as completed Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

5 participants