[WIP] Refector Makefile by howard0su · Pull Request #786 · DeviationTX/deviation

howard0su · 2019-03-03T11:54:26Z

No description provided.

PhracturedBlue

I do not expect you to necessarily address the code comments. They are just thoughts about the build system in general and things that are worth thinking about how we could improve.

PhracturedBlue · 2019-03-03T13:04:20Z

src/Makefile

-ifndef  TARGET
 TARGET   ?= devo8
 default: $(TARGET)



We should get rid of the default. running make without a target should just fail.

PhracturedBlue · 2019-03-03T13:07:08Z

src/Makefile

-	rm -f $(TARGET).$(EXEEXT) $(TARGET).exe $(TARGET).bin $(TARGET).dfu $(TARGET).list \
-		$(TARGET).map $(ODIR)/*.o $(ODIR)/*.o_ $(ODIR)/*.P  $(ODIR)/*.bin \
-		filesystem/$(FILESYSYTEM) 2> /dev/null || true
+$(foreach t,$(ALLTARGETS),$(eval $(call make-target,$t,$(notdir $(patsubst %/,%,$(dir $(wildcard target/tx/*/$(TARGET))))))))


This doesn't seem right...how does $TARGET get set here?

PhracturedBlue · 2019-03-03T13:09:08Z

src/Makefile.inc

+             $(wildcard $(SDIR)/config/*.c)
+
+ifdef MODULAR
+   SRC_C    += $(SDIR)/protocol/protocol.c


I never liked this. Modular builds are a target thing, and don't belong in the main makefile. but it is weird because the protocol code itself is here. I'm not sure what to do about it.

PhracturedBlue · 2019-03-03T13:10:40Z

src/Makefile.inc

+# Note that this must be after the 1st rule so that it doesn't execute by default#
+##################################################################################
+$(VERBOSE).SILENT:
+


This is one of the thinks I hate about Makefiles. why does the order of things like this matter? it is completely non-intuitive

PhracturedBlue · 2019-03-03T13:13:21Z

src/Makefile.inc

+zip: $(TARGET).zip
+
+%.zip: $(ALL) $(TARGET).dfu $(PROTO_MODULES)
+	#This is not an emulator build (emulator is hanled in target/common/emu/Makefile.inc)


knowledge of anything dfu related is target based and shouldn't be in the main file....but tis the general zip logic common to all tx? it is different for emus....it is at least somewhat different for radiolnk and opentx. Not sure if we can break this up or move it without making it either duplicated or hard to read.

PhracturedBlue · 2019-03-03T13:16:42Z

src/Makefile.inc

+#The main executable #
+######################
+$(TARGET).$(EXEEXT): $(LINKFILE) $(OBJS) $(LIBOPENCM3)
+	@echo " + Building '$@'"


libopencm3 stuff is target related. it really doesn't belong in the main makefile. except that it currently lives in the src/ directory. maybe it should go into a libs/ directory or under target somewhere...I dunno

sure. we can. we need first define the MACROs and document somewhere. then the code can be much clean.

PhracturedBlue · 2019-03-03T13:22:09Z

src/Makefile.inc

+
+# Use VERBOSE=1 to enable verbose make
+PROGMODE ?= STATUS_SCREEN
+


What is this? I can't find any reference to it anywhere else. No idea why we have it.

i don't know.

Move distclean to root Makefile Fix emu_devo8 move back from firmware

PhracturedBlue · 2019-03-08T14:10:51Z

src/Makefile.inc

 ifeq ("$(SRC_CXX)", "")
-	$(CC) -o $@ $(OBJS) $(LIBOPENCM3) $(LFLAGS) $(CFLAGS) $(EXTRA_CFLAGS)
+	$(CC) -o $@ $(OBJS) $(LIBOPENCM3) $(LFLAGS) $(LFLAGS2) $(CFLAGS) $(EXTRA_CFLAGS)
 else


Can we name his better? I have no idea what the difference is between LFLAGS and LFLAGS2 (Yes I know you didn't create it, but now is a good time to fix it)

PhracturedBlue · 2019-03-08T14:13:45Z

src/target/tx/devo/devo10/Makefile

 OPTIMIZE_DFU := 1

+TARGET=devo10
+


Why do I need this in every Makefile? Isn't it inferrable by the directory I'm in? The less things I need to define the better.
On the other hand, hiding away definitions makes things harder to read...I'm torn

I struggled as well.
I can pass TARGET from Makefile, maybe I should do that.

PhracturedBlue · 2019-03-08T14:15:57Z

src/Makefile

-ALLEMUS = $(foreach dir,$(ALLTXS:%=emu_%),$(if $(wildcard target/$(dir)),$(dir),))
+EMUS = $(addprefix emu_, $(TXS))
+ALLEMUS = $(addprefix emu_, $(ALLTXS))



Does this work if not every target has an emu?

I need to filter out some targets. but since we put target under family, it is a challenge to do so.

PhracturedBlue · 2019-03-08T14:18:20Z

src/target/tx/devo/emu_devof7/Makefile


-include target/drivers/mcu/emu/Makefile.inc
+SRC += $(SDIR)/target/tx/devo/devof7/ia9211_map.c
+CFLAGS +=-I$(SDIR)/target/drivers/display/spi/


This is weird to me. the ia9211_map file is emu only but lives in the devof7 dir?

no. it is a include file used by devof7 lcd driver as well.

howard0su requested a review from PhracturedBlue March 3, 2019 11:54

PhracturedBlue reviewed Mar 3, 2019

View reviewed changes

Refector Makefile

b6682f4

Move distclean to root Makefile Fix emu_devo8 move back from firmware

howard0su force-pushed the makefile branch from 0ff1cac to b6682f4 Compare March 3, 2019 14:05

howard0su added 12 commits March 3, 2019 22:18

fix target lookup

8d49805

Add VPATH

29427d7

use ODIR for optmize.ld

6443e5f

TEMP

d572f63

more fixes

64512a0

add global targets

c8a16ca

Fix build for Tx

c9e7d0f

Rename makefile.inc to makefile

d7f40eb

Merge remote-tracking branch 'upstream/master' into makefile

b1293b3

rename makefile.inc to makefile

041ec47

Get txs and emus build

c3dd365

Fix at9 and at10

63588ec

PhracturedBlue reviewed Mar 8, 2019

View reviewed changes

howard0su added 3 commits March 8, 2019 22:35

fix t12

6786457

uniq function has bug

0ccc9fe

Introduce FS_ROOT variable

aef7152


		# Use VERBOSE=1 to enable verbose make
		PROGMODE ?= STATUS_SCREEN

		OPTIMIZE_DFU := 1

		TARGET=devo10

Conversation

howard0su commented Mar 3, 2019

Uh oh!

PhracturedBlue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PhracturedBlue left a comment •

edited

Loading