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

Support for Docker and OSX for esp-open-sdk toolchain #14

Merged
merged 16 commits into from
Aug 25, 2016

Conversation

brainstorm
Copy link
Contributor

@brainstorm brainstorm commented Aug 21, 2016

Hello @cnlohr and @con-f-use,

I really appreciate your work in the esp82xx and I appreciate even more that you are putting some efforts into generalizing your fantastic dev environment for the community, love it!

Since I'm an OSX user, I couldn't run your setup right away. Instead, I've successfully dockerized @pfalcon's esp-open-sdk (which should fix pfalcon/esp-open-sdk#170, btw). This way I don't have to install the whole toolchain locally, hopefully facilitating portable cross-compiling toolchains for everyone else, see here for more context:

dockcross/dockcross#26

http://blogs.nopcode.org/brainstorm/2016/07/26/cross-compiling-with-docker

Anyway, I've tried to keep this PR as unintrusive as possible, not getting in your dev env way (i.e: docker is off by default in user.cfg). Happy to tweak it further to fit your needs and see it integrated upstream.

Cheers!

@brainstorm
Copy link
Contributor Author

Uses this docker hub image:

https://hub.docker.com/r/brainstorm/dockcross-esp-open-sdk/

For which I intend to reduce its disk footprint by using Alpine Linux or similar in the near future (so don't get scared by its 3G filesize, that will get down in size pretty soon)

@con-f-use
Copy link
Collaborator

Just to be sure regarding size. Did you delete all unnecessary files that esp-open-sdk creates? There is quite a lot of bloat of sources in there that don't need to remain after the build is done. At 3 GB I assume the bulk already is Linux components, but just wanted to check.

@brainstorm brainstorm changed the title Docker support for esp82xx Docker support for esp-open-sdk toolchain Aug 21, 2016

ifeq ($(strip $(DOCKER)), yes)
INCL = $(SDK)/include $(DOCKER_PREFIX)/esp82xx/include $(DOCKER_PREFIX)
SRCS = $(DOCKER_PREFIX)/esp82xx/fwsrc/uart.c \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@con-f-use ... how would you collapse those with a Makefile $(foreach) loop? It's just adding a prefix to each .c in SRCS :/

This c&p feels a bit too ugly to leave it as it is now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make has it's own filter and prefix functions. Also the += operator might be useful for INCL.

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 21, 2016

@con-f-use, very good point there! No, I'm not deleting those yet. Will check if esp-open-sdk has a special Makefile target for that post-install... Other than the tarball and that target, will check other things to remove, chime in if you have more files I can remove safely.

@con-f-use
Copy link
Collaborator

Almost everything in crosstool-NG/.build/ is unnecessary. Especially the tarball, src and (if I remember correctly) xtensa-elf directories, which is the bulk of data.

@cnlohr
Copy link
Owner

cnlohr commented Aug 22, 2016

Hmm... I kept going without merging... Now there's some conflicts... I was wondering, @con-f-use ... Do you see anything wrong with this as it is? Did you want your issues to be addressed by @brainstorm before merging?

@brainstorm
Copy link
Contributor Author

No worries @cnlohr, just don't merge for now, I'm optimizing the docker filesize by using Alpine Linux and bringing in @con-f-use's feedback. Will make that mergeable again sometime this week, sorry for the confusion ;)

Btw (offtopic-ish), did you guys see what espressif is up lately regarding dev environment(s)?:

https://github.com/espressif/esp-idf

@cnlohr
Copy link
Owner

cnlohr commented Aug 22, 2016

I did not see it!

@con-f-use
Copy link
Collaborator

Not sure about the README change here. You realize these are instructions on starting a new top-level project where esp82xx is a submodule?

@@ -150,14 +150,14 @@ Cope with submodules in top-level projects updates:

  - Then bump the version in the main project root folder:

-        git init project_name
-        cd project_name
-        git submodule add [email protected]:cnlohr/esp82xx.git
-        cp esp82xx/user.cfg.example user.cfg
-        cp esp82xx/Makefile.example Makefile
-        mkdir -p web/page
-        mkdir user
-        ln -s esp82xx/web/Makefile web/
+    ```
+    cd esp82xx
+    git pull
+    cd ..
+    git add esp82xx
+    git commit -m 'Bumped submodule version'
+    git push
+    ```

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 24, 2016

I'm confused, @con-f-use (really, swear, no bad pun intended)... are those changes on the README.md mine? I might have messed up with the branches perhaps? Will check ASAP.

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 24, 2016

Other than the common.h issue, this should be mergeable... not sure about the README.md changes you are mentioning @con-f-use, since they don't seem to be in this PR :_S

I'll try to use the dev branch on new PRs. Thanks both for your feedback and help!

@brainstorm
Copy link
Contributor Author

Eventually I think I'll switch to @vowstar's Docker images, they seem quite cleaner than mine:

https://github.com/vowstar/esp8266

@vowstar, I wonder if you have plans to support SDK 2.x there (if not already supported)?

@con-f-use
Copy link
Collaborator

Thanks for your effort!

Re: ReadMe - No idea where these changes came from, but they were there. Did you do a git push origin +HEAD^:master? Anyway, doesn't matter, it's all fine now. I will leave the merging to Charles.

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

Not sure yet, looks like the SDK variable is unset.

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 25, 2016

Oh I just noticed this nonsense I wrote, here's a mini patch:

mba-3:esp82xx romanvg$ git diff
diff --git a/common.mf b/common.mf
index bfb54ef..e827fd0 100644
--- a/common.mf
+++ b/common.mf
@@ -26,9 +26,7 @@ OPTS += -DWEB_PORT=$(WEB_PORT) -DCOM_PORT=$(COM_PORT) -DBACKEND_PORT=$(BACKEND_P
 ESP_ROOT := $(shell echo "$$ESP_ROOT")
 ifeq ($(strip $(DOCKER)), yes)
     ESP_ROOT := $(DOCKER_SDK_DEFAULT)
-else ifeq ($(strip $(ESP_ROOT)),)
-    $(warning Warning: No shell variable 'ESP_ROOT', using '$(SDK_DEFAULT)')
-    ESP_ROOT := $(SDK_DEFAULT)
+else
     DOCKCROSS =
 endif
 ifeq ($(strip $(ESP_ROOT)),)

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

Nope. If this line exists at all, it seems to break the variables.

$(TARGET) : $(SRCS) Makefile
    echo $(LINKFLAGS)
>>> $(eval $(call inside_docker)) <<<
    $(CC) $(CFLAGS) $(SRCS) -flto $(LINKFLAGS) -o $(TARGET)

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

I don't know. Is it possible to undelete your branch? That way I can revert this back out? I just can't seem to get the conditional to stop firing.

ifeq ($(strip $(DOCKER)), yes) Seems to always return true, even if it's false.

@brainstorm brainstorm restored the docker_support branch August 25, 2016 06:43
@brainstorm
Copy link
Contributor Author

Bummer, just restored the branch. Sorry @cnlohr, we should have tested it more thoroughly locally before merging into master :/

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

I found another bug...

    INCL += $(DOCKER_PREFIX)/esp82xx/include $(DOCKER_PREFIX)
    SRCS:=$(addprefix $(DOCKER_PREFIX), $(SRCS))
    FW_FILE1:=$(addprefix $(DOCKER_PREFIX), $(FW_FILE1))
    FW_FILE2:=$(addprefix $(DOCKER_PREFIX), $(FW_FILE2))
    TARGET:=$(addprefix $(DOCKER_PREFIX), $(TARGET))
    CC:=$(DOCKCROSS) $(CC)
    ESP_ROOT:=$(DOCKER_SDK_DEFAULT)
    ESPTOOL_PY:=$(DOCKCROSS) $(ESPTOOL_PY)

is what the set should be (I think) using eval seems to disregard the ifeq's etc.

@brainstorm
Copy link
Contributor Author

Thanks for that, Charles, don't know why I went for $(eval) on those in the first place... Will look at the ifeq ($(strip $(DOCKER)), yes) now... no idea why it's always true :/ Any hints @con-f-use?

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

Well, I don't know if I learned anything about git, but, after accidentally committing things the wrong way twice, at least the current master and dev both work. Aaaaaaa. @con-f-use does any more cleanup need to be done? Can we somehow pretend everything I did didn't happen?

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

p.s. Docker support. Is there any way of making it less intrusive? Like having it limited to just a small section of one of the .mf's?

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

Oh man! Now, I don't know how to re-open this pull request. I need to sleep. I really hope I start to understand this git thing more soon.

@con-f-use
Copy link
Collaborator

con-f-use commented Aug 25, 2016

Okay, I just got up and my mailbox was full. o_O

Can we somehow pretend everything I did didn't happen?

Well, yes we can. We can revert commits, by pointing HEAD to a past commit. This should almost NEVER be done, and is frowned upon for very good reasons.¹ However, I think this time it's warrented here.

{edit} I did it. Should be fine now. Hope I didn't go back too much! {/edit}


Warning (and how it was done)

These commands are absolutely lethal. Do this to the dev branch only and have a backup locally (totally independent clone that you don't use for any of this). Ensure no one commits or changes anything until completely done. Do not issue these commands twice (by accident). Only ever go back ONE commit, then double check if you did the right thing. If you want to go back more than one commit, repeat, then check, repeat, then check, ..., one commit at a time.

So the syntax to go one commit back is:

# Never do this!
git checkout dev
git push origin +HEAD^:dev   # Point head in the remote dev branch seven commits back
git reset --hard HEAD~     # Do the same locally

Once you are done with dev and are 199% positive this is what you want, you can do the same process of going back, then checking with master (merging dev into master does not work).

¹ Reasons this is never used: 1) If people are using your faulty commits, they end up with a detached head or a submodule pointing to nothing 2) If you mess up, the commit with this specific hash is forever gone - even if you commit the same changes again, it will have another hash 3) It's incredibly easy to mess this up 4) Merging becomes more difficult if others used your faulty commits

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 25, 2016

Oh dear, sorry for all the mess I've caused :_S

@cnlohr point taken, I'm extracting the docker stuff in its own docker.mf now. And don't worry about reopening the PR, I'll submit another one, this time against dev (master was the only option when I started hacking this together).

Still a bit puzzled about that misbehaving ifeq in inside_docker makefile function:

# Cross-compile toolchain in a docker image
ifeq (yes,yes)
/bin/sh: -c: line 0: syntax error near unexpected token `yes,yes'
/bin/sh: -c: line 0: `ifeq (yes,yes)'
make: *** [image.elf] Error 2

Was working neat (enabling/disabling docker support in my machine)... oh well, lesson learned: more local tests before merging for real, sigh.

@brainstorm
Copy link
Contributor Author

@cnlohr Seems that you are on the roll again, but just in case, for next time: http://megakemp.com/2016/08/25/git-undo/

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

  1. @con-f-use Is there any convenient way to force my local repo for like espusb to revert back?
  2. Your README changes have been lost, the ones that included the bit about not fearing about the trash at startup.
34,35c34,35
<     cp esp28xx/user.cfg.example user.cfg
<     cp esp28xx/Makefile.example Makefile
---
>     cp esp82xx/user.cfg.example user.cfg
>     cp esp82xx/Makefile.example Makefile
37c37
<     ln -s esp28xx/web/Makefile web/
---
>     ln -s esp82xx/web/Makefile web/
105a106,108
> For general troubleshooting hints, see [esptools troubleshooting page](https://github.com/themadinventor/esptool#troubleshooting).
> It is excellent!
> 
116a120,125
> We try to keep the generally interesting stuff on top.
> 
> ### Gibberish Serial Data right after Boot
> 
> If you get weird data at the start of your serial communication with the esp, don't forsake!
> The boot rom writes a log to the UART with the unusual timing of 74880 baud.

May want to cleanly merge.

Still looking forward to docker, next time I won't merge something right before I have to go to sleep.

@cnlohr
Copy link
Owner

cnlohr commented Aug 25, 2016

no idea

@con-f-use
Copy link
Collaborator

con-f-use commented Aug 25, 2016

cn-f-use
*My actual pets

@cnlohr
Copy link
Owner

cnlohr commented Aug 26, 2016

Any way for this to be re-opened? Also, I really hope that isn't in reference to you getting sick and tired of me. Maybe a new pull request?

@brainstorm
Copy link
Contributor Author

brainstorm commented Aug 26, 2016

Yeah! Will reopen that against dev as soon as I'm happy with refactoring
out docker parts from the makefile...

@cnlohr
Copy link
Owner

cnlohr commented Aug 26, 2016

I hope you feel better.

@brainstorm
Copy link
Contributor Author

No worries @cnlohr and thanks for your patience dealing with this! I'm feeling a bit better now... I need assistance with Makefiles though, it's driving me nuts. I tried moving the docker logic away from the main.mf file:

brainstorm@d043004

But it seems like ifeq gets interpreted right away after the include instead of being evaluated when the actual inside_docker function is called:

$ make burn
# Cross-compile toolchain in a docker image
ifeq (yes,yes)
/bin/sh: -c: line 0: syntax error near unexpected token `yes,yes'
/bin/sh: -c: line 0: `ifeq (yes,yes)'
make: *** [image.elf] Error 2

@con-f-use, really could use a bit of help here. I wish this was implemeted in SnakeMake or similar so that I could fix it myself easily, but now I'm as stuck as @cnlohr with that merge yesterday :_/

@con-f-use
Copy link
Collaborator

con-f-use commented Aug 26, 2016

Also, I really hope that isn't in reference to you getting sick and tired of me.

Certainly not, just found it amusing.

@con-f-use, really could use a bit of help here

Sorry guys, don't expect anything from me the next few days or so. I'm pretty sick, too. Caught something on Wednesday and been puking ever since (inb4 tmi).

  1. @con-f-use Is there any convenient way to force my local repo for like espusb to revert back?

Not sure. I just deleted the faulty one and did a new git clone --recursive. Guess you could do a revert or move the local head pointer, but I'd save any changes.

@cnlohr
Copy link
Owner

cnlohr commented Sep 4, 2016

Any motion on this front? I really think it could be helpful.

@brainstorm
Copy link
Contributor Author

Got stuck factoring it out the main.mf:

brainstorm@d043004

But have no idea how to tell make to evaluate it as a function and to address the weird ifeq behavior :/

@cnlohr
Copy link
Owner

cnlohr commented Sep 4, 2016

Is there possibly a simpler way to go about this? Like ifeq'ing out an include statement?

@cnlohr
Copy link
Owner

cnlohr commented Sep 4, 2016

Then shepherding the docker stuff to an include?

@brainstorm
Copy link
Contributor Author

Not so easy if one does not want to replicate targets/variables from main.mf and common.mf. I'm already using an include for the docker functionality... not sure I see how to do sth similar with an ifeq around the include docker.mf :_/ Trying/shuffling code right now...

@brainstorm
Copy link
Contributor Author

Thinking about this today... I don't think this makes sense anymore until docker's hypervisors (bhyve, xhyve or hyperkit) get real with USB passthrough support:

https://docs.docker.com/docker-for-mac/faqs/#/can-i-pass-through-a-usb-device-to-a-container

The mess I created with the makefiles is really not worth it if at the end of the day it's not really portable... so installing the SDK locally anyway for now :/

@cnlohr
Copy link
Owner

cnlohr commented Dec 14, 2016

I'm always up for something else if it's good... but we'll have to see if any of that comes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants