Navigation Menu

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

cpu/esp32: activate cpp feature #11922

Merged
merged 1 commit into from Jul 30, 2019

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jul 26, 2019

Contribution description

This PR activates the C++ feature for ESP32 boards. It simply adds the pthread module and libstdc++ to linked libraries to solve the problem of unresolved symbols when feature cpp is required.

Testing procedure

Compile and flash examples/riot_and_cpp, for example

make BOARD=esp32-wroom-32 -C examples/riot_and_cpp flash

to test the device with a C++ application.

Issues/PRs references

@gschorcht
Copy link
Contributor Author

@cladmi, @kaspar030 Since someone was asking for using cpp feature with ESP32, I tried to activate it. Principally, it is working when pthread module is used and libstdc++ is linked.

However, to avoid that module pthread and libstdc++ are used always even if they aren't needed, I tried to use them only, if feature cpp is required. Since there is no pseudomodule cpp, I just used FEATURE_REQUIRED in cpu/esp32/Makefile.dep:

ifneq (,$(filter cpp,$(FEATURES_REQUIRED)))
    USEMODULE += pthread
    BASELIBS += -lstdc++
endif

Unfortunatly, static tests fail with

Running './dist/tools/buildsystem_sanity_check/check.sh' x
Command output:
	Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
	Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
		cpu/esp32/Makefile.dep:ifneq (,$(filter cpp,$(FEATURES_REQUIRED)))

How should I realize this dependency? Or should I add module pthread and link libstdc++ always, even if they are not needed?

@gschorcht
Copy link
Contributor Author

@cladmi @kaspar030 If it isn't allowed to evaluate FEATURES_REQUIRED to determine dependencies, shouldn't the activation of FEATURE_REQUIRED=cpp not lead to enabling a pseudomodule cpp?

@cladmi
Copy link
Contributor

cladmi commented Jul 29, 2019

@gschorcht you should filter with FEATURES_USED. I put the info for this in the check.sh script, but it is for sure not the easiest place to find it.
At least the issue was detected automatically :)

# Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
# Handling specific behaviors/dependencies should by checking the content of:
# * `USEMODULE`
# * maybe `FEATURES_USED` if it is not a module (== not a periph_)

The issue with using FEATURES_REQUIRED, is that if cpp is enabled by an application with FEATURES_OPTIONAL, it would not match the condition.

FEATURES_USED has both.

FEATURES_USED = $(sort $(FEATURES_REQUIRED) $(FEATURES_OPTIONAL_USED))

@gschorcht
Copy link
Contributor Author

@cladmi Ah, thanks, I will try.

Adds module pthread and the libstdc++ to linked libraries to solve the problem with unresolved symbols when feature cpp is required.
@gschorcht
Copy link
Contributor Author

@cladmi Works in that way, thanks again.

@miri64 miri64 added Area: boards Area: Board ports Area: C++ Area: C++ wrapper Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 30, 2019
@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jul 30, 2019
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the CPP example application, and also the cpp11_* tests are working, on a esp32-wroom-32 board. ACK.

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 30, 2019
@leandrolanzieri leandrolanzieri merged commit 699fbc9 into RIOT-OS:master Jul 30, 2019
@gschorcht
Copy link
Contributor Author

@leandrolanzieri Thanks for testing and merging

@gschorcht gschorcht deleted the cpu/esp32/cpp branch July 30, 2019 21:53
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: C++ Area: C++ wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants