enhancement: zephyr: build fixes/improvements (re-spin of 949) #1083
Conversation
|
PR missing one of the required labels: {'documentation', 'api-sync', 'breaking-change', 'ci', 'bug', 'enhancement', 'new feature', 'internal', 'dependencies'} |
Emil-Juhl
changed the title
|
Hmm, I see the ECA check is failing because authorship should be attributed to Tom, who is the actual author of the change. Shouldn't this check to instead validate me, as the committer? (Of course, I put my sign-off on the change as well already) |
|
I approve of this if at all helpful. |
zephyr/CMakeLists.txt
Outdated
| # Zenoh pico feature configuration options | ||
| set(FRAG_MAX_SIZE 4096 CACHE STRING "Use this to override the maximum size for fragmented messages") | ||
| set(BATCH_UNICAST_SIZE 2048 CACHE STRING "Use this to override the maximum unicast batch size") | ||
| set(BATCH_MULTICAST_SIZE 2048 CACHE STRING "Use this to override the maximum multicast batch size") | ||
| set(Z_CONFIG_SOCKET_TIMEOUT 100 CACHE STRING "Default socket timeout in milliseconds") | ||
| set(Z_TRANSPORT_LEASE 10000 CACHE STRING "Use this to override transport lease time") | ||
| set(ZP_PERIODIC_SCHEDULER_MAX_TASKS 64 CACHE STRING "Use this to override maximum number of periodic tasks") | ||
|
|
||
| set(Z_FEATURE_UNSTABLE_API 0 CACHE STRING "Toggle unstable Zenoh-C API") | ||
| set(Z_FEATURE_PUBLICATION CONFIG_ZENOH_PICO_PUBLICATION CACHE STRING "Toggle publication feature") | ||
| set(Z_FEATURE_SUBSCRIPTION CONFIG_ZENOH_PICO_SUBSCRIPTION CACHE STRING "Toggle subscription feature") | ||
| set(Z_FEATURE_QUERY CONFIG_ZENOH_PICO_QUERY CACHE STRING "Toggle query feature") | ||
| set(Z_FEATURE_QUERYABLE CONFIG_ZENOH_PICO_QUERYABLE CACHE STRING "Toggle queryable feature") | ||
| set(Z_FEATURE_LIVELINESS CONFIG_ZENOH_PICO_LIVELINESS CACHE STRING "Toggle liveliness feature") | ||
| set(Z_FEATURE_INTEREST CONFIG_ZENOH_PICO_INTEREST CACHE STRING "Toggle interests") | ||
| set(Z_FEATURE_FRAGMENTATION CONFIG_ZENOH_PICO_FRAGMENTATION CACHE STRING "Toggle fragmentation") | ||
| set(Z_FEATURE_ENCODING_VALUES CONFIG_ZENOH_PICO_ENCODING_VALUES CACHE STRING "Toggle encoding values") | ||
| set(Z_FEATURE_MULTI_THREAD 1 CACHE STRING "Toggle multithread") | ||
|
|
||
| set(Z_FEATURE_LINK_TCP CONFIG_ZENOH_PICO_LINK_TCP CACHE STRING "Toggle TCP links") | ||
| set(Z_FEATURE_LINK_BLUETOOTH 0 CACHE STRING "Toggle Bluetooth links") | ||
| set(Z_FEATURE_LINK_WS 0 CACHE STRING "Toggle WebSocket links") | ||
| set(Z_FEATURE_LINK_SERIAL CONFIG_ZENOH_PICO_LINK_SERIAL CACHE STRING "Toggle Serial links") | ||
| set(Z_FEATURE_LINK_SERIAL_USB 0 CACHE STRING "Toggle Serial USB links") | ||
| set(Z_FEATURE_SCOUTING CONFIG_ZENOH_PICO_SCOUTING CACHE STRING "Toggle UDP scouting") | ||
| set(Z_FEATURE_LINK_UDP_MULTICAST CONFIG_ZENOH_PICO_LINK_UDP_MULTICAST CACHE STRING "Toggle UDP multicast links") | ||
| set(Z_FEATURE_LINK_UDP_UNICAST CONFIG_ZENOH_PICO_LINK_UDP_UNICAST CACHE STRING "Toggle UDP unicast links") | ||
| set(Z_FEATURE_MULTICAST_TRANSPORT CONFIG_ZENOH_PICO_MULTICAST_TRANSPORT CACHE STRING "Toggle multicast transport") | ||
| set(Z_FEATURE_UNICAST_TRANSPORT CONFIG_ZENOH_PICO_UNICAST_TRANSPORT CACHE STRING "Toggle unicast transport") | ||
| set(Z_FEATURE_RAWETH_TRANSPORT 0 CACHE STRING "Toggle raw ethernet transport") | ||
| set(Z_FEATURE_TCP_NODELAY 1 CACHE STRING "Toggle TCP_NODELAY") | ||
| set(Z_FEATURE_LOCAL_SUBSCRIBER 0 CACHE STRING "Toggle local subscriptions") | ||
| set(Z_FEATURE_SESSION_CHECK 1 CACHE STRING "Toggle publisher/querier session check") | ||
| set(Z_FEATURE_BATCHING 1 CACHE STRING "Toggle batching") | ||
| set(Z_FEATURE_MATCHING 1 CACHE STRING "Toggle matching feature") | ||
| set(Z_FEATURE_RX_CACHE 0 CACHE STRING "Toggle RX_CACHE") | ||
| set(Z_FEATURE_UNICAST_PEER CONFIG_ZENOH_PICO_UNICAST_PEER_MODE CACHE STRING "Toggle Unicast peer mode") | ||
| set(Z_FEATURE_AUTO_RECONNECT CONFIG_ZENOH_PICO_AUTO_RECONNECT CACHE STRING "Toggle automatic reconnection") | ||
| set(Z_FEATURE_LINK_TLS 0 CACHE STRING "Toggle TLS link") | ||
| set(Z_FEATURE_BATCH_TX_MUTEX 0 CACHE STRING "Toggle batch tx mutex") | ||
| set(Z_FEATURE_BATCH_PEER_MUTEX 0 CACHE STRING "Toggle batch peer mutex") | ||
| set(Z_FEATURE_PERIODIC_TASKS 0 CACHE STRING "Toggle periodic task support") | ||
| set(Z_FEATURE_ADVANCED_PUBLICATION 0 CACHE STRING "Toggle advanced publication") | ||
| set(Z_FEATURE_ADVANCED_SUBSCRIPTION 0 CACHE STRING "Toggle advanced subscription") | ||
| set(Z_FEATURE_MULTICAST_DECLARATIONS 0 CACHE STRING "Toggle multicast declarations") |
There was a problem hiding this comment.
To enhance modularity and avoid redundancy, I propose that these feature configurations be encapsulated within a separate file, such as feature-config.cmake, located within the root directory. This file can subsequently be included from both the zephyr module and the primary CMakeLists.txt file within the same directory.
By implementing this approach, we eliminate the need to duplicate the list of feature configurations, thereby streamlining the development process and ensuring consistency across different modules.
There was a problem hiding this comment.
Very valid point. I didn't actually snoop around that much within the CMake chain for building without using the west module infrastructure.
I can see that this is basically done in the base cmake and can just be factored out into an include file as you suggest.
I've given it a shot.
As a preparation for sharing the zenoh-pico feature configuration logic between the Zephyr module build and the regular builds, factor out the feature configuration logic into a separate file `cmake/feature_config.cmake`. Then simply include this feature config file from the base CMakeLists. No functional changes intended. Signed-off-by: Emil Dahl Juhl <[email protected]>
Emil-Juhl
force-pushed
the
emil/teburd-cherry-pick
branch
from
3c71b3a to
e91df3c
Compare
|
PR missing one of the required labels: {'api-sync', 'internal', 'breaking-change', 'dependencies', 'ci', 'new feature', 'enhancement', 'bug', 'documentation'} |
|
@teburd after dealing with the suggestion from @anedergaard I have put myself as author on the changes here. I figure they differ a lot from your original patch. Please let me know if you take any issue with this! |
anedergaard
left a comment
anedergaard
left a comment
There was a problem hiding this comment.
Thanks @Emil-Juhl for implementing my suggestion. I validated it as a Zephyr module and also used the CMakefile in the root directory.
The standalone build chain relies on cmake generating various config files based off cmake variables and template files. The zephyr cmake chain did not follow this approach. Due to this, compiling zenoh-pico as a west module inside a zephyr project would for example lead to warnings about redefinitions of the various Z_FEATURE_ macros that zenoh-pico relies on for controlling the feature set. Update the zephyr CMakeLists to use the existing feature_config.cmake helper, which takes care of generating the feature configuration files based off the various cmake variables. Since certain features are accessible via. Kconfig options, the corresponding cmake variables are set prior to inclusion of the feature_config helper, effectively overriding the default values. In addition, the zephyr CMakeLists is updated to fetch the zenoh-pico version from the `version.txt` file in the root of the project. This version is then forwarded to another configuration file via. cmake. This mimics the behavior of the CMakeLists from the root of the project. Since the zephyr cmake chain runs with a different base directory than a standalone build, the feature_config.cmake is updated to rely on a ZENOH_PICO_INCLUDEDIR variable for locating the respective configuration files. The root CMakeLists is updated to set this appropriately. These changes were based off the draft pull-request with zephyr build fixes: eclipse-zenoh#949 Signed-off-by: Emil Dahl Juhl <[email protected]>
Extend the kconfig options for zephyr to include MULTICAST_TRANSPORT, UNICAST_TRANSPORT, UNICAST_PEER, AUTO_RECONNECT, and INTEREST in addition to the existing options. These additions, aside from INTEREST, were originally propose in the pull-request: eclipse-zenoh#949 Signed-off-by: Emil Dahl Juhl <[email protected]>
When compiling zenoh-pico with Z_FEATURE_MULTI_THREAD and Z_FEATURE_UNICAST_TRANSPORT enabled, while leaving Z_FEATURE_UNICAST_PEER disabled, the `_zp_unicast_process_peer_event` function will be defined but unused. Only compile the function, alongside its own additional helpers, if Z_FEATURE_UNICAST_PEER is enabled. Signed-off-by: Emil Dahl Juhl <[email protected]>
Emil-Juhl
force-pushed
the
emil/teburd-cherry-pick
branch
from
e91df3c to
2a1f7f0
Compare
|
PR missing one of the required labels: {'ci', 'internal', 'enhancement', 'api-sync', 'dependencies', 'documentation', 'breaking-change', 'bug', 'new feature'} |
|
I rebased, and added one additional fix for a compiler warning when compiling with MULTI_THREAD and UNICAST_TRANSPORT enabled while UNICAST_PEER is left disabled. (see commit message below or in the commit itself). Sorry, I don't normally try to put additional things on top of approved PRs, but since no one from the team with write access has looked yet, I figured I could do it this time. |
3 participants
An updated version of #949 from @teburd (hence the authorship in the commit).
Basically, this hooks the Zephyr compilation steps into the same templated-config-file flow as (I believe) the rest of the integrations use. Without this, using the standard
westtooling to compile a zephyr project with zenoh-pico as a module would lead to compiler warnings about redefined macros for the various feature flags such as the one below:The approach here is to add individual CMake variables for all of the zenoh-pico feature flags. Some of them will then reflect their corresponding Kconfig option, while others will just take on a default value and can be overridden by passing flags to cmake when building.The approach here is to move the feature configuration logic from the base CMakeLists into a helper file and then include that from both the base CMakeLists and the one inside the zephyr folder. The zephyr one will set some of the feature flags prior to inclusion so that the values match their kconfig counterpart.
In addition, this adds a couple of new Kconfig options that map to some zenoh-pico feature flags.