-
Notifications
You must be signed in to change notification settings - Fork 1k
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
CMake Support #676
Comments
Yes please. BTW, a PR is always welcome. |
Just got back from an anniversary trip and catching up on work. I'll give an example within the next couple days. It will take some time, but shouldn't be too bad. In the meantime, look at https://github.com/lefticus/cpp_starter_project and how that CMakeLists.txt is setup. Pay attention to the use of |
I can think of a couple reasons to switch to CMake:
Reason 1 is more important because we can have doxygen elevate warnings to errors (using Doxyfile's WARN_AS_ERROR tag) and immediately fail generating docs. Without failing fast on warnings, the doxygen build action I put together will always result in "passed" if no actual errors are encountered. Doxygen however also treats *.txt files as C source code, but we can explicitly force CMakeLists.txt to be parsed like a python file because the comment syntax is the same (actual CMakeLists.txt content will have no bearing on documentation if the comments are done in a way that doesn't trigger doxygen's parsing). |
This is in the works. Going slower than I intended. |
No rush. I really appreciate you taking the initiative. Remember, good work is never finished...
We have contributing guidelines, but I'm not sure how much the NASA C-style guide or PEP8 would apply to CMake. I wouldn't worry about the Doxygen markup (essentially its Markdown syntax with some special inline cmds like |
CMakefiles have their own style usually. We can just agree on spaces, not tabs right now and leave the rest for later. |
I'm still exploring the CMake docs... The generation of the /utility/includes.h file from the /configure bash script might be rendered obsolete because I see we can use CMake to specify which extra support folder (from /utility directory) to include in the build process, but /RF24_config.h would need to compensate for this idea. I'm still brainstorming here; I have yet to try CMake out. |
That sounds like it would be a good way. I lost all the stuff I had done.
I'll try to get something up there within the next two weeks, though. The
basics weren't complicated.
…On Mon, Mar 22, 2021, 7:41 AM Brendan ***@***.***> wrote:
I'm still exploring the CMake docs... The generation of the
*/utility/includes.h* file from the */configure* bash script
<https://github.com/nRF24/RF24/blob/1187c56e4820785c1464062856ff783cd8d26d86/configure#L457>
might be rendered obsolete because I see we can use CMake to specify which
extra support folder (from */utility* directory) to include in the build
process, but */RF24_config.h* would need to compensate
<https://github.com/nRF24/RF24/blob/1187c56e4820785c1464062856ff783cd8d26d86/RF24_config.h#L53>
for this idea. I'm still brainstorming here; I have yet to try CMake out.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKBR4I75N7RPNPXCO6GDPLTE4UE5ANCNFSM4TM4PXMA>
.
|
@AndruePeters if you want, I can create a branch called "use-cmake" |
@AndruePeters if you want, I can create a branch called "use-cmake"
Yes please.
|
The "use-cmake' branch is created. I just found out that CMake can take in a config.h.in, replace @embedded_variables@ with CMake's variables' values (including ones commented out), and output the result as config.h. This would actually be better than modifying the RF24_config.h manually (like I mentioned above). Still learning here, but I'm determined enough to try this stuff out (on a separate/simple repo). I'm still not that much help (obviously), but it doesn't sound like you need it. |
Thanks. I'll start working there on it there. I'll take a look at that file
you were mentioning. You think it's okay if it doesn't support SSH builds
at this point?
…On Thu, Mar 25, 2021, 6:40 AM Brendan ***@***.***> wrote:
The "use-cmake' branch is created.
I just found out that CMake can take in a *config.h.in
<http://config.h.in>*, replace @embedded_variables@ with CMake's
variables' values (including ones commented out), and output the result as
*config.h*. This would actually be better than modifying the
RF24_config.h manually (like I mentioned above).
Still learning here, but I'm determined enough to try this stuff out (on a
separate/simple repo).
I'm still not that much help (obviously), but it doesn't sound like you
need it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKBR4NDD3F5VMEGKOSSLWDTFMHJVANCNFSM4TM4PXMA>
.
|
Yes, we can patch it up later. Personally, I think it's useful, but I don't like that it requires disabling security measures |
Sweet. I'm on this now. Got it forked and working on it. Sorry about all the delays. Full time job + grad school + family = crazy busy. |
Is there a reason there's currently no source directory? I'm mostly curious on the decisions for the project structure. |
Because the lib is so old... Using a src directory wasn't compatible with older versions of the Arduino IDE.
That's so good to hear. I almost pinged you yesterday because I'm trying to incorporate examples for the Pico SDK (which uses CMake), and I have a vague idea of how to do it. Mostly concerned about building for Linux vs building for Pico SDK. |
The approach I'm taking is fairly flexible. It's mostly as simple as defining a variable that checks for what version and then adds the proper files. I know nothing about Pico SDK, but I think this approach should work. I'll try to have a basic example done by Friday. |
we just merged in a new feature to master (it shouldn't affect Linux builds - excluding work on the python wrapper). @AndruePeters could you make a commit to your fork (I assume the use-cmake branch would be most applicable), so I can at least see your direction. Maybe I can mimic the same direction on the rp2xxx branch (for Pico SDK support). The work doesn't have to be complete; I guess I'm just not good with "teasers". |
I just pushed to my user branch. CMake itself is configuring everything. I'm still testing the actual build process, though. Currently to run: By making it work this way, it allows for device autoconfiguration like the script itself does. You can look in utility/CMakeLists.txt for how it's currently being handled. |
@kripton, please have a look at @AndruePeters fine work. This is what I was originally trying to do on the rp2xxx branch. @AndruePeters I'm getting a build error at this line in RF24_config.h. I still propose the idea to modify this line so that CMake will automatically adjust this when copying the file into the binary source tree. Or maybe there's a better way (I haven't tested this idea yet). CMakeLists.txt
link to CMake docs about RF24_config.h#include "utility/@RF24_DRIVER@/RF24_arch_config.h" Its hard to decide what value the See #642 also as the absence of the arm-linux-gnueabihf compiler nullifies some of the current CCFLAGS being set in the /configure script (exemplified in this comment's posted output). |
That's an easy fix. I'll get that up before midnight probably. |
I can make it default to SPIDEV. The current design allows a wide range of flexibility with how RF24_DRIVER gets set. I can implement the config script's behavior from within CMake and have it also default based upon the chip. I can fix the build error just fine like I mentioned in the previous comment. However, I don't completely follow what you mean about modifying the line for copying into the binary source tree. Can you explain? |
Its preformed by CMake's
I figured as much. I think we could also make use of CMake's |
Oh. Gotcha. I don't think it's necessary here if this is the only reason for wanting it. I think it will bring more confusion that anything. The build error was mostly an oversight from my part. Speaking of the |
@AndruePeters The problem is that /utility/includes.h doesn't exist (on github - see gitignore). If the user never runs the /configure script, then the includes.h file never gets copied from the appropriate driver dir in /utility. So, I'm not confident that simply including the /utility dir is the right/full fix. Did I understand that fix correctly?
Yes, RF24 lib still supports it indiscriminately (as to which unofficial fork is installed - our workflow used CoRfr/wiringPi). I remember a while back that @TMRh20 mentioned one of the network libs (RF24Gateway) doesn't support WiringPi.
Its not gone; its been re-incarnated by a multitude of offshoots. It seems Gordon's attempt to retire the lib was marginally ignored 😞 . |
Current "use-cmake"-branch on top of current "rp2xxx"-branch and modified to make the two approaches (building a shared library for Linux and including RF24 into a pico-sdk based project) work: #727 (comment). Sorry for cross-posting, just wanted to show that it's possible and as a response to #676 (comment) ;) By the way: nice work @AndruePeters. |
Erroneous output posted by @kripton in #727 (comment):
@AndruePeters this is the error I was referring to in #676 (comment). Although, the line number is probably off a little because its using the rp2xxx branch's RF24_config.h. |
Yeah. I'll take a look at these. Please critique this idea. I don't think that the user should necessarily have to run a configure script, and I'm trying to think of a decent design that does not require the configure script. My view is that everything should be handled from within CMake, and that the user can define the proper CMake Variables (or default to auto config) within their own project. So, if I was in a project for some Linux project using RF24, I'd probably expect to I implemented CMake at my job where we have a lot of custom code generation scripts that are ran. I think we should find a way to remove the utility file completely, or just see if you'd be okay with creating a script that only creates this file. That script can be called by CMake as part of the compilation or generation process. For the complexity of my job, a combination of both was used. For this, the latter might be sufficient. I just like removing unnecessary moving parts as needed. Thoughts? |
I recently looked into having 2 differently named CMakeLists.txt files (for the unreleased RF24Log lib), and it turns out CMake doesn't allow you to pick a different name for the CMakeLists.txt files. The suggested alternatives (found when googling) were:
@kripton I like the tactic you implemented in the top-level CMakeLists.txt file (involving |
No specific reason to change the approach. I just thought that it might be cleaner since the two styles to build/use the library are totally independent. So of course we can also leave it with the About the names: It would not matter if the name of the cmake-file for pico-sdk based projects is not CMakeLists.txt. That only matters if you actually call But as I said, the solution I proposed is still perfectly fine with me :) |
@AndruePeters I took another stab at building the lib on your fork's "use-cmake" branch, and I got it to work by changing a coupe things.
I also tested the old Our next (and hopefully final) challenge will be writing install rules and getting the examples_linux folder to build. Currently, the
I haven't been able to mimic this output using CMake. Although, I don't think versioned symlinks are a detrimental requirement, but I could be wrong because of users updating their RF24 lib's installation... Reminder: We need to run |
I just figured out that
Still trying to find out how to create a major+minor versioned symlink (librf24.so.1.4) in addition to the symlinks listed above... |
I've been toiling away at getting the examples to compile with CMake using the installed symlink(s)... I forgot about all the special compiler flags that are implemented in the /configure script. These flags should be added conditionally based on
Additionally, I couldn't get the |
Also forgot to mention in my last comment that the interrupt.c sources don't compile with a C compiler because it eventually The compiler traced the includes (using SPIDEV as the utility driver but also applies to RPi utility driver): Changing the suffix from interrupt.c to interrupt.cpp and setting the LANGUAGES property to |
ok, I've got the RF24 lib building and installing almost exactly like the current build system does. The @AndruePeters Could you look over the rp2xxx branch and tell me if I did anything unconventional or wrong. The biggest modifications are in the /utility/CMakeLists.txt and the top-level CMakeLists.txt. I'm thinking about making another .cmake module to compartmentalize the CPU detection and set compiler-friendly flags accordingly (see paragraph below). direct solution to #642The only thing left that I want to figure out before a PR is how to determine which GNU compiler CMake has selected. If its using the arm-gnu compiler, it still shows as GNU in the CMAKE_COMPILER_ID var. However, the current configure script injects optimization flags to the compiler (which at the time assumed using the arm-gnu compiler), but these optimization flags are not accepted by the x86/x64 GNU compiler (which is currently shipped with Ubuntu when running on a RPi). |
🤦🏼♂️I forgot about the python wrapper's install method which relies on the configure script's generated Makefile.inc for compiler flags and lib version number. The docs say that the RF24 lib needs to be fully installed (symlink and all) before installing the python wrapper, but I have my doubts about that because the setup.py file invokes the compiler via python's std libs. Hmm We could just mandate that the lib be built traditionally (using configure script) for installing the python wrapper until I've had time to explore pybind11 more intensively (tagging nRF24/pyRF24#3). |
Here is a useful link to the CMake FAQ about selecting a compiler; this is a must-read because having CMakeLists.txt force using a specific compiler (via CMake's |
Hey, I hope I've given enough for y'all to take and work on. I've realized I currently just don't have enough time to properly work and test these changes. Engineering job + family + grad school, etc. As long as this thread isn't closed, I can follow and comment on things as needed. But, I just don't have the time to do the work (despite being the one to open the issue). This book is a great resource on using modern CMake. The only design critique I have was regarding the location of the utility.h file. I can't seem to find the comment addressing that. I think the best fix it to keep it in CMAKE_BINARY_DIR as it keeps a clean build that way. The fix is just to add CMAKE_BINARY_DIR to the includes for that particular target. |
I didn't see anything with a cursory glance. Take a long at my previous post on here for a good book recommendation. |
About integrating remote installation of the RF24 lib (from host machine to target machine)
link to CMake docs about cross-compiling with Linux as the host and arm as the target @AndruePeters I think if I figure out how to specify where the linker looks for cross-compiling symlinks (& installation headers like with /usr/local/include/RF24), we can at least have a set of files to copy over remotely with ssh (probably using a separate bash script with elevated sudo privileges). On the other hand, if we host our own PPA repo that aids in distributing RF24 libs (via |
@AndruePeters Is the Doxygen.cmake module more useful to end-users? It certainly isn't useful for us because
I'm leaning toward removing this Doxygen.cmake module, but I'm trying to imagine a use case for end-users (in a similar convenience that the testing/fuzzing options provide) - maybe for outputting PDF, XML, or latex. Alternatively, we could modify the Doxygen.cmake module to execute the Concerning the tests folderThe current /tests folder is an unused relic from the ManiacBug dev era (prior to 2014), and it is only meant for the Arduino platform. Furthermore, the /tests folder may be broken since Arduino CLI doesn't use jam files and doesn't support the pde file format. I'm mentioning this because I noticed the cmd @TMRh20 I'm removing the /tests folder (on the rp2xxx branch) to avoid confusion with the CMake implementation's optional testing mechanisms. |
YEAH! I got the python wrapper to install from a CMake install of the RF24 lib 💯 . I also modified setup.py to check if the RF24 lib is installed and raise an exception if the /usr/local/lib/librf24.so file isn't found. The new exception prompt will tell users to install the RF24 lib first. This should avoid some confusion, but the Reminder: the python wrapper cannot be cross-compiled when the RF24 lib is installed with CMake. |
ok, I've employed a toolchain file in CMake to cross-compile the lib with ease. Currently, only the arm platform is configured for cross-compiling, but more toolchain files can easily be added with some Copy-n-Paste skills. I've also created a new "dpkg-deb" workflow to package the lib for arm platform, but it currently only compiles the lib using I encourage anyone to download the saved artifacts from the workflow and try installing the deb file on a RPi (examples still need to be built though)... I'm going to sleep now 💤 |
@Avamander I've been looking for a CMake style guide that we can link to and best suites contributions. I like this one. Although it doesn't really talk about syntax guidelines like indentation size & where the closing parenthesis should go for multi-line cmds (CMake docs use a mix of styles). There is also an official guideline in CMake docs, but it focuses more on writing a "Find-Module" script and variable naming. Maybe we can explicitly specify 4-space indentations and refer to both docs in our CONTRIBUTING.md |
@AndruePeters The RF24\cmake\CompilerWarnings.cmake doesn't seem to have the expected affect... I started building the new python wrappers using CMake, and the compiler warnings prevents building RF24 as a linked lib. However, compiling RF24 as a standalone lib doesn't have any warnings raised as errors (no warnings at all in fact). The sheer amount of warnings raised as errors feel like a rude awakening (there is a lot). As it stands, I don't think the The following is 1 warning that should be ignored for cross-platform reasons /mnt/c/Users/%USERNAME%/Documents/GitHub/pyRF24/RF24/RF24.cpp: In member function ‘void RF24::csn(bool)’:
/mnt/c/Users/%USERNAME%/Documents/GitHub/pyRF24/RF24/RF24.cpp:16:21: warning: unused parameter ‘mode’ [-Wunused-parameter]
16 | void RF24::csn(bool mode)
| ~~~~~^~~~ It would be too much work to workaround this warning (which is raised as an error by default). |
I understand that. Jason Turner and some other top C++ people recommend to treat warnings as errors. It's always been something I enable for releases, but disable for normal development. I find it gets in the way a lot, but it does force developers to submit quality code. Figure out what warnings you think should stay enabled for this code-base, and then force very PR to compile with these flags. With that said, I tend to turn it off until I'm ready for a release. So, to address the warning:
|
I think the seconds approach actually raises a separate set of warnings (definitely will raise a I've done the first approach already, but the warning still gets raised. I think |
Use a static cast
static_cast<void>(argument);
…On Fri, Aug 13, 2021, 12:28 AM Brendan ***@***.***> wrote:
I think the seconds approach actually raises a separate set of warnings
(definitely will raise a -Wold-style-cast but I can get around that).
I've done the first approach already, but the warning still gets raised. I
think -Wpedantic may be reasserting the Wunused-parameter
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKBR4JWO4K3N6AI2MYILJDT4SNQHANCNFSM4TM4PXMA>
.
|
yep that did it! All I have left to fix are all the |
Well, I fixed all the warnings... I still think its strange that the warnings don't show up when RF24 lib is built alone (they only showed up when I used RF24 lib as a dependency). Furthermore, I've been linking built/installed RF24 lib to RF24Network & RF24Mesh (all using CMake now), and the warnings never showed. I did copy (& |
Are you adding I hate conversion warnings. They're so important, but also a pain with most interfaces. |
Yep. Actually I had to rename the |
I have to use c-style casts for passing a |
Only disable the warnings you specifically need to. Also, const_const is
very tricky to use properly. It has huge potential to cause UB.
https://youtu.be/iuLwHoMFP_Y
…On Mon, Aug 23, 2021, 12:38 AM Brendan ***@***.***> wrote:
I have to use c-style casts for passing a pgm_read_ptr() types to printf()
for certain platforms - Teensy platforms crash when using const_cast
while all other platforms (including Linux) only like const_cast. I'm
disabling the extra compiler warnings until teensyduino core updates their
toolchain (which is rumored to happen soon).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKBR4MFJS4G5U342MOKT4DT6HGCZANCNFSM4TM4PXMA>
.
|
ok here is the details for the problem I came across: // no printf_P defined
#define pgm_read_ptr(addr) (*(void *const *)(addr)) avr/pgmspace.h (from Teensyduino-core for teensy3 as that's what the errors site) #define printf_P(...) printf(__VA_ARGS__)
// ....
#define pgm_read_ptr(addr) ({ \
typeof(addr) _addr = (addr); \
*(void * const *)(_addr); \
}) There is some confusion in the RF24 src
I feel like this belongs in a separate issue, but posting it here (for now). I have disabled the |
Describe the problem
Configuring RF24 as part of another project is not 'simple'. My current method calls a script the configures and
installs RF24 on the host system. However, I'd like to be able to use this repo as I would other libraries such as:
with a package manager, or using git submodules. One of the present issues regards the build system which
requires running the configure script and then using Make.
Describe the solution you'd like
The solution would be to combine the configuration and build system and configuration. CMake provides an interface
to handle this. CMake has also transitioned to becoming industry standard for build systems. The interface for building using
CMake can be similar to the current interface for the configure script; e.g.,
cmake . -Bbuild -Ddriver=RPi
Using CMake provides several benefits. CMake will produce a compiler_commands.json, which many tools are supporting. This is mostly a benefit for developers with Intellisense and similar technologies. It should make cross-compilation simpler. Allows better configuration and modularization to warnings and errors while also keeping things simple. CMake itself is distributed as a binary package, so you don't need sudo privileges to use it or update it.
The benefits about simplicity and modularity only hold true if you use the target-based system like it's intended to. I can show examples as needed.
The text was updated successfully, but these errors were encountered: