NuttX upgrade default compiler to GCC 7 #8551

dagar · 2018-01-01T15:29:41Z

Update NuttX GNU Arm Embedded Toolchain to the latest stable version.

GNU Arm Embedded Toolchain: 7-2017-q4-major December 18, 2017

nsh> ver all
HW arch: PX4FMU_V2
HW type: V2M
HW version: 0x0009000A
HW revision: 0x00000000
FW git-hash: 3bd0f045169ddc69a9ec74de4f53a2cb795bf685
FW version: 1.7.2 0 (17236480)
OS: NuttX
OS version: Release 7.22.0 (118882559)
OS git-hash: b18053574bf41712cef93e31bf178518f451a350
Build datetime: Jan  1 2018 15:32:02
Build uri: localhost
Toolchain: GNU GCC, 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
MFGUID: 313237313036510500470037
MCU: STM32F42x, rev. 3
UID: 470037:30365105:31323731

https://github.com/PX4/containers/blob/5c29816a92b5dad22333a41860ec0a0d0f608567/docker/px4-dev/Dockerfile_nuttx#L27-L30

dagar · 2018-01-01T15:33:29Z

TODO before merge

update OSX homebrew
sync all documentation

davids5 · 2018-01-01T16:34:28Z

@dagar
I get nervous when we stray from the ARM contributed versions from formerly from launchpad (https://developer.arm.com/open-source/gnu-toolchain/gnu-rm)... how do the build sizes compare?

dagar · 2018-01-01T17:08:38Z

It reduces px4fmu-v2_default by about 5kB.

I don't feel that strongly about the version, but things are different between CI, documentation, OSX, etc. I want to get them all in sync and thought we might as well go to the latest. What would you recommend following?

make check master (left) vs pr-nuttx_gcc7 (right).

px4fmu-v2_default compare

   text    data     bss     dec     hex filename
1024568    4053   19272 1047893   ffd55 nuttx_px4fmu-v2_default.elf    <-- master
1019472    4045   19296 1042813   fe97d nuttx_px4fmu-v2_default.elf    <-- pr-nuttx_gcc7

px4fmu-v2_default bloaty compare

dagar@dagar-desktop ~ % bloaty -d compileunits -s file ~/git/Firmware-upstream/build/px4fmu-v2_default/nuttx_px4fmu-v2_default.elf -- ~/git/Firmware/build/px4fmu-v2_default/nuttx_px4fmu-v2_default.elf
     VM SIZE                                                                FILE SIZE
 --------------                                                          --------------
  -0.9% -2.96Ki [None]                                                    +396Ki  +4.5%
  +4.0%    +357 ../../src/modules/mc_att_control/mc_att_control_main.cpp    +357  +4.0%
  +1.4%    +218 ../../src/modules/mavlink/mavlink_receiver.cpp              +218  +1.4%
  +0.9%    +180 ../../src/modules/mc_pos_control/mc_pos_control_main.cpp    +180  +0.9%
  +1.5%    +174 ../../src/lib/ecl/EKF/ekf_helper.cpp                        +174  +1.5%
  +1.8%    +172 ../../src/lib/ecl/EKF/control.cpp                           +172  +1.8%
  +2.2%    +147 ../../src/lib/ecl/EKF/ekf.cpp                               +147  +2.2%
   +45%    +132 ../../src/lib/controllib/block/BlockParam.cpp               +132   +45%
  +4.4%    +128 ../../src/modules/commander/PreflightCheck.cpp              +128  +4.4%
  +3.5%    +124 ../../src/systemcmds/pwm/pwm.cpp                            +124  +3.5%
  +1.2%    +109 ../../src/drivers/px4fmu/fmu.cpp                            +105  +1.2%
  -2.0%    -112 ../../src/modules/navigator/navigator_main.cpp              -112  -2.0%
  -1.2%    -122 ../../src/modules/logger/logger.cpp                         -122  -1.2%
  -0.9%    -125 ../../src/drivers/px4io/px4io.cpp                           -125  -0.9%
  -3.0%    -125 ../../src/modules/commander/mag_calibration.cpp             -125  -3.0%
  -1.8%    -129 ../../src/modules/sensors/voted_sensors_update.cpp          -129  -1.8%
  -0.8%    -163 ../../src/modules/mavlink/mavlink_messages.cpp              -163  -0.8%
  -1.4%    -206 ../../src/lib/ecl/EKF/mag_fusion.cpp                        -206  -1.4%
  -0.9%    -211 ../../src/lib/ecl/EKF/covariance.cpp                        -211  -0.9%
  -0.1%    -366 [329 Others]                                                -306  -0.1%
 -20.2% -2.23Ki ../../src/lib/ecl/EKF/optflow_fusion.cpp                 -2.23Ki -20.2%
  -0.5% -5.02Ki TOTAL                                                     +394Ki  +4.2%

jlecoeur

LGTM except the implicit-fallthrough warning that I would rather keep.

For what it's worth, I have been flying on pixhawk and pixfalcon flashed with firmwares compiled using gcc 7.1.0 to 7.2.0 without trouble.

jlecoeur · 2018-01-03T15:32:11Z

cmake/common/px4_base.cmake

@@ -332,6 +332,8 @@ function(px4_add_common_flags)
 		-Wunknown-pragmas
 		-Wunused-variable

+		-Wno-implicit-fallthrough # set appropriate level and update
+


I actually like this warning. Better be explicit when fallthrough is intended, like here:
https://github.com/PX4/Firmware/blob/bcceadcb85b293046166ed7898c96b7dbf9acd30/src/drivers/mpu9250/mpu9250_spi.cpp#L168-L184

dagar · 2018-01-03T15:48:34Z

@jlecoeur yes, I like the warning as well, but I was having issues with setting the appropriate level (one of the reasons I tagged you). By default it was failing on some of the switch statements you already changed.

jlecoeur · 2018-01-03T18:29:10Z

@dagar We can always fix the warnings. However I do not feel confident asking people to install a compiler that does not have a consistent behaviour on different systems (see below). Did you try it on an ubuntu PC or Mac?

There are no warnings with the gcc7 archlinux package (it was even CI tested before this PR). Here it is: https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/arm-none-eabi-gcc
There are no warnings either on arch with the same compiler as in the docker image (https://github.com/PX4/containers/blob/5c29816a92b5dad22333a41860ec0a0d0f608567/docker/px4-dev/Dockerfile_nuttx#L27-L30):

[...]
-- The ASM compiler identification is GNU
-- Found assembler: /home/jlecoeur/tmp/arm-none-eabi-gcc-7.2.1/bin/arm-none-eabi-gcc
-- Found PythonInterp: /usr/bin/python (found version "3.6.4") 
-- Found PY_jinja2: /usr/lib/python3.6/site-packages/jinja2  
-- C compiler: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
-- C++ compiler: arm-none-eabi-g++ (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
-- NuttX: px4io-v2 nsh cortex-m3
-- Configuring done
-- Generating done
-- Build files have been written to: /run/media/julien/Data/Documents/PX4/Firmware/build/px4io-v2_default
[153/153] Linking CXX executable nuttx_px4io-v2_default.elf
[601/601] Creating /run/media/julien/Data/Documents/P...irmware/build/px4fmu-v2_default/px4fmu-v2_default.px4

dagar · 2018-01-03T20:29:10Z

The containers are using GCC directly from Arm. https://developer.arm.com/open-source/gnu-toolchain/gnu-rm/downloads

I wish there were actual packages, but overall this seemed more sustainable allowing us to point all users on all platforms at the exact same version. We can still try to keep a wide range of versions working as needed, but I don't think the project can realistically support more than one official version.

LorenzMeier · 2018-01-03T20:31:48Z

The compiler version has proven essential to flight safety in the past, so in stark contrast to non-critical software we really can only support one single version officially. We put a lot of effort in testing and all bets are off if people switch the compiler.

jlecoeur · 2018-01-03T21:05:47Z

Sorry I was not clear. That compiler version is fine and I agree a single version can be supported.

That is just strange that the same code, with the same compiler, compiled on my machine but not in a docker container.

What about using docker for all builds? The only PX4 dependency would be docker.

dagar · 2018-01-03T22:33:15Z

What about using docker for all builds? The only PX4 dependency would be docker.**

That's almost the case now. All the "production" builds are built with docker now. Docker can also be our fall back to point people at for building locally, but I think most still prefer native (not quite the right term but you know what I mean) toolchains.

The docker environment follows the ubuntu environment instructions.

hamishwillee · 2018-01-03T22:38:06Z

What about using docker for all builds? The only PX4 dependency would be docker.

... and

our docker builds don't work on windows according to the docs. Even if they did, my computer doesn't have the essential requirements (Win Pro).
The setup is much more complicated. We need to work on turnkey zero-effort installation. The current mac setup is a pretty good example. The scripts we added for Linux are also heading the the right direction - they just need to be improved and validated in docker too.

jlecoeur · 2018-01-05T06:47:06Z

@dagar I could re-enable the warning and build successfully by basing px4io/px4-dev-nuttx on px4io/px4-dev-base:2017-12-08.
Let's hold this PR while we pinpoint where the issue is between px4io/px4-dev-base:2017-12-08 and px4io/px4-dev-base:latest.

dagar · 2018-01-09T05:47:58Z

Re-enable the warning in which build?

jlecoeur · 2018-01-09T09:02:26Z

@dagar px4fmu-v2_default
I used the Dockerfile in the PR linked above

dagar · 2018-01-12T21:44:33Z

Rebased on master.

r0gelion · 2018-01-12T22:17:26Z

@dagar Can we get the binary files (.px4) from Jenkins or QGC? or do we need to install GCC7 and compile locally?

dagar · 2018-01-12T22:26:20Z

For all PRs and branches you can now get the binaries from Jenkins under artifacts. If that's not the case somewhere please let me know.

santiago3dr · 2018-01-13T00:21:22Z

few flights on a quad running Pixracer (V4)
indoor test:
https://logs.px4.io/plot_app?log=6eb0e353-8fe2-4df6-849d-4f8d765e3bc0

full test flight:
https://logs.px4.io/plot_app?log=60cea00b-327d-4d2e-ae2e-fc92c66c2a37
https://logs.px4.io/plot_app?log=cfe25a5f-35eb-4e0d-8535-6e2503d29f56

all tested modes work as expected; no issues

Avysuarez · 2018-01-15T15:25:42Z

flight with pixhawk pro (V4pro) Good flight, no issues.
https://review.px4.io/plot_app?log=6f32ad58-c0bd-46d1-9acb-2b55598e3387

flight with pixhawk 1 (V2) Good flight, no issues.
https://review.px4.io/plot_app?log=da759105-271b-41bb-9ed9-9139ea2eae17

dagar · 2018-01-15T16:15:44Z

One trivial compile error on px4-same70xplained-v1. I'll push a fix to ignore the warning for now and we can fix it properly in NuttX. This doesn't invalidate any of the testing results.

@davids5 FYI

r0gelion

Flight testing with no issues, vehicles flew as expected.

dagar · 2018-01-15T16:46:08Z

Circleci failure unrelated. #8689

This is a temporary workaround for equal expressions in chip/sam_xdmac.c and can be reverted once fixed properly in NuttX. Needed for the GCC 7 upgrade in #8551.

dagar · 2018-01-15T17:31:47Z

Rebased on master to address circleci failure.

dagar · 2018-01-15T17:57:12Z

@hamishwillee please ping me when you're available and we'll do a pass to make sure everything is aligned with the single supported GCC version.

We can add a note about other versions, but really emphasize the point about this specific version of GCC 7.

dagar added Admin: Enhancement (improvement) 💡 NuttX (OS) labels Jan 1, 2018

dagar added this to the Release v1.8.0 milestone Jan 1, 2018

dagar requested review from LorenzMeier, jlecoeur, davids5 and PX4TestFlights January 1, 2018 15:29

dagar mentioned this pull request Jan 1, 2018

update docker containers (NuttX upgrade and ccache everywhere) #8535

Closed

dagar force-pushed the pr-nuttx_gcc7 branch from 3bd0f04 to 69ace23 Compare January 1, 2018 15:44

dagar added testflights_fixedwing labels Jan 1, 2018

jlecoeur reviewed Jan 3, 2018

View reviewed changes

dagar force-pushed the pr-nuttx_gcc7 branch from 9e7c5dd to 4ce5c81 Compare January 3, 2018 20:38

jlecoeur mentioned this pull request Jan 4, 2018

Nuttx GCC7: enable -Wimplicit-fallthrough #8583

Closed

dagar mentioned this pull request Jan 9, 2018

[WIP] delete gencpp and genmsg submodules #8612

Closed

dagar force-pushed the pr-nuttx_gcc7 branch from 4ce5c81 to 4145cf8 Compare January 9, 2018 05:47

jlecoeur mentioned this pull request Jan 9, 2018

NuttX: download gcc from https://developper.arm.com PX4/PX4-containers#104

Closed

dagar force-pushed the pr-nuttx_gcc7 branch from 4145cf8 to b01c4ff Compare January 12, 2018 05:47

jlecoeur previously approved these changes Jan 12, 2018

View reviewed changes

dagar dismissed jlecoeur’s stale review via 5061e0f January 12, 2018 21:41

dagar force-pushed the pr-nuttx_gcc7 branch from b01c4ff to 5061e0f Compare January 12, 2018 21:41

dagar requested review from a team and removed request for PX4TestFlights January 12, 2018 21:43

dagar removed testflights_fixedwing labels Jan 12, 2018

r0gelion previously approved these changes Jan 15, 2018

View reviewed changes

jlecoeur previously approved these changes Jan 15, 2018

View reviewed changes

dagar added 2 commits January 15, 2018 12:31

NuttX upgrade default compiler to GCC 7

af085c8

NuttX Make.defs.in ignore -Werror=logical-op

2e8c979

This is a temporary workaround for equal expressions in chip/sam_xdmac.c and can be reverted once fixed properly in NuttX. Needed for the GCC 7 upgrade in #8551.

dagar dismissed stale reviews from jlecoeur and r0gelion via 2e8c979 January 15, 2018 17:31

dagar force-pushed the pr-nuttx_gcc7 branch from 62a39cd to 2e8c979 Compare January 15, 2018 17:31

dagar merged commit 5ab6834 into master Jan 15, 2018

dagar deleted the pr-nuttx_gcc7 branch January 15, 2018 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NuttX upgrade default compiler to GCC 7 #8551

NuttX upgrade default compiler to GCC 7 #8551

dagar commented Jan 1, 2018 •

edited

Loading

dagar commented Jan 1, 2018 •

edited

Loading

davids5 commented Jan 1, 2018

dagar commented Jan 1, 2018

jlecoeur left a comment

jlecoeur Jan 3, 2018

dagar commented Jan 3, 2018

jlecoeur commented Jan 3, 2018

dagar commented Jan 3, 2018

LorenzMeier commented Jan 3, 2018

jlecoeur commented Jan 3, 2018

dagar commented Jan 3, 2018

hamishwillee commented Jan 3, 2018

jlecoeur commented Jan 5, 2018

dagar commented Jan 9, 2018

jlecoeur commented Jan 9, 2018

dagar commented Jan 12, 2018

r0gelion commented Jan 12, 2018

dagar commented Jan 12, 2018

santiago3dr commented Jan 13, 2018

Avysuarez commented Jan 15, 2018

dagar commented Jan 15, 2018

r0gelion left a comment

dagar commented Jan 15, 2018

dagar commented Jan 15, 2018

dagar commented Jan 15, 2018

NuttX upgrade default compiler to GCC 7 #8551

NuttX upgrade default compiler to GCC 7 #8551

Conversation

dagar commented Jan 1, 2018 • edited Loading

dagar commented Jan 1, 2018 • edited Loading

davids5 commented Jan 1, 2018

dagar commented Jan 1, 2018

jlecoeur left a comment

Choose a reason for hiding this comment

jlecoeur Jan 3, 2018

Choose a reason for hiding this comment

dagar commented Jan 3, 2018

jlecoeur commented Jan 3, 2018

dagar commented Jan 3, 2018

LorenzMeier commented Jan 3, 2018

jlecoeur commented Jan 3, 2018

dagar commented Jan 3, 2018

hamishwillee commented Jan 3, 2018

jlecoeur commented Jan 5, 2018

dagar commented Jan 9, 2018

jlecoeur commented Jan 9, 2018

dagar commented Jan 12, 2018

r0gelion commented Jan 12, 2018

dagar commented Jan 12, 2018

santiago3dr commented Jan 13, 2018

Avysuarez commented Jan 15, 2018

dagar commented Jan 15, 2018

r0gelion left a comment

Choose a reason for hiding this comment

dagar commented Jan 15, 2018

dagar commented Jan 15, 2018

dagar commented Jan 15, 2018

dagar commented Jan 1, 2018 •

edited

Loading

dagar commented Jan 1, 2018 •

edited

Loading