-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Conversation
Refactoring looks nice and more clear. But what about back compatibility? |
There was a problem hiding this 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)") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
+1 looking nice! |
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. |
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. |
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! |
@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. |
Then, please, replace it with |
- 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
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
Now we have 3 main makefiles:
I don't think what we should add additional 4th Makefile. It should replace some of previous files instead. |
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. |
@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 )
|
That's right, we decide remove support for old Espressif bootloader.
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 |
- major refactoring for consistent handling of rboot and non-rboot builds - minor bug fixes - feature complete now
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).
Yes, it's first priority. |
Some thoughts on the refactoring: I) We have to give a chance to the users to adapt to the new makefile re-organization.
1.) Info message II) Issues
While this is true for
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?= |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)","") |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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. |
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
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: phase 2: phase 3: phase 4: This way we do not break things and our users keep complete control of there projects. What do you think? |
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? |
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. |
Perfect. Make sure to add an info message guiding our users.
If you compile the library / or a project with ENABLE_GDB=1 then
|
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
So far in the latest
What is still missing is that one:
|
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. |
We can think about patching the physical params by wrapping around |
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)
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)
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)
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)
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)
I recently started to create completely refactored makefile sming.mk for my rboot projects with many additions:
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.