ESP-IDF CMakeLists #800

bakerstu · 2024-10-20T06:37:51Z

The goal of this pull request is to no longer require a separate OpenMRNIDF repository.

Provides a CMakeLists file such that openmrn can be included as a registered idf component as part of a standard ESP-IDF application build.
Provides an esp-idf specific include path with appropriate files linked to it.
Makes a few minor code tweaks to support the ESP-IDF build.

bakerstu · 2024-10-20T06:39:07Z

etc/esp-idf/CMakeLists.txt

+# Suppress compilation warnings in OpenMRN
+###############################################################################
+
+target_compile_options(${COMPONENT_LIB} PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-Wno-volatile>)


Does this need to be PUBLIC?

I opted for PUBLIC primarily to suppress a warning from Executor.hxx/cxx showing up if you include the hxx in your project. We might be able to get away with INTERFACE though, I'll check and confirm.

As noted elsewhere, we can reduce this to PRIVATE

This is required because of a warning generated by the usage of Executor::sequence_. We should revisit this at some point.

etc/esp-idf/CMakeLists.txt

atanisoft

Overall looks good.

One other warning that we should review:

[1112/1124] Building CXX object esp-idf/main/CMakeFiles/__idf_main.dir/main.cpp.obj
In file included from .../main/ThermalMonitor.hxx:20,
                 from .../main/main.cpp:55:
..../OpenMRNIDF/src/utils/Fixed16.hxx: In member function 'float Fixed16::to_float() const':
..../OpenMRNIDF/src/utils/Fixed16.hxx:274:17: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  274 |         return *reinterpret_cast<float *>(&f);

I'm not 100% certain how we should handle this one. This is coming up in ESP-IDF v5.3 (tested with v5.3.1 locally).

include/esp-idf/can_ioctl.h

atanisoft · 2024-10-20T11:51:11Z

etc/esp-idf/CMakeLists.txt

+###############################################################################
+# Suppress compilation warnings in OpenMRN
+###############################################################################
+target_compile_options(${COMPONENT_LIB} PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-Wno-volatile>)


with INTERFACE instead of PUBLIC the following warning is logged:

[880/1084] Building CXX object esp-idf/OpenMRNIDF/CMakeFiles/__idf_OpenMRNIDF.dir/src/executor/Executor.cpp.obj .../managed_components/OpenMRNIDF/src/executor/Executor.cpp: In member function 'virtual void* ExecutorBase::entry()': .../managed_components/OpenMRNIDF/src/executor/Executor.cpp:322:15: warning: '++' expression of 'volatile'-qualified type is deprecated [-Wvolatile] 322 | ++sequence_; | ^~~~~~~~~

We can reduce this to PRIVATE but we should revisit this specific file and evaluate how we can eliminate the warning entirely (gcc pragma option perhaps?)

Agree that we should revisit this.

atanisoft · 2024-10-20T12:15:42Z

etc/esp-idf/README.md

+cd <application path>/components
+ln -s <openmrn path>/etc/esp-idf/ openmrn
+```
+This will result in openmrn being compiled as a registered idf component with the name "openmrn".


We should also document using idf_component.yml which is the approach recommended by Espressif for dependency management.

A version of this could be:

dependencies: OpenMRN: version: {branch, hash, tag, etc} git: https://github.com/bakerstu/openmrn.git path: etc/esp-idf

The path line overrides where idf.py looks for the CMakeLists.txt within the referenced repository (it defaults to root of the repository).

If you include override_path it will skip looking at the repository and use a local copy instead, which can use environment variables, such as: ${OPENMRNPATH}/etc/esp-idf.

The version line is optional, when not present idf.py build will collect the latest git hash and record it in dependencies.lock. If there is a newer version available in git it will present the following during build:

-- Building ESP-IDF components for target esp32s3 NOTICE: Following dependencies have new versions available: Dependency "OpenMRNIDF": "5d7b0b3157e3411b3f71ccfacedaef6cb1202bff" -> "63e2b2e3f6ba34719f50194240113247793ac3ac" Consider running "idf.py update-dependencies" to update your lock file.

atanisoft · 2024-10-20T11:56:09Z

etc/esp-idf/CMakeLists.txt

@@ -0,0 +1,162 @@
+set(SRCS
+    $ENV{OPENMRNPATH}/src/dcc/dcc_constants.cxx


Instead of relying on $ENV{OPENMRNPATH} we likely should use ${CMAKE_CURRENT_SOURCE_DIR} to determine where the source files and include paths actually are relative to the CMakeLists.txt file.

if the CMakeLists.txt is not on the top level, then we would probably need an variable that denotes toplevel first.

${COMPONENT_DIR} can also be used for referencing the location of CMakeLists.txt

I'm actually not in favor of changing this. We have an established history of a top level "config" file that setups up environment variables for a repository. @balazsracz and I have also decided to use a Makefile for the top level project build that invokes CMake. We find this a bit more intuitive as it allows us to simply use the command "make" and then all the CMake magic happens under the hood.

...also, we have additional non-CMake related targets in our project makefile for doing special tasks such as creating "release" artifacts, etc.

In that case, OpenMRNIDF will continue to live on as it's not feasible to pass in env vars when using idf_component.yml to manage dependencies.

I'm revisiting this. It looks like ${COMPONENT_DIR} does not follow the symbolic link to the origin path. I'm exploring options.

@atanisoft I pushed a possible solution. Can you take a look?

etc/esp-idf/CMakeLists.txt

src/utils/SocketClient.cxx

etc/esp-idf/CMakeLists.txt

atanisoft · 2024-10-20T12:27:51Z

etc/esp-idf/CMakeLists.txt

+set(IDF_DEPS
+    app_update
+    bootloader_support
+    driver


I'm going to put this on my update list for ESP-IDF v5.3 support updates (hopefully a relatively small update), this driver component was split up and can be replaced with:

esp_driver_gpio

esp_driver_i2c

but there is currently a compilation failure in Esp32HardwareI2C.hxx due to a dependency on the older driver format. I'll take care of migrating to the newer driver code and cleanup of this specific line as optional based on ESP-IDF version (pre-5.3 it will include driver and current Esp32HardwareI2C code, 5.3+ it will depend on the newer I2C driver).

etc/esp-idf/CMakeLists.txt

atanisoft · 2024-10-20T12:39:40Z

etc/esp-idf/README.md

@@ -0,0 +1,7 @@
+# ESP-IDF Build Instructions


please consider adding an idf_component.yml in this directory with the following contents:

dependencies: idf: "<5.3"

This will restrict compilation of OpenMRN to ESP-IDF version 5.2.x or earlier by default until I can get a PR setup for the driver updates in ESP-IDF v5.3

We don't use the IDF component manager for several reasons:

it becomes impossible to compile the code when the espressif servers are not reachable over the internet (have you tried compiling code while you are on an airplane?)

the compilation becomes not deterministic from the git hash

Both of these are big problems for a professional project. Reproducible compilation results are essential for finding bugs and retroactive maintenance. We are talking about 10y+ of support needs in this product domain, not a 6-month product development cycle like many other domains.

Having said that, it should be possible to use ther component manager if someone wants it. It is just a requirement for the compilation and dependencies to work correctly if someone chooses not to use it.

Reproducible builds is a great call-out and has a rather simple solution. Limit your external dependencies (beyond esp-idf itself) AND embed the github link and hash (which typically can not change) and the build system will only use that specific version for external dependencies.

As for offline (airplane) usage, this is doable with small adjustments to the idf_component.yml and pre-staging the external dependencies locally. In the idf_component.yml file you would leverage the override_path or path element to shortcut the lookup of the component with a local path to where the dependency actually lives. You might need to remove version/git/etc elements though (I forget now on that).

Cloning the dependencies locally into a components tree (as git submodules) would also work for this, optionally adding the following snippet to the top level CMakeLists.txt file:

list(APPEND EXTRA_COMPONENT_DIRS "../common_components/Filesystem/")

This allows simplified dependencies mapping in component / main CMakeLists.txt files.

also note that the above snippet would ONLY check the ESP-IDF version and does not specifically deal with dependencies. If the ESP-IDF version check does not match the declared version it will abort the build with a message for the user to fix their ESP-IDF version selection.

@atanisoft I've added an idf_component.yml file. This causes a warning for my non-managed build, but we can choose to ignore it. Can you have a look and provide some feedback?

commented on the file above, we should just include the basic snippet listed above to only restrict the idf version check.

@bakerstu it looks like no variant of declaration in idf_component.yml will work correctly with the current version of the idf component manager that I can find. The path: etc/esp-idf bit tries to force the git checkout to that path rather than reference that path for where to find the top level directory.

I suspect it only applies to when publishing the component to their registry and not for when consuming it. For now let's drop the file and I'll see if I can find a suitable way to make it work correctly or log an issue for them to fix it

bakerstu · 2024-10-27T20:21:27Z

Overall looks good.

One other warning that we should review:

[1112/1124] Building CXX object esp-idf/main/CMakeFiles/__idf_main.dir/main.cpp.obj
In file included from .../main/ThermalMonitor.hxx:20,
                 from .../main/main.cpp:55:
..../OpenMRNIDF/src/utils/Fixed16.hxx: In member function 'float Fixed16::to_float() const':
..../OpenMRNIDF/src/utils/Fixed16.hxx:274:17: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  274 |         return *reinterpret_cast<float *>(&f);

I'm not 100% certain how we should handle this one. This is coming up in ESP-IDF v5.3 (tested with v5.3.1 locally).

Probably this is intentional. If so, we should consider an inline disabling of this warning using the push/pop method.

atanisoft · 2024-10-27T20:35:36Z

etc/esp-idf/idf_component.yml

@@ -0,0 +1,6 @@
+## IDF Component Manager Manifest File
+dependencies:
+  OpenMRN:


Close, we can comment out this OpenMRN section as it would create a circular dependency which may trip up the dependency logic in esp-idf.

atanisoft · 2024-10-27T20:39:24Z

src/freertos_drivers/esp32/Esp32Gpio.hxx

@@ -35,7 +35,7 @@
 #ifndef _DRIVERS_ESP32GPIO_HXX_
 #define _DRIVERS_ESP32GPIO_HXX_

-#include "freertos_drivers/arduino/GpioWrapper.hxx"
+#include "freertos_drivers/common/GpioWrapper.hxx"


should we update arduino/libify.sh since it handles copying from common -> arduino? ref

Looks like these files are already handled in arduino/libify.sh.

in libify they are copied to arduino from common.

let's track this via an issue to unify the libify bits so we can avoid ifdef etc

bakerstu added 5 commits October 20, 2024 01:03

CMake build.

12870e9

Fix file extension.

f752bb1

Cleanup CMakeLists.

a8287d4

Fix compiler warning (redundant move in return statement).

a21ec31

Add readme instructions.

9a3209e

bakerstu requested review from balazsracz and atanisoft October 20, 2024 06:37

bakerstu commented Oct 20, 2024

View reviewed changes

bakerstu added 2 commits October 20, 2024 01:40

Remove extra newline.

2d0d242

Cleanup target compile options with comments.

508047f

bakerstu commented Oct 20, 2024

View reviewed changes

etc/esp-idf/CMakeLists.txt Outdated Show resolved Hide resolved

atanisoft reviewed Oct 20, 2024

View reviewed changes

bakerstu added 5 commits October 27, 2024 12:11

Merge branch 'master' into bakerstu-esp-idf-cmake

549d68f

Add BLE files to the build.

3557d7d

Apply review comments.

8373152

Add idf_component.yml file.

0659958

Remove "root" marker from the file system path.

1129d56

bakerstu requested a review from atanisoft October 27, 2024 20:21

atanisoft reviewed Oct 27, 2024

View reviewed changes

Remove idf_component.yml

d100f88

bakerstu merged commit 36833d8 into master Nov 1, 2024
4 checks passed

bakerstu deleted the bakerstu-esp-idf-cmake branch November 1, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESP-IDF CMakeLists #800

ESP-IDF CMakeLists #800

bakerstu commented Oct 20, 2024

bakerstu Oct 20, 2024

atanisoft Oct 20, 2024

atanisoft Oct 21, 2024

bakerstu Oct 27, 2024

atanisoft left a comment

atanisoft Oct 20, 2024

bakerstu Oct 27, 2024

atanisoft Oct 20, 2024

atanisoft Oct 20, 2024

balazsracz Oct 21, 2024

atanisoft Oct 22, 2024

bakerstu Oct 27, 2024

bakerstu Oct 27, 2024

atanisoft Oct 27, 2024

bakerstu Oct 27, 2024

bakerstu Oct 27, 2024

atanisoft Oct 20, 2024

bakerstu Oct 27, 2024

atanisoft Oct 20, 2024

balazsracz Oct 21, 2024

atanisoft Oct 21, 2024

atanisoft Oct 21, 2024

bakerstu Oct 27, 2024

atanisoft Oct 27, 2024

atanisoft Oct 27, 2024

bakerstu commented Oct 27, 2024

atanisoft Oct 27, 2024

atanisoft Oct 27, 2024

bakerstu Nov 1, 2024

atanisoft Nov 1, 2024

atanisoft Nov 1, 2024

		@@ -0,0 +1,162 @@
		set(SRCS
		$ENV{OPENMRNPATH}/src/dcc/dcc_constants.cxx

ESP-IDF CMakeLists #800

ESP-IDF CMakeLists #800

Conversation

bakerstu commented Oct 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atanisoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakerstu commented Oct 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment