Cmake cleanup: stage1 #247

conte91 · 2019-12-04T11:33:45Z

Trying to recover from the multitude of added compilations that the BitBoxBase firmware introduced. This PR adds modularization to the code, and tries to bulid with as little PRODUCT_*, APP_* etc. definitions as possible. In order to make sure that nothing breaks when undefining those macros, they are enforced to be #defined in the relevant headers (so if a code unit doesn't really need the macro, it won't be able to include the headers that depend on it).

NickeZ · 2019-12-04T12:34:46Z

One thing I noticed was that I intended that we should put the definitions of APP_ macros in a config file (such as platform_config.h) because it is easier to review it there than in the cmake files.

However I also realized now that platform_config.h probably isn't an appropriate name anymore. What do you think about renaming it to product_config.h?

NickeZ · 2019-12-04T12:35:17Z

Also just redefinig the APP_ macros should be its own PR. that can be done independently of any other refactoring.

NickeZ · 2019-12-04T12:38:08Z

we could also have a separate app_config.h with the APP_ macros in it. WDYT?

conte91 · 2019-12-04T13:59:08Z

However I also realized now that platform_config.h probably isn't an appropriate name anymore. What do you think about renaming it to product_config.h?

The PRODUCT_ macros are imho a wrong concept anyway. We use the "bigger" enum (all the products we have or might have in the future) to deduce the "smaller" enums (the board that the firmware shall run on). IMHO we should split this into two configs (PLATFORM, EDITION) to allow separation of the hardware-dependent code that doesn't depend on, for example, whether the firmware is multi or btc-only. See 436611b for a draft of how this could look like.

conte91 · 2019-12-04T13:59:24Z

we could also have a separate app_config.h with the APP_ macros in it. WDYT?

+1

NickeZ · 2019-12-04T14:33:09Z

However I also realized now that platform_config.h probably isn't an appropriate name anymore. What do you think about renaming it to product_config.h?

The PRODUCT_ macros are imho a wrong concept anyway. We use the "bigger" enum (all the products we have or might have in the future) to deduce the "smaller" enums (the board that the firmware shall run on). IMHO we should split this into two configs (PLATFORM, EDITION) to allow separation of the hardware-dependent code that doesn't depend on, for example, whether the firmware is multi or btc-only. See 436611b for a draft of how this could look like.

That makes sense to me in case it reduces the built object files.

conte91 · 2019-12-04T15:03:31Z

Also just redefinig the APP_ macros should be its own PR. that can be done independently of any other refactoring.

Makes sense - I'll open a separate request for this.

codecov-io · 2019-12-17T16:15:54Z

Codecov Report

Merging #247 into master will decrease coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   41.66%   41.44%   -0.22%     
==========================================
  Files          96       94       -2     
  Lines        5439     5407      -32     
==========================================
- Hits         2266     2241      -25     
+ Misses       3173     3166       -7

Impacted Files	Coverage Δ
src/ui/components/show_logo.c	`0% <ø> (ø)`	⬆️
src/hww.c	`0% <ø> (ø)`	⬆️
src/usb/usb.c	`0% <ø> (ø)`	⬆️
src/ui/components/ui_images.c	`0% <ø> (ø)`	⬆️
src/ui/components/confirm_gesture.c	`0% <ø> (ø)`	⬆️
src/u2f.c	`38.11% <ø> (ø)`	⬆️
src/ui/components/trinary_input_string.c	`0% <ø> (ø)`	⬆️
src/commander/commander.c	`10.82% <ø> (ø)`	⬆️
src/touch/gestures.c	`96.07% <ø> (ø)`	⬆️
src/ui/components/waiting.c	`0% <ø> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd496db...a9710c9. Read the comment docs.

NickeZ

overall looks good to me. Remember that we must double check that the usb descriptors stay the same and that the btconly firmware doesn't have any u2f/eth symbols in it.

NickeZ · 2020-01-27T17:06:38Z

src/CMakeLists.txt

-    target_link_libraries(${elf} PRIVATE c asf4-drivers-min -Wl,-u,exception_table)
+    add_dependencies(${elf} libwally-core)
+    add_dependencies(${elf} noise-c)
+    target_link_libraries(${elf} PRIVATE c asf4-drivers-min noiseprotocol -Wl,-u,exception_table)


I think noise and libwally shouldn't be needed in the bootloader.

Fixed. There was a missing dependency between libwally.a and libwally-core (the target actually building libwally!), so the dep tree was broken.

NickeZ · 2020-01-27T17:09:13Z

src/CMakeLists.txt

+      target_sources(${elf} PRIVATE ${BITBOX02-FIRMWARE-SOURCES})
+    else()
+      target_sources(${elf} PRIVATE ${BITBOXBASE-FIRMWARE-SOURCES})
+    endif()


I don't think this pattern (find string in elf name) works well here. It works well for the "semhosting" string because it is binary. I think it would be better if you created two variables: "FIRMWARES-BITBOX02" and "FIRMWARE-BITBOXBASE" a bit like there is for the bootloaders. Then you could iterate only over the bb02 firmwares or the bbb firmwares and add sources.

benma · 2020-02-25T11:28:02Z

src/touch/gestures_bitboxbase.c

@@ -0,0 +1,385 @@
+// Copyright 2019 Shift Cryptosecurity AG


this file is nowhere to be found in cmakelists.txt, how does that work?

benma · 2020-02-25T11:28:31Z

src/touch/gestures_bitboxbase.c

+#endif
+
+#include "gestures.h"
+#if PLATFORM_BITBOXBASE == 1


is this redundant? hopefully this file would only be compiled for bitboxbase only.

benma · 2020-02-25T11:29:16Z

src/touch/gestures.c

@@ -25,6 +25,10 @@
 #endif

 #include "gestures.h"
+#include "gestures_impl.h"
+#if PLATFORM_BITBOXBASE == 1
+#include "gestures_bitboxbase.h"


what's the difference to gestures_bitboxbase.c?

As the files related to APP_BTC don't depend on any PRODUCT* macro, but only on the APP_* macros, move those to a separate CMake unit (app_btc-multi, app_btc-btc), to avoid multiple unnecessary compilations.

Most of the workflow, components, and UI code doesn't depend on the platform it will be run on. Compile it only once to save many CMake build entries. The gestures code is a dependency of most components, but it has a slight dependency on the hardware platform. Split it into an API and an implementation file to let including the header possible without having to define the platform the code will run on.

conte91 force-pushed the cmake_cleanup_stage1 branch 3 times, most recently from 80d1276 to 7c36a20 Compare December 17, 2019 15:51

conte91 force-pushed the cmake_cleanup_stage1 branch from 7c36a20 to 635cf8f Compare January 27, 2020 15:27

conte91 requested a review from NickeZ January 27, 2020 16:11

NickeZ requested changes Jan 27, 2020

View reviewed changes

conte91 force-pushed the cmake_cleanup_stage1 branch 4 times, most recently from 8ae0f80 to b20204b Compare January 28, 2020 11:40

conte91 requested a review from NickeZ January 28, 2020 12:12

conte91 force-pushed the cmake_cleanup_stage1 branch from b20204b to a9710c9 Compare January 30, 2020 10:32

benma reviewed Feb 25, 2020

View reviewed changes

conte91 added 4 commits February 25, 2020 12:42

cmake: Extract some of the common source files to avoid recompilation.

107fa6e

cmake: Split app sources into separate libraries.

ce10c20

As the files related to APP_BTC don't depend on any PRODUCT* macro, but only on the APP_* macros, move those to a separate CMake unit (app_btc-multi, app_btc-btc), to avoid multiple unnecessary compilations.

Squash me: Remove gestures_bitboxbase.c

c6b3b40

conte91 force-pushed the cmake_cleanup_stage1 branch from a9710c9 to c6b3b40 Compare February 25, 2020 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake cleanup: stage1 #247

Cmake cleanup: stage1 #247

conte91 commented Dec 4, 2019

NickeZ commented Dec 4, 2019

NickeZ commented Dec 4, 2019

NickeZ commented Dec 4, 2019

conte91 commented Dec 4, 2019 •

edited

Loading

conte91 commented Dec 4, 2019

NickeZ commented Dec 4, 2019

conte91 commented Dec 4, 2019

codecov-io commented Dec 17, 2019 •

edited

Loading

NickeZ left a comment

NickeZ Jan 27, 2020

conte91 Jan 28, 2020

NickeZ Jan 27, 2020

conte91 Jan 28, 2020

benma Feb 25, 2020

benma Feb 25, 2020

benma Feb 25, 2020

Cmake cleanup: stage1 #247

Are you sure you want to change the base?

Cmake cleanup: stage1 #247

Conversation

conte91 commented Dec 4, 2019

NickeZ commented Dec 4, 2019

NickeZ commented Dec 4, 2019

NickeZ commented Dec 4, 2019

conte91 commented Dec 4, 2019 • edited Loading

conte91 commented Dec 4, 2019

NickeZ commented Dec 4, 2019

conte91 commented Dec 4, 2019

codecov-io commented Dec 17, 2019 • edited Loading

Codecov Report

NickeZ left a comment

Choose a reason for hiding this comment

NickeZ Jan 27, 2020

Choose a reason for hiding this comment

conte91 Jan 28, 2020

Choose a reason for hiding this comment

NickeZ Jan 27, 2020

Choose a reason for hiding this comment

conte91 Jan 28, 2020

Choose a reason for hiding this comment

benma Feb 25, 2020

Choose a reason for hiding this comment

benma Feb 25, 2020

Choose a reason for hiding this comment

benma Feb 25, 2020

Choose a reason for hiding this comment

conte91 commented Dec 4, 2019 •

edited

Loading

codecov-io commented Dec 17, 2019 •

edited

Loading