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

All architecture-specific files isolated in single folder #6370

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RavilN
Copy link

@RavilN RavilN commented Mar 27, 2024

Now custom architecture-specific source files can be located in one isolated folder. For default architectures (win32 and posix), it is the folder arch/eventloop_posix. For custom architectures, this folder could be under the arch folder for public code, or outside of the open62541 source code tree for private code. The location of the folder with architecture-specific files can be set using the new build option UA_ARCH_FOLDER. Default architecture-specific files under the folder arch/eventloop_posix are renamed, they start with "eventloop_arch_ ". This way no need to modify the list of files in the file CMakeLists.txt for custom architecture build.

…be located outside of the open62541 source code tree. Folder location can be set using new build option UA_ARCH_FOLDER. Architecture-specific files renamed, they start by "eventloop_arch_ ".
@jpfr
Copy link
Member

jpfr commented Mar 28, 2024

It is a good idea to make the architecture porting easier / cleaner.

Two remarks.

The POSIX EventLoop shouldn't be renamed to generic "arch". It heavily relies on the POSIX socket API.
Everything that deviates from that (like the lwip raw API) should do a fork and not share code with the POSIX implementation.
What can be potentially shared is already in the eventloop_common folder.

We can put additional CMakeLists.txt files in the /arch/eventloop_xxx folders.
That way, from the main CMakeLists.txt we would just include the correct folder.
And all the additional complexity is encapsulated there.

@RavilN
Copy link
Author

RavilN commented Mar 28, 2024

The reason to rename arch\eventloop_posix\eventloop_posix.h to arch\eventloop_posix\eventloop_arch.h was because plugins\crypto\mbedtls\ua_mbedtls_create_certificate.c was including it with path arch\eventloop_posix\eventloop_posix.h, which is architecture-specific. What I am not sure about is: whether this file must be included for all architectures. I assumed that each architecture needs to implement its version of this file, so for the freertos it would be like arch\eventloop_freertos\eventloop_freertos.h. In this case we would need to change file plugins\crypto\mbedtls\ua_mbedtls_create_certificate.c when new architecture is added. To avoid such changes, one of the ways would be to have the file name the same for all architectures, and include it only by file name, which is architecture-neutral, like eventloop_arch.h, where arch indicates it has architecture-specific code. To make the compiler find it, path to the architecture-specific files is added on include directories using the new option UA_ARCH_FOLDER.

If this file needs always to be included, then I think it can be moved to the folder eventloop_common.

@jpfr
Copy link
Member

jpfr commented May 22, 2024

The file plugins\crypto\mbedtls\ua_mbedtls_create_certificate.c should not require the POSIX eventloop includes.
It generates a certificate as a bytestring...

That being said, my two remarks from above still hold.
Please update the PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants