Skip to content
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

completely refactored makefile #985

Closed
wants to merge 17 commits into from

Conversation

tius2000
Copy link
Contributor

@tius2000 tius2000 commented Feb 25, 2017

I recently started to create completely refactored makefile sming.mk for my rboot projects with many additions:

  • roms with and without bootloader
  • automatic flash address caclulation
  • automatic linker script creation
  • dependency tracking for spiffs sources
  • dependency tracking for all header files included in cpp and c sources
  • memanalyzer for rboot builds added
  • automatic patch of esp_init_data_default.bin for system_get_vdd33()
  • improved terminal handling for windows
  • allow duplicate source and header file names (in different directories)
  • EXTRA_SRC and EXTRA_OBJ options added
  • assembler support added

When reorganizing the file, I tried hard to group things that belong together. It is feature complete now, any comments are very welcome! It has a different name, so there are no collisions with the current makefiles. To use it, simple include it from your project makefile (see example).

OS-specific files for Windows, Linux, Darvin and FreeBSD are provided, but only the Windows has been tested so far (appart from the Travis build test).

It is up to the maintainers, whether this could be a starting point for a new unified makefile or might just serve as an inspiration for further development of the existing makefiles.

@anakod
Copy link
Member

anakod commented Feb 28, 2017

Refactoring looks nice and more clear. But what about back compatibility?

Copy link
Contributor

@slaff slaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance the changes look good. You should get rid of Perl as dependency and use python, which we already use for esptool.py and memanalyzer.py. Also I saw only flashing of rBoot based projects. Can you point out where is the case where you can create ROMs that do not need bootloader ( the old two roms setup with one at 0x00 and another one with iram1_text at 0x4.. ?

Sming/sming.mk Outdated
#==============================================================================
# do flash address calculations
#------------------------------------------------------------------------------
hexcalc = $(shell $(PERL) -e"printf '0x%06X', $(1)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to depend on python here instead of asking the users of Sming to install also perl on their machines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, python is not a requirement for the windows make process as esptool.py and memanalyzer.py are provided in compiled form within the udk. I do not have any experience with python, but the following command should work:

hexcalc = $(shell $(PYTHON) -c"print(format(int($(1)),'\#08x'))")

However, I do not know a replacement for this line

$(PERL) -ple "s{(^\s*irom0_0_seg *: *).*}{\\1org = $(IROM0_ORG0), len = $(IROM0_SIZE)}" $< >$@

so any help is welcome!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esptool.py and memanalyzer.py are provided in compiled form

Thanks for bringing to my attention that we are using esptool.exe. I will have to check if that is compiled python file or some other esptool. If it is the latter we should move to esptool.py to have all the latest advantages from it.

@ADiea
Copy link
Contributor

ADiea commented Feb 28, 2017

+1 looking nice!

@tius2000
Copy link
Contributor Author

Thank you @anakod! I tried hard not to break things and keep backward compatibility. However, only rboot roms are supported so far and much more testing will be required.

@tius2000
Copy link
Contributor Author

Hi @slaff, I like the idea of creating roms without bootloader! However, I'm not familiar with that option. Is there a sample linker file (e. g. for Basic_Blink)? Is there anything else that needs to be changed beside the linker file? I will try to include that option, too.

@riban-bw
Copy link
Contributor

riban-bw commented Mar 5, 2017

However, I do not know a replacement for this line
$(PERL) -ple "s{(^\s*irom0_0_seg *: *).*}{\\1org = $(IROM0_ORG0), len = $(IROM0_SIZE)}" $< >$@

If you explain what this perl code does then someone may be able to offer a python alternative. There are fewer developers who know both perl and python than know just one!

@tius2000
Copy link
Contributor Author

tius2000 commented Mar 5, 2017

@riban-bw: This line is just a simple search and replace with a regular expression, something a *nix user probably would use sed for. However, I wanted to minimize the number of required tools for windows builds and used perl instead.

@slaff slaff added this to the 3.2.0 milestone Mar 6, 2017
@slaff
Copy link
Contributor

slaff commented Mar 6, 2017

something a *nix user probably would use sed for.

Then, please, replace it with sed. Sed is also part of cygwin/mingw for Windows.

@tius2000 tius2000 changed the title complete refactored makefile completely refactored makefile Mar 6, 2017
- use sed for linker file creation instead of Perl
- removed vpath to avoid source file collisions
- more specific include paths to avoid header file collisions
- EXTRA_SRC added
@tius2000
Copy link
Contributor Author

tius2000 commented Mar 6, 2017

I removed the dependency on Perl and changed the directory handling to allow projects with multiple directories with duplicate names for source and header files. A new option EXTRA_SRC was added.

ifndef SMING_HOME
$(error SMING_HOME is not set. Please configure it in Makefile-user.mk)
endif
ifndef ESP_HOME
Copy link
Member

@anakod anakod Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remove SMING_HOME check from this file? Where it should be declared?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it is a good idea not to have settings regarding the build enviroment withing the project files. That is why I think it is better to set SMING_HOME as as a enviroment variable. The makefile than can find and include sming.mk and all plattform specific settings (e. g. windows.mk).

@anakod
Copy link
Member

anakod commented Mar 7, 2017

Now we have 3 main makefiles:

  1. Makefile (to build Sming itself)
  2. Makefile-project.mk (to build classic projects)
  3. Makefile-rboot.mk (to build rboot projects)
  • we have 4 additional system related Makefile (Win, Linux, MacOS, FreeBSD)

I don't think what we should add additional 4th Makefile. It should replace some of previous files instead.
You plan to implement only rboot build or classic projects also? Because I'm sure will be right to have single universal makefile instead of two independend for both project types.

@tius2000
Copy link
Contributor Author

tius2000 commented Mar 7, 2017

Hi @anakod, thank you for your feedback. You are right, we should not have more makefiles but less. I did not plan to include support for the Espressif bootloader (classic projects) unless there is a real need (see discussion #971).

However, I really like @slaff's idea of builds without any bootloader and would like to include that as an option. Perhaps someone can give me a hint where to find the correct linker and flash settings for this?

Of course, this PR may also just serve as an inspiration for further development of the existing makefiles.

@slaff
Copy link
Contributor

slaff commented Mar 7, 2017

However, I really like @slaff's idea of builds without any bootloader and would like to include that as an option. Perhaps someone can give me a hint where to find the correct linker and flash settings for this?

@tius2000 If you compile the current Basic_Blink sample, or any other non-rboot projects, you will actually create a standalone application. The application comes with two ROM files, the one starting at 0x000 contains the .text .data .rodata sections (IRAM and DRAM) and the other one contains libraries/SDK code (SPI Flash )

esptool2 -quiet -bin -boot0 out/build/app.out out/firmware/0x00000.bin .text .data .rodata
# --boot0 above is for single, non-bootloaded apps.
esptool2 -quiet -lib out/build/app.out out/firmware/0x0a000.bin 

@anakod
Copy link
Member

anakod commented Mar 7, 2017

I did not plan to include support for the Espressif bootloader

That's right, we decide remove support for old Espressif bootloader.

(classic projects)

Classic project - I mean project without bootloader - single application splitted to two binaries as @slaff described above (or both binaries can be joined to single).

ANOTHER OPTION
Also we can drop support for applications without rboot bootloader at all. We can consider this option but:
1, We should provide solution compatible with old small 512 KB flash also
2. rBoot add additional delays in startup process. It can be really critical for battery powered deveices
3. In my opinion better to keep both options, it give us more flexibility

- major refactoring for consistent handling of rboot and non-rboot builds
- minor bug fixes
- feature complete now
@slaff slaff self-assigned this Mar 9, 2017
@anakod
Copy link
Member

anakod commented Mar 9, 2017

I don't think what current solution with two different makefiles is good, even if we skip question about code quality\comments.We have too many related issues during last month (just for example after moving debugf to irom and other).

WITHOUT causing "pain" to our users.

Yes, it's first priority.

@slaff
Copy link
Contributor

slaff commented Mar 13, 2017

Some thoughts on the refactoring:

I) We have to give a chance to the users to adapt to the new makefile re-organization.
Therefore the following could help:

  • At the moment old and new makefiles are present. Please, replace the content of Makefile with the content from sming.mk and delete sming.mk.
  • Rename Linux.mk to Makefile-Linux.mk and remove the old Makefile-linux.mk file. Apply the same to the other OS makefiles.
  • For Makefile-rboot.mk and Makefile-project.mk should be present but having the following content:

1.) Info message $(info "...") saying that the Makefile-{rboot|project}.mk will be removed in the next versions of Sming
2.) Internally include the new makefile.

II) Issues

  • Assember .S files are included always.
AS system/irq_check.s
AS gdb/gdbstub-entry.s

While this is true for irq_check.s gdbstub-entry.s should be included ONLY when ENABLE_GDB=1 is given.

  • Compilation of the library with make -f sming.mk finishes with error:
sed -r "s/(^\s*irom0_0_seg *: *).*/\\1org = 0x4020A000, len = 0x0F6000/" /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//compiler/ld/rboot.rom0.ld >out/build/rom0.ld
LD out/build/app_0.out
out/build/app_app.a(user_main.o):(.text.user_init+0x4): undefined reference to `init()'
out/build/app_app.a(user_main.o): In function `user_init':
/home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/appinit/user_main.cpp:70: undefined reference to `init()'
collect2: error: ld returned 1 exit status
make: *** [out/build/app_0.out] Error 1

That has to be fixed.

III) Makefile not in synch with the latest changes. I will write shortly comments next to the line numbers.

Sming/Windows.mk Outdated
#------------------------------------------------------------------------------
ESPTOOL ?= $(SDK_TOOLS)/esptool.exe
GET_FILESIZE ?= stat --printf="%s"
MEMANALYZER ?= $(SDK_TOOLS)/memanalyzer.exe $(OBJDUMP).exe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Windows we also use the python version of memanalyzer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make python a new required for windows builds. Is there any advantage of the python version?

Copy link
Contributor

@slaff slaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good ideas and organizational changes, but still long way to go before merging this PR. See comments for details.

Sming/sming.mk Outdated
@@ -0,0 +1,662 @@
# sming.mk
#
# to do:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point take care to remove all comments from the top of the makefile and add them to the commit history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

EXTRA_LIBS ?=

# your extra source files (default: none)
EXTRA_SRC ?=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already variable called MODULES.Please, don't introduce yet another variable for the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that MODULES is used to specify directories? EXTRA_SRC is used to specify single files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXTRA_SRC is used to specify single files.

Ok, then leave it there.

Sming/sming.mk Outdated
# default serial settings
#------------------------------------------------------------------------------
# use the maximum speed supported by your board to get things done faster
COM_SPEED_ESPTOOL ?= 921600
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the maximum supported speed from most devices - 115200. Not only from some (http://digital.ni.com/public.nsf/allkb/D37754FFA24F7C3F86256706005B9BE7). change the COM_SPEED_SERIAL too. Otherwise we will be overwhelmed with issues saying that flashing aborted, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, you are probably right ;-)

Sming/sming.mk Outdated
OBJDUMP := $(XTENSA_TOOLS)/xtensa-lx106-elf-objdump
STRIP := $(XTENSA_TOOLS)/xtensa-lx106-elf-strip

PERL ?= perl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of Perl. Use stock bash commands and if bash cannot help only then use python.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check, whether it is possible to use standard bash commands instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in most cases python should be already available. Current Makefile require python as dependency, am I right? If yes - we can skip additional code branches and make things more simple. Python is good for calculation and other things like that in my opinion.

Copy link
Contributor Author

@tius2000 tius2000 Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, currently neither python nor perl are required for windows builds or are standard tools on windows machines. I will try to remove dependencies on both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sming/sming.mk Outdated
export COMPILE := gcc
export PATH := $(XTENSA_TOOLS):$(PATH)

ifeq ("$(PYTHON)","")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of Perl. Use stock bash commands and if bash cannot help only then use python.

ifneq ($(USER_FLASH_SIZE),0)
$(info USER_FLASH_ADDR = $(USER_FLASH_ADDR) ($(USER_FLASH_SIZE) bytes))
endif
ifneq ($(SPIFF_SIZE),0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need calculations for RBOOT and non-RBOOT projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, whether I understand this. There are calculations in both branches, but those lines are for debug info only.

# name for the target project
TARGET = app

APP_AR := $(BUILD_BASE)/$(TARGET)_app.a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should tackle RBOOT AND non-RBOOT projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a different library name for non-rboot?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, better use the same library name, and same application name.

#==============================================================================
# (re-)compile sming framework if required
#------------------------------------------------------------------------------
LIBSMING := sming
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use ':=' instead of '=' ? Is there any reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I always prefer ':=' if possible. It has less confusing effects (more conventional logic, no "backward assignments") for occasional make users.

#==============================================================================
# create custom linker scripts
#------------------------------------------------------------------------------
IROM0_ORG0 := $(call hexcalc, 0x40200000 + $(ROM0_ADDR))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should tackle RBOOT and non-RBOOT projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done by the different values for ROM0_ADDR. Does it not work for you?

@tius2000
Copy link
Contributor Author

Dear @slaff, thank you very much for reviewing! I agree, there is still a long way before that should be merge.

However, I was wondering, whether marking the new makefile as "experimental" in the beginning might be an option. That way we will not break things, but users willing to try the new options can give it try. You could also place a hint with $(info) within the old makefiles to motivate the user to give it a try.

@anakod
Copy link
Member

anakod commented Mar 13, 2017

I'm prefer to replace old Makefiles with your version, but please take attention to II) and III) points in @slaff comment above.

- printf and shell used for address calculations
- EXTRA_OBJ added
@tius2000
Copy link
Contributor Author

Well, IMHO top priority should be not to break any existing projects. As I do not think that we can fix any possible incompatibility for every existing project before the first release, it might be a good idea to move to the new makefile in four phases.

phase 1:
Use the new name and mark the new makefile as "experimental". Include a message in the old makefiles to tell the users that there is something new.

phase 2:
Fix and improve the new makefile based on user feedback.

phase 3:
Mark the old makefiles as "deprecated" and inform the users, that they should switch to the new make process.

phase 4:
Remove the old makefiles or replace them with a stub pointing to the new makefile.

This way we do not break things and our users keep complete control of there projects. What do you think?

@tius2000
Copy link
Contributor Author

II) Issues

Assember .S files are included always.

AS system/irq_check.s
AS gdb/gdbstub-entry.s

While this is true for irq_check.s gdbstub-entry.s should be included ONLY when ENABLE_GDB=1 is given.

The whole gdb directory structure looks very confusing to me. makefile-rboot.mk references $(THIRD_PARTY_DIR)/gdbstub and I kept this. However, this directory does not exist on my system. I see copies of gdbstub-entry.s in Sming/gdb and Sming/third-party/esp-gdbstub. This is why the compilation goes wrong.

Which copy should be removed? Which directory should be compiled when ENABLE_GDB is set?

@tius2000
Copy link
Contributor Author

Compilation of the library with make -f sming.mk finishes with error

The new makefile is meant as a replacement for makefile-rboot.mk and _makefile-project.mk. It is not meant also to replace Makefile at the moment. However, this might be a task for a later phase.

@slaff
Copy link
Contributor

slaff commented Mar 14, 2017

The new makefile is meant as a replacement for makefile-rboot.mk and _makefile-project.mk

Perfect. Make sure to add an info message guiding our users.

Which directory should be compiled when ENABLE_GDB is set?

If you compile the library / or a project with ENABLE_GDB=1 then

  1. there are new compile flags appended to the current ones.
  2. The $(THIRD_PARTY_DIR)/gdbstub directory is added to the directories that include header files
  3. The $(THIRD_PARTY_DIR)/gdbstub directory is added to the directories that contain source code and all (*.c, *.cpp AND *.s) files in it should be compiled.
else ifeq ($(ENABLE_GDB), 1)
	CFLAGS += -Og -ggdb -DGDBSTUB_FREERTOS=0 -DENABLE_GDB=1
	MODULES		 += $(THIRD_PARTY_DIR)/gdbstub
	EXTRA_INCDIR += $(THIRD_PARTY_DIR)/gdbstub
	STRIP := @true
else

@tius2000
Copy link
Contributor Author

If you compile the library / or a project with ENABLE_GDB=1 then

there are new compile flags appended to the current ones.
The $(THIRD_PARTY_DIR)/gdbstub directory is added to the directories that include header files
The $(THIRD_PARTY_DIR)/gdbstub directory is added to the directories that contain source code and all (*.c, *.cpp AND *.s) files in it should be compiled.

I kept this from makefile-rboot.mk. However, the path seems not to be correct. Is there something wrong with my setup?

- make clear, that the os specific files are new and belong to sming.mk
@slaff slaff removed this from the 3.2.0 milestone Nov 14, 2017
@slaff
Copy link
Contributor

slaff commented Nov 14, 2017

So far in the latest develop we have the following implemented:

  • roms with and without bootloader
  • automatic flash address calculation + automatic linker script adjustment per application
  • dependency tracking for all header files included in cpp and c sources ( that includes dependency tracking for spiffs sources )
  • memanalyzer for rboot builds added

What is still missing is that one:

  • automatic patch of esp_init_data_default.bin for system_get_vdd33()
    @tius2000 Is that still needed ? Can you refresh my memory and tell me why was that added and do we need to have it for all applications?

@tius2000
Copy link
Contributor Author

Hi @slaff, if you want to use system_get_vdd33(), you need to patch esp_init_data_default.bin, otherwise you will get wrong values.

@slaff slaff removed the 3 - Review label Apr 24, 2018
@slaff
Copy link
Contributor

slaff commented Oct 12, 2018

you need to patch esp_init_data_default.bin, otherwise you will get wrong values.

We can think about patching the physical params by wrapping around register_chipv6_phy. This way we will have better control over the phy data and be better prepared for changes in the phy params in new SDKs.

@slaff slaff closed this Jun 2, 2019
mikee47 added a commit to mikee47/Sming that referenced this pull request Jun 10, 2019
Rename `ResolveSourcePath` -> `AbsoluteSourcePath`
Rename `ResolveObjPath` -> `AbsoluteObjPath`
Add root source parameter
Don't mess about with `EXTRA_INCDIR`, that's for application code, use `INCDIR` directly
Add `EXTRA_SRCFILES` (as per SmingHub#985)
mikee47 added a commit to mikee47/Sming that referenced this pull request Jun 10, 2019
Rename `ResolveSourcePath` -> `AbsoluteSourcePath`
Rename `ResolveObjPath` -> `AbsoluteObjPath`
Add root source parameter
Don't mess about with `EXTRA_INCDIR`, that's for application code, use `INCDIR` directly
Add `EXTRA_SRCFILES` (as per SmingHub#985)
mikee47 added a commit to mikee47/Sming that referenced this pull request Jun 16, 2019
Rename `ResolveSourcePath` -> `AbsoluteSourcePath`
Rename `ResolveObjPath` -> `AbsoluteObjPath`
Add root source parameter
Don't mess about with `EXTRA_INCDIR`, that's for application code, use `INCDIR` directly
Add `EXTRA_SRCFILES` (as per SmingHub#985)
mikee47 added a commit to mikee47/Sming that referenced this pull request Jun 16, 2019
Rename `ResolveSourcePath` -> `AbsoluteSourcePath`
Rename `ResolveObjPath` -> `AbsoluteObjPath`
Add root source parameter
Don't mess about with `EXTRA_INCDIR`, that's for application code, use `INCDIR` directly
Add `EXTRA_SRCFILES` (as per SmingHub#985)
mikee47 added a commit to mikee47/Sming that referenced this pull request Jun 18, 2019
Rename `ResolveSourcePath` -> `AbsoluteSourcePath`
Rename `ResolveObjPath` -> `AbsoluteObjPath`
Add root source parameter
Don't mess about with `EXTRA_INCDIR`, that's for application code, use `INCDIR` directly
Add `EXTRA_SRCFILES` (as per SmingHub#985)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants