Running this script will parse the top of all source files
that are not submodules, examples or test cases, to find
all #include's and then do basically two things:
1) Reorder and group the headers so that px4_* headers are
included first, then local headers (headers of the project
that aren't submodules) then C++ headers if any, then C
headers if any, then system headers (which includes submodules)
and finally any #includes that are inside #if*...#endif
constructs.
2) Fix the use of "" or <> in a consistent manner.
Afterwards few fixes might be necessary for compile errors that
pop up. Most of those are already fixed in my previous commits.
However, I was not able to test a compilation for ros
(with __PX4_ROS defined) -- so some more fixes might be necessary
because of the header reordering.
The script comes with a progress counter and an error summary
(if any) at the end (so no scrolling back is necessary).
It is highly recommended to only run this script in a clean
project with no outstanding changes that need to be committed.
In fact, the script enforces this (unless you pass --force).
Reverting a run of the script is then easy with 'git checkout .'.
It is also possible to run the script on a single file
by passing that on the command line. In that case it
might make sense to pass --debug too, though that was really
meant for just me, while developing the script. This will write
debug output into the file that is passed, so you don't
want to commit that! ;)
<systemlib/err.h> --> "systemlib/err.h" that fix_headers.sh
would miss because it comes after code it doesn't understand.
Effectively remove the '__EXPORT extern perf_counter_t perf_alloc('
line, because currently perf_alloc is defined to be NULL, and
after running fix_headers.sh that happens *before* this header
is included (the order of headers will be changed).
Do not define NULL to be (void*)0: that only works for C.
In fact, the conversions needed for NULL are so full of exceptions
that standard C++ introduced a new *keyword*: nullptr.
That's what we really should be using for C++ code.
In this case I just include the correct header to define NULL
the correct way.
Not really related to the header line:
Removed an #include <time.h> because I noted that px4_time.h
was already included... and moved a #include <sys/time.h>
to the top of the file (not really a fan of including headers
in the middle unless absolutely necessary).
Removed a include of queue.h because I noted it wasn't used.
In this particular case it does no harm,
but since in other cases it can lead to
problems I didn't want to add an exception
for this case to fix_headers.sh, that currently
chokes on this because it doesn't know better
than that it's a bad thing.
Note on how #pragma once works: when encountered
(aka the #ifdef that it is inside has to
be true), the compiler marks the whole
file as "seen" (this is implementation
defined, but most implementations store
the inode of the file). Subsequent #include's
of that file/inode are then completely skipped.
Hence it doesn't matter if the #pragma is at
the beginning, at the end or in the middle,
but it should be encountered every #include,
usually, and thus not be inside an #if... #endif
construct.
These headers files were missing from the header files that
I added them to; the fact that they were missing didn't
lead to compile errors because by coincidence the missing
headers are included in the source files before including
these headers. But, after the reordering of header inclusions
by Tools/fix_headers.sh, these cases will give rise to compiler
errors.
Using cout drags in std::localeconv which isn't defined
on QURT. While this file is also used for POSIX, it doesn't
seem to harm much to use printf there as well.
We don't have C++ unsafe headers (anymore).
I added a test to fix_headers.sh that checks if certain "unsafe"
headers are ONLY included inside a __BEGIN_DECLS ... __END_DECLS
(because after all, they are unsafe), as well as checking that
no other header files are included inside such a block. The rationale
of the latter is that if a file is a C header and it declares
function prototypes (otherwise it doesn't matter) and is sometimes
included outside a __BEGIN_DECLS ... __END_DECLS block (from a C++
source file) then it has to be C++ safe and doesn't ever to be
included from inside such a block; while if a file is a C++ header
then obviously it should never be included from such a block.
fix_headers.sh subsequently found several safe headers to be
included from inside such a block, and those that were (apparently
in the past) unsafe were included only sometimes inside such a
block and often outside it. I had a look at those files and saw
that at least an attempt has been made to make them C++ safe,
but especially because they already are included OFTEN outside
a __BEGIN_DECLS ... __END_DECLS (from C++ source files) the
best decision seems to treat them as safe.
This is not risky: .c files that define such functions still
generate C-linkage for their functions. If a C++ unsafe C header
is included outside a __BEGIN_DECLS ... __END_DECLS block then
the only possible result would be an undefined reference to
a function with C++-linkage that will not exist. Aka, when
something links after this commit, then the commit was correct.
I did build all targets and they all linked.
This patch reorders px4_defines.h to make it more readable (I think)
but more importantly, cleans up the #include <math.h>/<cmath>
and [std::]isfinite stuff.
My main goal was to completely get rid of including math.h/cmath,
because that doesn't really belong in a header that is supposed to
define macro's and is included in almost every source file (if not
all).
I'm not sure what it did before ;) (pun intended), but now it does
the following:
PX4_ISFINITE is only used in C++ code (that was already the case,
but hereby is official; for C code just use 'isfinite()') and is
defined to be std::isfinite, except on __PX4_QURT because that uses
the HEXAGON toolset which (erroneously) defines isfinite as macro.
I would have liked to remove PX4_ISFINITE completely from the code
and just use std::isfinite whereever that is needed, but that would
have required changing the libecl submodule, and at the moment I'm
getting tired of changing submodules... so maybe something for the
future.
Also, all includes of <math.h> or <cmath> have been removed except
for __PX4_NUTTX. Like the HEXAGON toolset NuttX currently defines
isfinite as macro for C++. So, we could have solved this in the
same was as __P4_QURT; but since we can fix NuttX ourselves I chose
to add a kludge to px4_defines.h instead that fixes this problem,
until the time that NuttX can be fixed (again postponing changing
a submodule). The kludge still demands including <cmath>, thus.
After removal of the math header file, it needed to be included
in source files that actually need it, of course.
Finally, I had a look at the math macro's (like M_PI, M_PI_F,
M_DEG_TO_RAD etc). These are sometimes (erroneously) defined in
certain math.h header files (like both, hexagon and nuttx).
This is incorrect: neither the C nor the C++ standard defines
math constants (neither as macro nor otherwise). The "problem"
here was that px4_defines.h defined some of the M_*_F float
constants in terms of the M_* double constant, which are
sometimes not defined either thus. So, I cleaned this up by
defining the M_*_F math constants as float literals in px4_defines.h,
except when they are defined in math.h for that platform.
This means that math.h has to be always included when using those
constants, but well; not much difference there as those files
usually also need/use the macro NAN (which *is* a standard macro
defined by math.h).
Finally finally, DEFAULT_PARAM_FILE was removed as it isn't
used anymore.
All in all I think the resulting px4_defines.h is nice, giving me
much less the feeling of a nearly unmaintainable and over time
slowly growing collection of kludges and hacks.
The "targets" posix_rpi_common, qurt_sdflight_default and
posix_sdflight_default are not real targets; they are
just files in cmake/configs that are included by other
targets.
These are some changes that I needed to compile
most of the unsupported targets.
After this all (make list_config_targets) compile
for me except: posix_eagle_muorb, posix_sdflight_default
and qurt_eagle_legacy_driver_default.
app.h, generated from app.h_in, has unnecessary code duplication
and isn't a header file (it defines globals, static functions
and doesn't have a header guard, moreover, it has a 'using namespace
std;'). Because of this, a real headerfile that declares the stuff
defined in apps.h was missing leading to even more code duplication:
scattered forward declarations in .cpp files and an often repeated
type of std::map<std::string, px4_main_t>.
This patch moves cmake/qurt/apps.h_in to src/platforms/apps.cpp.in
(with some changes) and removes cmake/posix/apps.h_in.
Then src/platforms/apps.cpp.in is split into src/platforms/apps.cpp.in
and src/platforms/apps.h.in, splitting declarations from definitions.
A typedef is defined for the map (apps_map_type).
The main difference between cmake/posix/apps.h_in and
cmake/qurt/apps.h_in was that the first defined a global 'apps',
while qurt stores the apps in QShell. I opted to get rid of
the global variable (which are in general evil for various reasons)
and used the API of cmake/qurt/apps.h_in where a provided 'apps'
map is initialized with a call to init_app_map. Thus removing
the existing code duplication.
The px4_os_add_flags defined in cmake/posix/px4_impl_posix.cmake did
add (threading) libraries to added_exe_linker_flags, which subsequently
end up in CMAKE_EXE_LINKER_FLAGS and then have no effect because those
flags are passed to the linker before any of the object files and static
libraries.
Those libraries are already added correctly in the corresponding
src/firmware/*/CMakeLists.txt files (for qurt, nuttx AND posix).
I left in the non-library linker flag '-pthread' for the bebop board,
although it seems very weird to me that this is needed (is it?).
If that is needed then it seems weird to link manually (that is,
src/firmware/*/CMakeLists.txt) with -lpthread.
For linux/g++ -pthread is added to the CXXFLAGS as it should be:
this causes the compiler to define _REENTRANT which is needed
for (the interface of) certain libraries to become thread-safe.
Offically one also can just pass -pthread to the linker, which then
causes the right libraries to be linked, but just linking with
-lpthread -lm -lrt works too.
I ran into this while adding support for libcwd, which explicitly
complains that _REENTRANT isn't defined when trying to link with
the thread-safe version of libcwd (-lcwd_r) and then tells you
to use -pthread.
On second thought, using % just isn't correct. When a make
target looks like:
foo% bar%:
...
Then that implies that it will build BOTH foo% and bar% whenever
that rule is triggered (by either) for any value of the pattern %.
Clearly that is not what we want / intend.
So, this commit goes back to using config targets without a % in it
by generating a full list of them.
It also turns sitl_deprecation into a Hidden Target.
Finally, now that we have those target lists anyway, I added
a 'help' and 'list_config_targets' targets that print the
available targets.
Note that the '%' catch-all target only works when all other targets
either have a recipe or are marked as .PHONY, otherwise such targets
are only interpretted as dependencies and still executre the '%'
target afterwards, which is not what we want.
* This allows one to run 'make posix list_vmd_make_targets' and get output like
>make posix list_vmd_make_targets
[...]
-- Build files have been written to:
/usr/src/debian/px4/Firmware/Firmware.git/build_posix_sitl_default
PX4 CONFIG: /usr/src/debian/px4/Firmware/Firmware.git/build_posix_sitl_default
Scanning dependencies of target list_vmd_make_targets
[100%] List of acceptable 'posix_sitl_default' <viewer_model_debugger> targets:
none
none_iris
none_iris_opt_flow
none_tailsitter
[...]
replay_solo_valgrind
replay_typhoon_h480_valgrind
[100%] Built target list_vmd_make_targets
Or, run 'make list_vmd_make_targets' from the build_posix_* directory.
* This adds the list_cmake_targets make target to print all
cmake targets that one can match with PX4_NO_OPTIMIZATION.
PX4_NO_OPTIMIZATION is ignored (do optimization as normal)
when the CONFIG isn't posix_sitl_*.
* Add comment in Makefile on how/where to find all targets.
This allows one to set a semi-colon separated list of regular
expressions in the environment variable PX4_NO_OPTIMIZATION
to control which (cmake generated) targets should be compiled
without optimization.
Suppressing optimization can be necessary for debugging in
a debugger, especially when trying to step through the code
or needing to print variables that otherwise are optimized out.
EXAMPLE
export PX4_NO_OPTIMIZATION="px4;^modules__uORB;^modules__systemlib$"
will result in the following messages during cmake configuration:
[...]
-- Disabling optimization for target 'platforms__posix__px4_layer'
because it matches the regexp 'px4' in env var PX4_NO_OPTIMIZATION
-- Disabling optimization for target 'modules__systemlib' because it
matches the regexp '^modules__systemlib$' in env var PX4_NO_OPTIMIZATION
-- Disabling optimization for target 'modules__uORB' because it matches
the regexp '^modules__uORB' in env var PX4_NO_OPTIMIZATION
-- Disabling optimization for target 'examples__px4_simple_app' because
it matches the regexp 'px4' in env var PX4_NO_OPTIMIZATION
-- Disabling optimization for target 'modules__uORB__uORB_tests' because
it matches the regexp '^modules__uORB' in env var PX4_NO_OPTIMIZATION
-- Disabling optimization for target 'px4' because it matches the regexp
'px4' in env var PX4_NO_OPTIMIZATION
Note that a list of all (currently used) target names can be printed
with the following command line from within the required build directory:
find . -wholename '*/CMakeFiles/*.dir/flags.make' | xargs dirname | xargs basename -a | sort -u | sed -e 's/.dir$//'
While the config_sitl_rcS_dir is used more extensively (and actually
only depending on the base cmake target), the variables
config_sitl_viewer and config_sitl_debugger are just used to be
passed on for the run_config target; config_sitl_debugger is even
*always* 'disable'. Hence, they don't really need to be cached
(INTERNAL or not). Before this patch FILEPATH was used instead
of INTERNAL, but I doubt very much that that was intended. That
only makes sense when cmake-gui would be used and then would pop-up
a file browser to let the user pick a file (while really they
need to pick a directory, so it's wrong either way).
The ONLY reason caching would be used is when a developer edits
the build_posix_sitl_*/CMakeCache.txt files, changes these values
and then runs cmake in the build directory again, now overriding
the values intended here. Nevertheless, I left in the caching.
The main change in this commit (that theoretically has no real effect)
is that I removed the duplicated maintenance of
posix_sitl_broadcast.cmake. When that file was added, it was an
exact copy of posix_sitl_default.cmake and is since not always
maintained to remain the same. I don't think that difference is
in anyway relevant for the broadcasting part though.
Note that I think that something like that also holds for the
posix_sitl_replay.cmake; it would be a lot better - maintenance-wise
- when it was just derived from (or the same as) posix_sitl_default,
I think.
Doesn't really do anything, but while working on understanding
things I did this cleanup, making the Makefile a lot shorter
and more flexible for future changes ;).
Over time I made a few changes unrelated to what I'm really working on.
These changes are hereby committed first. The bug fixes are related to
what I'm doing in that I need them to be fixed for future commits.
Tools/sitl_run.sh: rename label to rcS_dir and fix usage help.
cmake/common/px4_base.cmake: Remove the check on OUT_UNPARSED_ARGUMENTS,
and a few typos and indentation issues.
cmake/configs/posix_sitl_replay.cmake: Set the correct variable
(config_sitl_rcS_dir) to the correct directory.
cmake/nuttx/px4_impl_nuttx.cmake: typos and indentation issues,
and removal of a redundant FORCE (INTERNAL implies FORCE).
cmake/posix/px4_impl_posix.cmake: typos and indentation issues.
cmake/qurt/px4_impl_qurt.cmake: typos and indentation issues.
src/modules/mavlink/mavlink_ftp.cpp : possible strict-aliasing breakage.
NOTES
The second argument passed to sitl_run.sh is the value of
config_sitl_rcS_dir. This fact is missing from the usage help.
I renamed 'label' to 'rcS_dir' to better reflect this.
Also, for the 'replay' config the wrong variable was set causing
the call to sitl_run.sh to skip an argument and fail (ie the
debugger was passed as rcS_dir and so on).
The check on OUT_UNPARSED_ARGUMENTS in px4_parse_function_args
basically causes every passed IN variable to be REQUIRED and is
therefore a bug. The test for the presence of the REQUIRED arguments
follows directly after and is sufficient for this job. This bug went
unnoticed because currently every argument to OPTIONS, ONE_VALUE,
and MULTI_VALUE is actually passed to the function(s) calling
px4_parse_function_args (them being REQUIRED or not).
The changes in mavlink_ftp.cpp are to avoid a possible aliasing bug
and (mostly) to avoid the compiler warning/error: dereferencing type-
punned pointer will break strict-aliasing rules [-Werror=strict-aliasing].
When running a simulation with, for example,
make posix jmavsim
px4 crashed almost 100% reproducable near start up.
This turned out to be a stack overflow. On gitter
it was suggested that the main reason for this could
be stack sizes, as currently used, assume a 32bit pointer
size and that doubling the stack size for everything
but NuttX would be the Right Thing.
This is the solution that I came up with (it makes
my core dumps disappear).
After I got a compiler warning for a printf format in this file
(which I cannot reproduce anymore; for some reason g++ is usually
quite happy with all the errors in this file), I fixed all 'printf'
formats to match the type of their arguments as follows:
uint8_t : %hhu
uint16_t : %hu (or %hx)
uint32_t : %u (or %x)
int16_t : %hd
int and int32_t : %d
long int : %ld
Since this file is C++, what we REALLY should be using is
ostream operator<<'s of course (type safe by design and faster
(compile time type matching, no need for format decoding)).
Carlo Wood